- Document how thread-safety is ensured for each member of the

dlls/ole32/compobj_private.h

@@ -139,41 +139,52 @@ MARSHAL_Compare_Mids(wine_marshal_id *mid1,wine_marshal_id *mid2) {
 HRESULT MARSHAL_Disconnect_Proxies(APARTMENT *apt);
 HRESULT MARSHAL_GetStandardMarshalCF(LPVOID *ppv);
 
+/* Thread-safety Annotation Legend:
+ *
+ * RO   - The value is read only. It never changes after creation, so no
+ *        locking is required.
+ * LOCK - The value is written to only using Interlocked* functions.
+ * CS   - The value is read or written to with a critical section held.
+ *        The identifier following "CS" is the specific critical section that
+ *        must be used.
+ */
+
 /* an interface stub */
 struct ifstub   
 {
-    struct list       entry;
-    IRpcStubBuffer   *stubbuffer;
-    IID               iid;
-    IPID              ipid;
-    IUnknown         *iface;
-
-    BOOL              table;
+    struct list       entry;      /* entry in stub_manager->ifstubs list (CS stub_manager->lock) */
+    IRpcStubBuffer   *stubbuffer; /* RO */
+    IID               iid;        /* RO */
+    IPID              ipid;       /* RO */
+    IUnknown         *iface;      /* RO */
+    BOOL              table;      /* CS stub_manager->lock */
 };
 
 
 /* stub managers hold refs on the object and each interface stub */
 struct stub_manager
 {
-    struct list       entry;
-    struct list       ifstubs;
+    struct list       entry;      /* entry in apartment stubmgr list (CS apt->cs) */
+    struct list       ifstubs;    /* list of active ifstubs for the object (CS lock) */
     CRITICAL_SECTION  lock;
-    APARTMENT        *apt;        /* owning apt */
+    APARTMENT        *apt;        /* owning apt (RO) */
 
-    DWORD             refcount;   /* count of 'external' references */
-    OID               oid;        /* apartment-scoped unique identifier */
-    IUnknown         *object;     /* the object we are managing the stub for */
-    DWORD             next_ipid;  /* currently unused */
+    ULONG             extrefs;    /* number of 'external' references (LOCK) */
+    ULONG             refs;       /* internal reference count (CS apt->cs) */
+    OID               oid;        /* apartment-scoped unique identifier (RO) */
+    IUnknown         *object;     /* the object we are managing the stub for (RO) */
+    ULONG             next_ipid;  /* currently unused (LOCK) */
 };
 
+ULONG stub_manager_int_addref(struct stub_manager *This);
+ULONG stub_manager_int_release(struct stub_manager *This);
 struct stub_manager *new_stub_manager(APARTMENT *apt, IUnknown *object);
-int stub_manager_ref(struct stub_manager *m, int refs);
-int stub_manager_unref(struct stub_manager *m, int refs);
-IRpcStubBuffer *stub_manager_ipid_to_stubbuffer(struct stub_manager *m, IPID *iid);
+ULONG stub_manager_ext_addref(struct stub_manager *m, ULONG refs);
+ULONG stub_manager_ext_release(struct stub_manager *m, ULONG refs);
+IRpcStubBuffer *stub_manager_ipid_to_stubbuffer(struct stub_manager *m, const IPID *iid);
 struct ifstub *stub_manager_new_ifstub(struct stub_manager *m, IRpcStubBuffer *sb, IUnknown *iptr, REFIID iid, BOOL tablemarshal);
 struct stub_manager *get_stub_manager(OXID oxid, OID oid);
 struct stub_manager *get_stub_manager_from_object(OXID oxid, void *object);
-void stub_manager_delete_ifstub(struct stub_manager *m, IPID *ipid);
 
 IRpcStubBuffer *mid_to_stubbuffer(wine_marshal_id *mid);
 

dlls/ole32/marshal.c

@@ -83,6 +83,7 @@ typedef struct _wine_marshal_data {
 
 IRpcStubBuffer *mid_to_stubbuffer(wine_marshal_id *mid)
 {
+    IRpcStubBuffer *ret;
     struct stub_manager *m;
 
     if (!(m = get_stub_manager(mid->oxid, mid->oid)))
@@ -91,7 +92,10 @@ IRpcStubBuffer *mid_to_stubbuffer(wine_marshal_id *mid)
         return NULL;
     }
 
-    return stub_manager_ipid_to_stubbuffer(m, &mid->ipid);
+    ret = stub_manager_ipid_to_stubbuffer(m, &mid->ipid);
+
+    stub_manager_int_release(m);
+    return ret;
 }
 
 /* creates a new stub manager and sets mid->oid when mid->oid == 0 */
@@ -117,16 +121,23 @@ static HRESULT register_ifstub(wine_marshal_id *mid, REFIID riid, IUnknown *obj,
         COM_ApartmentRelease(apt);
         if (!manager) return E_OUTOFMEMORY;
 
-        if (!tablemarshal) stub_manager_ref(manager, 1);
-
         mid->oid = manager->oid;
     }
 
     ifstub = stub_manager_new_ifstub(manager, stub, obj, riid, tablemarshal);
     if (!ifstub)
+    {
+        stub_manager_int_release(manager);
+        /* FIXME: should we do another release to completely destroy the
+         * stub manager? */
         return E_OUTOFMEMORY;
+    }
+
+    if (!tablemarshal) stub_manager_ext_addref(manager, 1);
 
     mid->ipid = ifstub->ipid;
+
+    stub_manager_int_release(manager);
     return S_OK;
 }
 
@@ -510,6 +521,7 @@ StdMarshalImpl_MarshalInterface(
   if ((manager = get_stub_manager_from_object(mid.oxid, pUnk)))
   {
       mid.oid = manager->oid;
+      stub_manager_int_release(manager);
   }
   else
   {
@@ -560,7 +572,8 @@ StdMarshalImpl_UnmarshalInterface(LPMARSHAL iface, IStream *pStm, REFIID riid, v
   if (hres) return hres;
   
   /* check if we're marshalling back to ourselves */
-  if ((stubmgr = get_stub_manager(mid.oxid, mid.oid)))
+  /* FIXME: commented out until we can get the tests passing with it uncommented. */
+  if (/*(apt->oxid == mid.oxid) &&*/ (stubmgr = get_stub_manager(mid.oxid, mid.oid)))
   {
       TRACE("Unmarshalling object marshalled in same apartment for iid %s, returning original object %p\n", debugstr_guid(riid), stubmgr->object);
     
@@ -568,9 +581,10 @@ StdMarshalImpl_UnmarshalInterface(LPMARSHAL iface, IStream *pStm, REFIID riid, v
       if ((md.mshlflags & MSHLFLAGS_TABLESTRONG) || (md.mshlflags & MSHLFLAGS_TABLEWEAK))
           FIXME("table marshalling unimplemented\n");
       
-      /* clean up the stubs */
-      stub_manager_delete_ifstub(stubmgr, &mid.ipid);
-      stub_manager_unref(stubmgr, 1);
+      /* unref the ifstub. FIXME: only do this on success? */
+      stub_manager_ext_release(stubmgr, 1);
+
+      stub_manager_int_release(stubmgr);
       return hres;
   }
 
@@ -616,12 +630,9 @@ StdMarshalImpl_ReleaseMarshalData(LPMARSHAL iface, IStream *pStm) {
         return RPC_E_INVALID_OBJREF;
     }
     
-    /* currently, each marshal has its own interface stub.  this might
-     * not be correct. but, it means we don't need to refcount anything
-     * here. */
-    stub_manager_delete_ifstub(stubmgr, &mid.ipid);
-    
-    stub_manager_unref(stubmgr, 1);
+    stub_manager_ext_release(stubmgr, 1);
+
+    stub_manager_int_release(stubmgr);
 
     return S_OK;
 }

dlls/ole32/rpc.c

@@ -791,9 +791,10 @@ COM_RpcReceive(wine_pipe *xpipe) {
             goto end;
         }
 
-        /* this should destroy the stub manager in the case of only one connection to it */
-        stub_manager_unref(stubmgr, 1);
-        
+        stub_manager_ext_release(stubmgr, 1);
+
+        stub_manager_int_release(stubmgr);
+
         goto end;
     } else if (reqtype == REQTYPE_REQUEST) {
 	wine_rpc_request	*xreq;

dlls/ole32/stubmanager.c

@@ -6,6 +6,7 @@
  *
  * Copyright 2002 Marcus Meissner
  * Copyright 2004 Mike Hearn for CodeWeavers
+ * Copyright 2004 Robert Shearman (for CodeWeavers)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -41,6 +42,12 @@
 
 WINE_DEFAULT_DEBUG_CHANNEL(ole);
 
+static void stub_manager_delete_ifstub(struct stub_manager *m, struct ifstub *ifstub);
+static struct ifstub *stub_manager_ipid_to_ifstub(struct stub_manager *m, const IPID *ipid);
+
+/* creates a new stub manager and adds it into the apartment. caller must
+ * release stub manager when it is no longer required. the apartment and
+ * external refs together take one implicit ref */
 struct stub_manager *new_stub_manager(APARTMENT *apt, IUnknown *object)
 {
     struct stub_manager *sm;
@@ -55,19 +62,21 @@ struct stub_manager *new_stub_manager(APARTMENT *apt, IUnknown *object)
     IUnknown_AddRef(object);
     sm->object = object;
     sm->apt    = apt;
-    EnterCriticalSection(&apt->cs);
-    sm->oid    = apt->oidc++;
-    LeaveCriticalSection(&apt->cs);
-    
+
+    /* start off with 2 references because the stub is in the apartment
+     * and the caller will also hold a reference */
+    sm->refs   = 2;
+
     /* yes, that's right, this starts at zero. that's zero EXTERNAL
      * refs, ie nobody has unmarshalled anything yet. we can't have
      * negative refs because the stub manager cannot be explicitly
      * killed, it has to die by somebody unmarshalling then releasing
      * the marshalled ifptr.
      */
-    sm->refcount = 0;
-
+    sm->extrefs = 0;
+    
     EnterCriticalSection(&apt->cs);
+    sm->oid    = apt->oidc++;
     list_add_head(&apt->stubmgrs, &sm->entry);
     LeaveCriticalSection(&apt->cs);
 
@@ -76,6 +85,30 @@ struct stub_manager *new_stub_manager(APARTMENT *apt, IUnknown *object)
     return sm;
 }
 
+/* m->apt->cs must be held on entry to this function */
+static void stub_manager_delete(struct stub_manager *m)
+{
+    struct list *cursor;
+
+    TRACE("destroying %p (oid=%s)\n", m, wine_dbgstr_longlong(m->oid));
+
+    list_remove(&m->entry);
+
+    while ((cursor = list_head(&m->ifstubs)))
+    {
+        struct ifstub *ifstub = LIST_ENTRY(cursor, struct ifstub, entry);
+        stub_manager_delete_ifstub(m, ifstub);
+    }
+
+    IUnknown_Release(m->object);
+
+    DeleteCriticalSection(&m->lock);
+
+    HeapFree(GetProcessHeap(), 0, m);
+}
+
+/* gets the stub manager associated with an object - caller must call
+ * release on the stub manager when it is no longer needed */
 struct stub_manager *get_stub_manager_from_object(OXID oxid, void *object)
 {
     struct stub_manager *result = NULL;
@@ -96,6 +129,7 @@ struct stub_manager *get_stub_manager_from_object(OXID oxid, void *object)
         if (m->object == object)
         {
             result = m;
+            stub_manager_int_addref(result);
             break;
         }
     }
@@ -108,6 +142,40 @@ struct stub_manager *get_stub_manager_from_object(OXID oxid, void *object)
     return result;    
 }
 
+/* increments the internal refcount */
+ULONG stub_manager_int_addref(struct stub_manager *This)
+{
+    ULONG refs;
+
+    EnterCriticalSection(&This->apt->cs);
+    refs = ++This->refs;
+    LeaveCriticalSection(&This->apt->cs);
+
+    TRACE("before %ld\n", refs - 1);
+
+    return refs;
+}
+
+/* decrements the internal refcount */
+ULONG stub_manager_int_release(struct stub_manager *This)
+{
+    ULONG refs;
+    APARTMENT *apt = This->apt;
+
+    EnterCriticalSection(&apt->cs);
+    refs = --This->refs;
+
+    TRACE("after %ld\n", refs);
+
+    if (!refs)
+        stub_manager_delete(This);
+    LeaveCriticalSection(&apt->cs);
+
+    return refs;
+}
+
+/* gets the stub manager associated with an object id - caller must call
+ * release on the stub manager when it is no longer needed */
 struct stub_manager *get_stub_manager(OXID oxid, OID oid)
 {
     struct stub_manager *result = NULL;
@@ -128,6 +196,7 @@ struct stub_manager *get_stub_manager(OXID oxid, OID oid)
         if (m->oid == oid)
         {
             result = m;
+            stub_manager_int_addref(result);
             break;
         }
     }
@@ -140,59 +209,30 @@ struct stub_manager *get_stub_manager(OXID oxid, OID oid)
     return result;
 }
 
-/* add some external references (ie from a client that demarshalled an ifptr) */
-int stub_manager_ref(struct stub_manager *m, int refs)
+/* add some external references (ie from a client that unmarshaled an ifptr) */
+ULONG stub_manager_ext_addref(struct stub_manager *m, ULONG refs)
 {
-    int rc = InterlockedExchangeAdd(&m->refcount, refs) + refs;
-    TRACE("added %d refs to %p (oid %s), rc is now %d\n", refs, m, wine_dbgstr_longlong(m->oid), rc);
+    ULONG rc = InterlockedExchangeAdd(&m->extrefs, refs) + refs;
+
+    TRACE("added %lu refs to %p (oid %s), rc is now %lu\n", refs, m, wine_dbgstr_longlong(m->oid), rc);
+
     return rc;
 }
 
 /* remove some external references */
-int stub_manager_unref(struct stub_manager *m, int refs)
+ULONG stub_manager_ext_release(struct stub_manager *m, ULONG refs)
 {
-    int rc = InterlockedExchangeAdd(&m->refcount, -refs) - refs;
-    
-    TRACE("removed %d refs from %p (oid %s), rc is now %d\n", refs, m, wine_dbgstr_longlong(m->oid), rc);
+    ULONG rc = InterlockedExchangeAdd(&m->extrefs, -refs) - refs;
     
-    if (rc == 0)
-    {
-        TRACE("destroying %p (oid=%s)\n", m, wine_dbgstr_longlong(m->oid));
-
-        EnterCriticalSection(&m->apt->cs);
-        list_remove(&m->entry);
-        LeaveCriticalSection(&m->apt->cs);
-
-        /* table strong and normal marshals have a ref on us, so we
-         * can't die while they are outstanding unless the app does
-         * something weird like explicitly killing us (how?)
-         */
-
-        EnterCriticalSection(&m->lock);
-        if (!list_empty(&m->ifstubs))
-        {
-            ERR("PANIC: Stub manager %s is being destroyed with outstanding interface stubs\n", wine_dbgstr_longlong(m->oid));
-            
-            /* assert( FALSE ); */
-            /* fixme: this will happen sometimes until we do proper lifecycle management via IRemUnknown */
-        }
-
-        /* fixme: the lifecycle of table-weak marshals is not
-         * currently understood. results of testing against dcom98
-         * appear to contradict Essential COM   -m
-         */
-        
-        LeaveCriticalSection(&m->lock);        
-
-        IUnknown_Release(m->object);
+    TRACE("removed %lu refs from %p (oid %s), rc is now %lu\n", refs, m, wine_dbgstr_longlong(m->oid), rc);
 
-        HeapFree(GetProcessHeap(), 0, m);
-    }
+    if (rc == 0)
+        stub_manager_int_release(m);
 
-    return refs;
+    return rc;
 }
 
-static struct ifstub *stub_manager_ipid_to_ifstub(struct stub_manager *m, IPID *ipid)
+static struct ifstub *stub_manager_ipid_to_ifstub(struct stub_manager *m, const IPID *ipid)
 {
     struct list    *cursor;
     struct ifstub  *result = NULL;
@@ -213,7 +253,7 @@ static struct ifstub *stub_manager_ipid_to_ifstub(struct stub_manager *m, IPID *
     return result;
 }
 
-IRpcStubBuffer *stub_manager_ipid_to_stubbuffer(struct stub_manager *m, IPID *ipid)
+IRpcStubBuffer *stub_manager_ipid_to_stubbuffer(struct stub_manager *m, const IPID *ipid)
 {
     struct ifstub *ifstub = stub_manager_ipid_to_ifstub(m, ipid);
     
@@ -239,7 +279,7 @@ struct ifstub *stub_manager_new_ifstub(struct stub_manager *m, IRpcStubBuffer *s
     stub->table = tablemarshal;
     stub->iid = *iid;
     stub->ipid = *iid; /* FIXME: should be globally unique */
-    
+
     EnterCriticalSection(&m->lock);
     list_add_head(&m->ifstubs, &stub->entry);
     LeaveCriticalSection(&m->lock);
@@ -247,28 +287,14 @@ struct ifstub *stub_manager_new_ifstub(struct stub_manager *m, IRpcStubBuffer *s
     return stub;
 }
 
-/* fixme: should ifstubs be refcounted? iid should be ipid */
-void stub_manager_delete_ifstub(struct stub_manager *m, IPID *ipid)
+static void stub_manager_delete_ifstub(struct stub_manager *m, struct ifstub *ifstub)
 {
-    struct ifstub *ifstub;
-
-    TRACE("m=%p, m->oid=%s, ipid=%s\n", m, wine_dbgstr_longlong(m->oid), debugstr_guid(ipid));
-    
-    EnterCriticalSection(&m->lock);
+    TRACE("m=%p, m->oid=%s, ipid=%s\n", m, wine_dbgstr_longlong(m->oid), debugstr_guid(&ifstub->ipid));
     
-    if ((ifstub = stub_manager_ipid_to_ifstub(m, ipid)))
-    {
-        list_remove(&ifstub->entry);
-        
-        IUnknown_Release(ifstub->stubbuffer);
-        IUnknown_Release(ifstub->iface);
+    list_remove(&ifstub->entry);
         
-        HeapFree(GetProcessHeap(), 0, ifstub);
-    }
-    else
-    {
-        WARN("could not map ipid %s to ifstub\n", debugstr_guid(ipid));
-    }
-    
-    LeaveCriticalSection(&m->lock);
+    IUnknown_Release(ifstub->stubbuffer);
+    IUnknown_Release(ifstub->iface);
+
+    HeapFree(GetProcessHeap(), 0, ifstub);
 }