www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 4092] New: broken memory management for COM objects derived from IUnknown

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092

           Summary: broken memory management for COM objects derived from
                    IUnknown
           Product: D
           Version: unspecified
          Platform: Other
        OS/Version: Windows
            Status: NEW
          Severity: normal
          Priority: P2
         Component: druntime
        AssignedTo: sean invisibleduck.org
        ReportedBy: r.sagitario gmx.de


--- Comment #0 from Rainer Schuetze <r.sagitario gmx.de> 2010-04-14 23:44:39
PDT ---
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

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Apr 14 2010
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #1 from Rainer Schuetze <r.sagitario gmx.de> 2010-05-01 00:14:19
PDT ---
I just recently noticed, that AddRef and Release should use addRoot and
removeRoot, not addRange and removeRange.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
May 01 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092


Trass3r <mrmocool gmx.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mrmocool gmx.de


--- Comment #2 from Trass3r <mrmocool gmx.de> 2010-08-04 06:10:42 PDT ---
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

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Aug 04 2010
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092


Walter Bright <bugzilla digitalmars.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugzilla digitalmars.com


--- Comment #3 from Walter Bright <bugzilla digitalmars.com> 2011-02-07
22:12:48 PST ---
I'm not comfortable with this change. COM objects should be refcounted, not
gc'd.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 07 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #4 from Rainer Schuetze <r.sagitario gmx.de> 2011-02-08 00:41:49
PST ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 08 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #5 from Rainer Schuetze <r.sagitario gmx.de> 2011-02-15 23:18:19
PST ---
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
}

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Feb 15 2011
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092


Andrej Mitrovic <andrej.mitrovich gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrej.mitrovich gmail.com


--- Comment #6 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-09-27
07:10:31 PDT ---
(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__ //-------------------------------------- +/ ----- -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #7 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-09-27
07:11:21 PDT ---
(In reply to comment #6)
 Can a a synchronized block be used instead?
Actually `atomicOp!"+="(count, 1)` could be used as well, no? -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #8 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-09-27
07:16:42 PDT ---
(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); -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #9 from Rainer Schuetze <r.sagitario gmx.de> 2013-09-27 08:33:05
PDT ---
 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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #10 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-09-27
08:56:35 PDT ---
(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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #11 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-09-27
09:00:44 PDT ---
 (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? -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #12 from Rainer Schuetze <r.sagitario gmx.de> 2013-09-27 13:31:42
PDT ---
(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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #13 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-09-27
13:33:33 PDT ---
(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? -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 27 2013
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #14 from Rainer Schuetze <r.sagitario gmx.de> 2013-09-27 13:52:28
PDT ---
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.

-- 
Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
Sep 27 2013
prev sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=4092



--- Comment #15 from Andrej Mitrovic <andrej.mitrovich gmail.com> 2013-09-28
04:20:28 PDT ---
(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. -- Configure issuemail: http://d.puremagic.com/issues/userprefs.cgi?tab=email ------- You are receiving this mail because: -------
Sep 28 2013