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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------

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: -------