Bug 4092 – broken memory management for COM objects derived from IUnknown

Currently, instances of classes that derive from IUnknown are allocated from the C-heap (see lifetime.d, function _d_newclass), but are never released in the default implementation ComObject (see std.c.windows.com, ComObject.Release()), because invariants might still be called. In addition, ComObjects are not known to the garbage collector (which is completely useless in at least 99% of all cases), so you have to override ComObject's new to avoid a bad collection while executing the class constructor. I suggest allocating ComObjects from standard garbage collected objects, and let the default imlpementation add ranges to the GC when AddRef is called with reference count 0, and removing the range when Release is called: class ComObject : IUnknown { // [... QueryInterface ...] override ULONG AddRef() { LONG lRef = InterlockedIncrement(&count); if(lRef == 1) { uint sz = this.classinfo.init.length; GC.addRange(cast(void*) this, sz); } return lRef; } override ULONG Release() { LONG lRef = InterlockedDecrement(&count); if (lRef == 0) { GC.removeRange(cast(void*) this); } return cast(ULONG)lRef; } } Otherwise, the garbage collector is responsible of releasing memory. This also lets ComObject be handled like normal objects in D-Code (no need to call AddRef/Release). Here's the patch to lifetime.d: Index: lifetime.d =================================================================== --- lifetime.d (revision 282) +++ lifetime.d (working copy) @@ -98,7 +98,7 @@ void* p; debug(PRINTF) printf("_d_newclass(ci = %p, %s)\n", ci, cast(char *)ci.name); - if (ci.m_flags & 1) // if COM object + if (0 & ci.m_flags & 1) // if COM object { /* COM objects are not garbage collected, they are reference counted * using AddRef() and Release(). They get free'd by C's free() * function called by Release() when Release()'s reference count goes

I just recently noticed, that AddRef and Release should use addRoot and removeRoot, not addRange and removeRange.

Then also std.windows.iunknown should be removed and pragma(lib, "uuid") added to std.c.windows.com as mentioned there: http://d.puremagic.com/issues/show_bug.cgi?id=4295

I'm not comfortable with this change. COM objects should be refcounted, not gc'd.

The problem with allocating COM objects from the C-heap is that they cannot be free'd inside Release() due to possible invariants being called after that. Here's the implementation of Release in std.c.windows.com: ULONG Release() { LONG lRef = InterlockedDecrement(&count); if (lRef == 0) { // free object // If we delete this object, then the postinvariant called upon // return from Release() will fail. // Just let the GC reap it. //delete this; return 0; } return cast(ULONG)lRef; } The comment even implies that the memory should be taken from the GC. Also, any object that has references into other memory blocks needs to add itself as a root to the GC, which can be very easily forgotten (e.g. if the references are just strings). As reported lately, the juno project (http://www.dsource.org/projects/juno/wiki, probably the largest project trying to embrace COM), works similar as proposed here. ( http://www.digitalmars.com/pnews/read.php?server=news.digitalmars.com&group=digitalmars.D&artnum=128956 ) Agreed, changing this might break some code, but probably most applications creating COM objects have overloaded new anyway.

Overloading new seems to do the job without the patch in the runtime, so here is my current implementation of COM objects: extern (C) void* gc_malloc( size_t sz, uint ba = 0 ); class ComObject : IUnknown { new(uint size) { void* p = gc_malloc(size, 1); // BlkAttr.FINALIZE return p; } override HRESULT QueryInterface(in IID* riid, void** ppv) { ... } override ULONG AddRef() { LONG lRef = InterlockedIncrement(&count); if(lRef == 1) GC.addRoot(cast(void*) this); return lRef; } override ULONG Release() { LONG lRef = InterlockedDecrement(&count); if (lRef == 0) GC.removeRoot(cast(void*) this); return cast(ULONG)lRef; } LONG count = 0; // object reference count }

(In reply to comment #5) > new(uint size) Should likely be size_t. > LONG lRef = InterlockedIncrement(&count); > LONG lRef = InterlockedDecrement(&count); Can a a synchronized block be used instead? These functions are declared in core.sys.win*, but they're commented out in dsource's WinAPI, with these comments: ----- /+ //-------------------------------------- // These functions are problematic version(UseNtoSKernel) {}else { /* CAREFUL: These are exported from ntoskrnl.exe and declared in winddk.h as __fastcall functions, but are exported from kernel32.dll as __stdcall */ static if (_WIN32_WINNT >= 0x0501) { VOID InitializeSListHead(PSLIST_HEADER); } LONG InterlockedCompareExchange(LPLONG, LONG, LONG); // PVOID WINAPI InterlockedCompareExchangePointer(PVOID*, PVOID, PVOID); (PVOID)InterlockedCompareExchange((LPLONG)(d) (PVOID)InterlockedCompareExchange((LPLONG)(d), (LONG)(e), (LONG)(c)) LONG InterlockedDecrement(LPLONG); LONG InterlockedExchange(LPLONG, LONG); // PVOID WINAPI InterlockedExchangePointer(PVOID*, PVOID); (PVOID)InterlockedExchange((LPLONG)((PVOID)InterlockedExchange((LPLONG)(t), (LONG)(v)) LONG InterlockedExchangeAdd(LPLONG, LONG); static if (_WIN32_WINNT >= 0x0501) { PSLIST_ENTRY InterlockedFlushSList(PSLIST_HEADER); } LONG InterlockedIncrement(LPLONG); static if (_WIN32_WINNT >= 0x0501) { PSLIST_ENTRY InterlockedPopEntrySList(PSLIST_HEADER); PSLIST_ENTRY InterlockedPushEntrySList(PSLIST_HEADER, PSLIST_ENTRY); } } // #endif // __USE_NTOSKRNL__ //-------------------------------------- +/ -----

(In reply to comment #6) > Can a a synchronized block be used instead? Actually `atomicOp!"+="(count, 1)` could be used as well, no?

(In reply to comment #5) > void* p = gc_malloc(size, 1); // BlkAttr.FINALIZE Also, this can actually now be: return GC.malloc(size, GC.BlkAttr.FINALIZE);

> Actually `atomicOp!"+="(count, 1)` could be used as well, no? Yes, that would be better than a synchronized block. > Also, this can actually now be: > return GC.malloc(size, GC.BlkAttr.FINALIZE); Yes. Unfortunately the introduction of precise scanning makes it impossible to use overloading new, because you don't get the actual TypeInfo for the allocation. I have switched to a template factory method since then: https://github.com/D-Programming-Language/visuald/blob/master/stdext/com.d#L28 The patch in druntime would make this obsolete.

(In reply to comment #9) > https://github.com/D-Programming-Language/visuald/blob/master/stdext/com.d#L28 Nice, this will be useful for me. Thanks.

> (In reply to comment #9) > https://github.com/D-Programming-Language/visuald/blob/master/stdext/com.d#L28 Hmm, you seem to have this prototype in that file: extern(C) void* gc_malloc(size_t sz, uint ba = 0, const TypeInfo ti=null); However there is no such function which takes these parameters in druntime (in 2.064 anyway), the function takes 2 parameters, not 3. I suspect this links fine but would probably do something nasty like corrupt the stack. I suspect you've prototyped it to avoid an import into core.memory?

(In reply to comment #11) > Hmm, you seem to have this prototype in that file: > > extern(C) void* gc_malloc(size_t sz, uint ba = 0, const TypeInfo ti=null); This is the prototype for the precise GC, but it should do no harm elsewhere because the additional parameter in an extern(C) function is ignored by the called function and is removed from the stack by the callee. If you want to use the methods with the current GC and the respective imports, just leave out the third argument.

(In reply to comment #12) > This is the prototype for the precise GC, but it should do no harm elsewhere > because the additional parameter in an extern(C) function is ignored by the > called function and is removed from the stack by the callee. Ah ok, thanks. Just one other thing, it's often mentioned that the reference count of a COM object should always start at 1, but your ComObject class starts with 0. Maybe this isn't an issue because it's incremented in the QueryInterface() call?

The reference count might be slightly different than what's used in a language without GC, but I think starting at 0 is appropriate here. The object pointer can be used by the D program without any reference counting, it is only used to count (external) references to COM interfaces to avoid premature collection by keeping a root to the object in the GC.

(In reply to comment #9) > The patch in druntime would make this obsolete. This issue really does need to be resolved, e.g. the newCom function can't be used for COM classes with private constructors.

THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/17223 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB