Re: [OLE #21] Make apartment access thread safe, rework OLE TLS management

[Robert Shearman]

Mike Hearn wrote:
> On Mon, 2005-01-03 at 11:47 -0600, Robert Shearman wrote:
> > > > Also, it may be best to split up the apartment structure into the stuff 
> > > > that really is apartment-local and the stuff that is thread-local, and 
> > > > then we have NtCurrentTeb()->ReservedForOle point to the thread-local 
> > > > structure, which has a member that points to the apartment struct.
> > >
> > > That makes sense. I'll implement it now.
> >
> > One other thing: we also need the MTA to be dynamically allocated and 
> > destroyed and so therefore also ref counted.
> >
> > Rob
>
> OK, I implemented the new OLE TLS management, and it clears up the code
> a lot, no more confusing apt->parent business.
>
> I didn't make the MTA dynamically allocated because this patch already
> contains two changes that could really have been one given enough pain,
> so I added it to the todo list instead.
>
> I think this patch is pretty much ready to go, the OLE TLS changes are
> straightforward enough, so I'm CCing wine-patches. Any comments on the
> last changes are welcome as usual.
>
> Alexandre: this patch is against the previous patches sent, so they must
> be applied in order.
>
> Mike Hearn <mh(a)codeweavers.com>
> - Make apartment access thread-safe by introducing refcounting and wider
>  usage of the apartment lock
> - Rework OLE TLS management to eliminate uninitialised apartments and
>  parent chaining
>
> ------------------------------------------------------------------------
>
> --- dlls/ole32/compobj.c  (revision 67)
> +++ dlls/ole32/compobj.c  (local)
> @@ -30,6 +30,8 @@
>  *   - Rewrite the CoLockObjectExternal code, it does totally the wrong
>  *     thing currently (should be controlling the stub manager)
>  *
> + *   - Make the MTA dynamically allocated and refcounted
> + *
>  *   - Implement the service control manager (in rpcss) to keep track
>  *     of registered class objects: ISCM::ServerRegisterClsid et al
>  *   - Implement the OXID resolver so we don't need magic pipe names for
> @@ -99,16 +101,17 @@ static void COM_ExternalLockFreeList(voi
>
> const CLSID CLSID_StdGlobalInterfaceTable = { 0x00000323, 0, 0, {0xc0, 0, 0, 0, 0, 0, 0, 0x46} };
>
> -APARTMENT MTA, *apts;
> +APARTMENT MTA;
> +struct list apts = LIST_INIT( apts );
>
> -static CRITICAL_SECTION csApartment;
> +CRITICAL_SECTION csApartment;
> static CRITICAL_SECTION_DEBUG critsect_debug =
> {
>     0, 0, &csApartment,
>     { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList },
>       0, 0, { 0, (DWORD)(__FILE__ ": csApartment") }
> };
> -static CRITICAL_SECTION csApartment = { &critsect_debug, -1, 0, 0, 0, 0 };
> +CRITICAL_SECTION csApartment = { &critsect_debug, -1, 0, 0, 0, 0 };

I can't see the need to make this non-static at the moment.
> /*
>  * This lock count counts the number of times CoInitialize is called. It is
> @@ -236,6 +239,7 @@ static void COM_UninitMTA(void)
>     MTA.oxid = 0;
> }
>
> +
> /* creates an apartment structure which stores OLE thread-local
>  * information. Call with COINIT_UNINITIALIZED to create an apartment
>  * that will be initialized with a model later. Note: do not call
> @@ -243,100 +247,138 @@ static void COM_UninitMTA(void)
>  * with a different COINIT value */
> APARTMENT* COM_CreateApartment(DWORD model)
> {
> -    APARTMENT *apt;
> -    BOOL create = (NtCurrentTeb()->ReservedForOle == NULL);
> +    APARTMENT *apt = COM_CurrentApt();
>
> -    if (create)
> +    if (!apt)
>     {
> +        if (model & COINIT_MULTITHREADED)
> +        {
> +            TRACE("thread 0x%lx is entering the multithreaded apartment\n", GetCurrentThreadId());
> +            COM_CurrentInfo()->apt = &MTA;
> +            return apt;
> +        }
> +
> +        TRACE("creating new apartment, model=%ld\n", model);
> +
>         apt = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(APARTMENT));
>         apt->tid = GetCurrentThreadId();
>         DuplicateHandle(GetCurrentProcess(), GetCurrentThread(),
>                         GetCurrentProcess(), &apt->thread,
>                         THREAD_ALL_ACCESS, FALSE, 0);
> -    }
> -    else
> -        apt = NtCurrentTeb()->ReservedForOle;
>
> -    list_init(&apt->proxies);
> -    list_init(&apt->stubmgrs);
> -    apt->oidc = 1;
> -    
> -    apt->model = model;
> -    if (model & COINIT_APARTMENTTHREADED) {
> -      /* FIXME: how does windoze create OXIDs? */
> -      apt->oxid = MTA.oxid | GetCurrentThreadId();
> -      apt->win = CreateWindowA(aptWinClass, NULL, 0,
> -			       0, 0, 0, 0,
> -			       0, 0, OLE32_hInstance, NULL);
> -      InitializeCriticalSection(&apt->cs);
> -    }
> -    else if (!(model & COINIT_UNINITIALIZED)) {
> -      apt->parent = &MTA;
> -      apt->oxid = MTA.oxid;
> -    }
> -    EnterCriticalSection(&csApartment);
> -    if (create)
> -    {
> -        if (apts) apts->prev = apt;
> -        apt->next = apts;
> -        apts = apt;
> +        list_init(&apt->stubmgrs);
> +        apt->oidc = 1;
> +        InitializeCriticalSection(&apt->cs);
> +
> +        apt->model = model;
> +
> +        /* we don't ref the apartment as CoInitializeEx will do it for us */
> +
> +        if (model & COINIT_APARTMENTTHREADED)
> +        {
> +            /* FIXME: how does windoze create OXIDs? */
> +            apt->oxid = MTA.oxid | GetCurrentThreadId();
> +            apt->win = CreateWindowA(aptWinClass, NULL, 0,
> +                                     0, 0, 0, 0,
> +                                     0, 0, OLE32_hInstance, NULL);
> +        }
> +
> +        EnterCriticalSection(&csApartment);
> +        list_add_head(&apts, &apt->entry);
> +        LeaveCriticalSection(&csApartment);
> +
> +        COM_CurrentInfo()->apt = apt;
>     }
> -    LeaveCriticalSection(&csApartment);
> -    NtCurrentTeb()->ReservedForOle = apt;
> +
>     return apt;
> }
>
> -static void COM_DestroyApartment(APARTMENT *apt)
> +DWORD COM_ApartmentAddRef(APARTMENT *apt)
> +{
> +    return InterlockedIncrement(&apt->refs);
> +}
> +
> +/* if null, use current apt and on destroy clear the TEB apt field */
> +DWORD COM_ApartmentRelease(APARTMENT *apartment)

I don't really like functions like this doing special things on certain 
input values. I don't think it is that big a deal to require the few 
callers of this function to go to the effort of getting the apartment 
themselves.
> {
> +    APARTMENT *apt = apartment ? apartment : COM_CurrentInfo()->apt;
> +
>     EnterCriticalSection(&csApartment);
> -    if (apt->prev) apt->prev->next = apt->next;
> -    if (apt->next) apt->next->prev = apt->prev;
> -    if (apts == apt) apts = apt->next;
> -    apt->prev = NULL; apt->next = NULL;
> -    LeaveCriticalSection(&csApartment);
> -    if (apt->model & COINIT_APARTMENTTHREADED) {
> -      /* disconnect proxies to release the corresponding stubs.
> -       * It is confirmed in "Essential COM" in the sub-chapter on
> -       * "Lifecycle Management and Marshalling" that the native version also
> -       * disconnects proxies in this function. */
> -      /* FIXME: this should also be called for MTA destruction, but that
> -       * requires restructuring how apartments work slightly. */
> -      MARSHAL_Disconnect_Proxies(apt);
>
> -      if (apt->win) DestroyWindow(apt->win);
> -      DeleteCriticalSection(&apt->cs);
> +    if (InterlockedDecrement(&apt->refs) == 0)
> +    {
> +        TRACE("destroying apartment %p, oxid %s\n", apt, wine_dbgstr_longlong(apt->oxid));
> +
> +        if (!apartment) COM_CurrentInfo()->apt = NULL;
> +
> +        MARSHAL_Disconnect_Proxies(apt);
> +
> +        list_remove(&apt->entry);
> +        if ((apt->model & COINIT_APARTMENTTHREADED) && apt->win) DestroyWindow(apt->win);
> +
> +        if (!list_empty(&apt->stubmgrs))
> +        {
> +            FIXME("PANIC: Apartment being destroyed with outstanding stubs, what do we do now?\n");

To answer your question: forcefully destroy them all.
> +        }
> +
> +        if (apt->filter) IUnknown_Release(apt->filter);
> +
> +
> +        DeleteCriticalSection(&apt->cs);
> +        CloseHandle(apt->thread);
> +        HeapFree(GetProcessHeap(), 0, apt);
> +
> +        apt = NULL;
>     }
> -    CloseHandle(apt->thread);
> -    HeapFree(GetProcessHeap(), 0, apt);
> -}
>
> -/* The given OXID must be local to this process: you cannot use apartment
> -   windows to send RPCs to other processes. This all needs to move to rpcrt4 */
> +    LeaveCriticalSection(&csApartment);
>
> -APARTMENT *COM_ApartmentFromOXID(OXID oxid)
> +    return apt ? apt->refs : 0;
> +}
> +
> +/* The given OXID must be local to this process: you cannot use
> + * apartment windows to send RPCs to other processes. This all needs
> + * to move to rpcrt4.
> + *
> + * The ref parameter is here mostly to ensure people remember that
> + * they get one, you should normally take a ref for thread safety.
> + */
> +APARTMENT *COM_ApartmentFromOXID(OXID oxid, BOOL ref)
> {
> -    APARTMENT *apt = NULL;
> +    APARTMENT *result = NULL;
> +    struct list *cursor;
>
> EnterCriticalSection(&csApartment);
> -    apt = apts;
> -    while (apt && apt->oxid != oxid) apt = apt->next;
> +    LIST_FOR_EACH( cursor, &apts )
> +    {
> +        struct apartment *apt = LIST_ENTRY( cursor, struct apartment, entry );
> +        if (apt->oxid == oxid)
> +        {
> +            result = apt;
> +            if (ref) COM_ApartmentAddRef(result);
> +            break;
> +        }
> +    }
>     LeaveCriticalSection(&csApartment);
>
> -    return apt;
> +    return result;
> }
>
> -HWND COM_GetApartmentWin(OXID oxid)
> +HWND COM_GetApartmentWin(OXID oxid, BOOL ref)
> {
> -    APARTMENT *apt = COM_ApartmentFromOXID(oxid);
> +    APARTMENT *apt;
>
> -    return apt ? apt->win : NULL;
> +    apt = COM_ApartmentFromOXID(oxid, ref);
> +    if (!apt) return NULL;
> +
> +    return apt->win;
> }
>
> /* Currently inter-thread marshalling is not fully implemented, so this does nothing */
> static LRESULT CALLBACK COM_AptWndProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
> {
>   return DefWindowProcA(hWnd, msg, wParam, lParam);
> -} 
> +}
>
> /*****************************************************************************
>  * This section contains OpenDllList implemantation
> @@ -467,8 +509,7 @@ HRESULT WINAPI CoInitialize(
>  *   CoUninitialize
>  */
> HRESULT WINAPI CoInitializeEx(
> -	LPVOID lpReserved,	/* [in] pointer to win32 malloc interface
> -                                   (obsolete, should be NULL) */
> +	LPVOID lpReserved,	/* [in] pointer to win32 malloc interface (obsolete, should be NULL) */
>   DWORD dwCoInit		/* [in] A value from COINIT specifies the threading model */
> )
> {
> @@ -482,21 +523,6 @@ HRESULT WINAPI CoInitializeEx(
>     ERR("(%p, %x) - Bad parameter passed-in %p, must be an old Windows Application\n", lpReserved, (int)dwCoInit, lpReserved);
>   }
>
> -  apt = NtCurrentTeb()->ReservedForOle;
> -  if (apt && !(apt->model == COINIT_UNINITIALIZED))
> -  {
> -    if (dwCoInit != apt->model)
> -    {
> -      /* Changing the threading model after it's been set is illegal. If this warning is triggered by Wine
> -         code then we are probably using the wrong threading model to implement that API. */
> -      ERR("Attempt to change threading model of this apartment from 0x%lx to 0x%lx\n", apt->model, dwCoInit);
> -      return RPC_E_CHANGED_MODE;
> -    }
> -    hr = S_FALSE;
> -  }
> -  else
> -    hr = S_OK;
> -
>   /*
>    * Check the lock count. If this is the first time going through the initialize
>    * process, we have to initialize the libraries.
> @@ -512,13 +538,22 @@ HRESULT WINAPI CoInitializeEx(
>
> COM_InitMTA();
>
> +    /* we may need to defer this until after apartment initialisation */
>     RunningObjectTableImpl_Initialize();
>   }
>
> -  if (!apt || apt->model == COINIT_UNINITIALIZED) apt = COM_CreateApartment(dwCoInit);
> +  /* doing this twice is harmless */
> +  apt = COM_CreateApartment(dwCoInit);
>
> -  InterlockedIncrement(&apt->inits);
> -  if (hr == S_OK) NtCurrentTeb()->ReservedForOle = apt;
> +  if (dwCoInit != apt->model)
> +  {
> +    /* Changing the threading model after it's been set is illegal. If this warning is triggered by Wine
> +       code then we are probably using the wrong threading model to implement that API. */
> +    ERR("Attempt to change threading model of this apartment from 0x%lx to 0x%lx\n", apt->model, dwCoInit);
> +    return RPC_E_CHANGED_MODE;
> +  }
> +
> +  InterlockedIncrement(&apt->refs);

I think it would be cleaner to put the ref-count increment inside the 
apartment creation function. That way, no callers of the 
COM_CreateApartment function can make the mistake of creating an 
apartment without incrementing its refcount.
> return hr;
> }
> @@ -529,14 +564,20 @@ HRESULT WINAPI CoInitializeEx(
> void COM_FlushMessageQueue(void)
> {
>     MSG message;
> -    APARTMENT *apt = NtCurrentTeb()->ReservedForOle;
> +    APARTMENT *apt = COM_CurrentApt();
>
> if (!apt || !apt->win) return;
>
> TRACE("Flushing STA message queue\n");
>
> -    while (PeekMessageA(&message, NULL, 0, 0, PM_REMOVE)) {
> -        if (message.hwnd != apt->win) continue;
> +    while (PeekMessageA(&message, NULL, 0, 0, PM_REMOVE))
> +    {
> +        if (message.hwnd != apt->win)
> +        {
> +            WARN("discarding message 0x%x for window %p\n", message.message, message.hwnd);
> +            continue;
> +        }
> +
>         TranslateMessage(&message);
>         DispatchMessageA(&message);
>     }
> @@ -561,17 +602,10 @@ void COM_FlushMessageQueue(void)
> void WINAPI CoUninitialize(void)
> {
>   LONG lCOMRefCnt;
> -  APARTMENT *apt;
>
> TRACE("()\n");
>
> -  apt = NtCurrentTeb()->ReservedForOle;
> -  if (!apt) return;
> -  if (InterlockedDecrement(&apt->inits)==0) {
> -    NtCurrentTeb()->ReservedForOle = NULL;
> -    COM_DestroyApartment(apt);
> -    apt = NULL;
> -  }
> +  COM_ApartmentRelease(NULL);
>
> /*
>    * Decrease the reference count.
> @@ -1134,14 +1168,14 @@ end:
>
> /******************************************************************************
>  *		CoRegisterClassObject	[OLE32.@]
> - * 
> + *
>  * Registers the class object for a given class ID. Servers housed in EXE
>  * files use this method instead of exporting DllGetClassObject to allow
>  * other code to connect to their objects.
>  *
>  * RETURNS
> - *   S_OK on success, 
> - *   E_INVALIDARG if lpdwRegister or pUnk are NULL, 
> + *   S_OK on success,
> + *   E_INVALIDARG if lpdwRegister or pUnk are NULL,
>  *   CO_E_OBJISREG if the object is already registered. We should not return this.
>  *
>  * SEE ALSO
> @@ -1217,7 +1251,7 @@ HRESULT WINAPI CoRegisterClassObject(
>   if (dwClsContext & CLSCTX_LOCAL_SERVER) {
>       IClassFactory *classfac;
>
> -      hr = IUnknown_QueryInterface(newClass->classObject, &IID_IClassFactory, 
> +      hr = IUnknown_QueryInterface(newClass->classObject, &IID_IClassFactory,
>                                    (LPVOID*)&classfac);
>       if (hr) return hr;
>
> @@ -1556,7 +1590,7 @@ HRESULT WINAPI CoCreateInstance(
>   LPCLASSFACTORY lpclf = 0;
>
> if (!COM_CurrentApt()) return CO_E_NOTINITIALIZED;
> -        
> +
>   /*
>    * Sanity check
>    */
> @@ -1573,15 +1607,15 @@ HRESULT WINAPI CoCreateInstance(
>    * Rather than create a class factory, we can just check for it here
>    */
>   if (IsEqualIID(rclsid, &CLSID_StdGlobalInterfaceTable)) {
> -    if (StdGlobalInterfaceTableInstance == NULL) 
> +    if (StdGlobalInterfaceTableInstance == NULL)
>       StdGlobalInterfaceTableInstance = StdGlobalInterfaceTable_Construct();
>     hres = IGlobalInterfaceTable_QueryInterface( (IGlobalInterfaceTable*) StdGlobalInterfaceTableInstance, iid, ppv);
>     if (hres) return hres;
> -    
> +
>     TRACE("Retrieved GIT (%p)\n", *ppv);
>     return S_OK;
>   }
> -  
> +
>   /*
>    * Get a class factory to construct the object we want.
>    */
> @@ -2000,19 +2034,21 @@ HRESULT WINAPI CoInitializeWOW(DWORD x,D
>  */
> HRESULT WINAPI CoGetState(IUnknown ** ppv)
> {
> -	APARTMENT * apt = COM_CurrentInfo();
> +    HRESULT    hr  = E_FAIL;
>
> -	FIXME("\n");
> +    FIXME("possible stub\n");

I believe the function does everything that is required of it - the only 
problem is that we don't currently do anything with the state. I'm not 
sure if we should though.
> -	if(apt && apt->state) {
> -	    IUnknown_AddRef(apt->state);
> -	    *ppv = apt->state;
> -	    FIXME("-- %p\n", *ppv);
> -	    return S_OK;
> -	}
> -	*ppv = NULL;
> -	return E_FAIL;
> +    *ppv = NULL;
> +
> +    if (COM_CurrentInfo()->state)
> +    {
> +        IUnknown_AddRef(COM_CurrentInfo()->state);
> +        *ppv = COM_CurrentInfo()->state;
> +        TRACE("apt->state=%p\n", COM_CurrentInfo()->state);
> +        hr = S_OK;
> +    }
>
> +    return hr;
> }
>
> /***********************************************************************
> @@ -2021,22 +2057,19 @@ HRESULT WINAPI CoGetState(IUnknown ** pp
>  */
> HRESULT WINAPI CoSetState(IUnknown * pv)
> {
> -    APARTMENT * apt = COM_CurrentInfo();
> +    FIXME("(%p),stub!\n", pv);
>
> -    if (!apt) apt = COM_CreateApartment(COINIT_UNINITIALIZED);
> +    if (pv) IUnknown_AddRef(pv);
>
> -	FIXME("(%p),stub!\n", pv);
> +    if (COM_CurrentInfo()->state)
> +    {
> +        TRACE("-- release %p now\n", COM_CurrentInfo()->state);
> +        IUnknown_Release(COM_CurrentInfo()->state);
> +    }
>
> -	if (pv) {
> -	    IUnknown_AddRef(pv);
> -	}
> +    COM_CurrentInfo()->state = pv;
>
> -	if (apt->state) {
> -	    TRACE("-- release %p now\n", apt->state);
> -	    IUnknown_Release(apt->state);
> -	}
> -	apt->state = pv;
> -	return S_OK;
> +    return S_OK;
> }
>
> @@ -2191,7 +2224,7 @@ HRESULT WINAPI CoGetTreatAsClass(REFCLSI
> done:
>     if (hkey) RegCloseKey(hkey);
>     return res;
> -    
> +
> }
>
> /***********************************************************************
> --- dlls/ole32/compobj_private.h  (revision 67)
> +++ dlls/ole32/compobj_private.h  (local)
> @@ -38,12 +38,8 @@
> #include "winreg.h"
> #include "winternl.h"
>
> -/* Windows maps COINIT values to 0x80 for apartment threaded, 0x140
> - * for free threaded, and 0 for uninitialized apartments. There is
> - * no real advantage in us doing this and certainly no release version
> - * of an app should be poking around with these flags. So we need a
> - * special value for uninitialized */
> -#define COINIT_UNINITIALIZED 0x100
> +struct apartment;
> +
>
> /* exported interface */
> typedef struct tagXIF {
> @@ -59,7 +55,7 @@ typedef struct tagXIF {
> /* exported object */
> typedef struct tagXOBJECT {
>   IRpcStubBufferVtbl *lpVtbl;
> -  struct tagAPARTMENT *parent;
> +  struct apartment *parent;
>   struct tagXOBJECT *next;
>   LPUNKNOWN obj;           /* object identity (IUnknown) */
>   OID oid;                 /* object ID */
> @@ -83,7 +79,7 @@ struct ifproxy
> struct proxy_manager
> {
>   const IInternalUnknownVtbl *lpVtbl;
> -  struct tagAPARTMENT *parent;
> +  struct apartment *parent;
>   struct list entry;
>   LPRPCCHANNELBUFFER chan; /* channel to object */
>   OXID oxid;               /* object exported ID */
> @@ -93,11 +89,13 @@ struct proxy_manager
>   CRITICAL_SECTION cs;     /* thread safety for this object and children */
> };
>
> -/* apartment */
> -typedef struct tagAPARTMENT {
> -  struct tagAPARTMENT *next, *prev, *parent;
> +/* this needs to become a COM object that implements IRemUnknown */
> +struct apartment
> +{
> +  struct list entry;       
> +
> +  DWORD refs;              /* refcount of the apartment */
>   DWORD model;             /* threading model */
> -  DWORD inits;             /* CoInitialize count */

This should be moved into the TLS struct, not removed entirely.
> DWORD tid;               /* thread id */
>   HANDLE thread;           /* thread handle */
>   OXID oxid;               /* object exporter ID */
> @@ -107,13 +105,12 @@ typedef struct tagAPARTMENT {
>   LPMESSAGEFILTER filter;  /* message filter */
>   XOBJECT *objs;           /* exported objects */
>   struct list proxies;     /* imported objects */
> -  LPUNKNOWN state;         /* state object (see Co[Get,Set]State) */
>   LPVOID ErrorInfo;        /* thread error info */

You can remove this - it should no longer be needed.
> DWORD listenertid;       /* id of apartment_listener_thread */
>   struct list stubmgrs;    /* stub managers for exported objects */
> -} APARTMENT;
> +};
>
> -extern APARTMENT MTA, *apts;
> +typedef struct apartment APARTMENT;
>
> extern void* StdGlobalInterfaceTable_Construct(void);
> extern void  StdGlobalInterfaceTable_Destroy(void* self);
> @@ -196,27 +193,41 @@ int WINAPI FileMonikerImpl_DecomposePath
>
> HRESULT WINAPI __CLSIDFromStringA(LPCSTR idstr, CLSID *id);
>
> +/* compobj.c */
> +APARTMENT *COM_CreateApartment(DWORD model);
> +APARTMENT *COM_ApartmentFromOXID(OXID oxid, BOOL ref);
> +DWORD COM_ApartmentAddRef(APARTMENT *apt);
> +DWORD COM_ApartmentRelease(APARTMENT *apt);
> +
> +extern CRITICAL_SECTION csApartment;
> +
> +/* this is what is stored in TEB->ReservedForOle */
> +struct oletls
> +{
> +    struct apartment *apt;
> +    IErrorInfo       *errorinfo;   /* see errorinfo.c */
> +    IUnknown         *state;       /* see CoSetState */
> +};
> +
> /*
>  * Per-thread values are stored in the TEB on offset 0xF80,
>  * see http://www.microsoft.com/msj/1099/bugslayer/bugslayer1099.htm
>  */
> -static inline APARTMENT* COM_CurrentInfo(void)
> +
> +/* will create if necessary */
> +static inline struct oletls *COM_CurrentInfo(void)
> {
> -  APARTMENT* apt = NtCurrentTeb()->ReservedForOle;
> -  return apt;
> +    if (!NtCurrentTeb()->ReservedForOle)     
> +        NtCurrentTeb()->ReservedForOle = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct oletls));

I guess are ignoring failures from HeapAlloc then. There isn't really 
much we can do about it anyway though.
> +    
> +    return NtCurrentTeb()->ReservedForOle;
> }
> +
> static inline APARTMENT* COM_CurrentApt(void)
> -{
> -  APARTMENT* apt = COM_CurrentInfo();
> -  if (apt && apt->parent) apt = apt->parent;
> -  return apt;
> +{  
> +    return COM_CurrentInfo()->apt;
> }
>
> -/* compobj.c */
> -APARTMENT *COM_CreateApartment(DWORD model);
> -HWND COM_GetApartmentWin(OXID oxid);
> -APARTMENT *COM_ApartmentFromOXID(OXID oxid);
> -
> #define ICOM_THIS_MULTI(impl,field,iface) impl* const This=(impl*)((char*)(iface) - offsetof(impl,field))
>
> #endif /* __WINE_OLE_COMPOBJ_H */
> --- dlls/ole32/errorinfo.c  (revision 67)
> +++ dlls/ole32/errorinfo.c  (local)
> @@ -20,7 +20,8 @@
>  * NOTES:
>  *
>  * The errorinfo is a per-thread object. The reference is stored in the
> - * TEB at offset 0xf80
> + * TEB at offset 0xf80. Does anything actually access this directly?
> + * If not we should make this use a different field and drop the apt->parent hack.
>  */

Nothing should reference it directly as the offset is OS dependent.
> #include <stdarg.h>
> @@ -483,20 +484,20 @@ HRESULT WINAPI CreateErrorInfo(ICreateEr
>  */
> HRESULT WINAPI GetErrorInfo(ULONG dwReserved, IErrorInfo **pperrinfo)
> {
> -	APARTMENT * apt = COM_CurrentInfo();
> -
> -	TRACE("(%ld, %p, %p)\n", dwReserved, pperrinfo, COM_CurrentInfo()->ErrorInfo);
> +	TRACE("(%ld, %p, %p)\n", dwReserved, pperrinfo, COM_CurrentInfo()->errorinfo);
>
> if(!pperrinfo) return E_INVALIDARG;
> -	if (!apt || !apt->ErrorInfo)
> +        
> +	if (!COM_CurrentInfo()->errorinfo)
>   {
>      *pperrinfo = NULL;
>      return S_FALSE;
>   }
>
> -	*pperrinfo = (IErrorInfo*)(apt->ErrorInfo);
> +	*pperrinfo = COM_CurrentInfo()->errorinfo;
> +        
>   /* clear thread error state */
> -	apt->ErrorInfo = NULL;
> +	COM_CurrentInfo()->errorinfo = NULL;
>   return S_OK;
> }
>
> @@ -506,18 +507,16 @@ HRESULT WINAPI GetErrorInfo(ULONG dwRese
> HRESULT WINAPI SetErrorInfo(ULONG dwReserved, IErrorInfo *perrinfo)
> {
>   IErrorInfo * pei;
> -	APARTMENT * apt = COM_CurrentInfo();
>
> TRACE("(%ld, %p)\n", dwReserved, perrinfo);
>
> -	if (!apt) apt = COM_CreateApartment(COINIT_UNINITIALIZED);
> -
>   /* release old errorinfo */
> -	pei = (IErrorInfo*)apt->ErrorInfo;
> -	if(pei) IErrorInfo_Release(pei);
> +	pei = COM_CurrentInfo()->errorinfo;
> +	if (pei) IErrorInfo_Release(pei);
>
> /* set to new value */
> -	apt->ErrorInfo = perrinfo;
> -	if(perrinfo) IErrorInfo_AddRef(perrinfo);
> +	COM_CurrentInfo()->errorinfo = perrinfo;
> +	if (perrinfo) IErrorInfo_AddRef(perrinfo);
> +        
>   return S_OK;
> }
> --- dlls/ole32/marshal.c  (revision 67)
> +++ dlls/ole32/marshal.c  (local)
> @@ -109,9 +109,13 @@ static HRESULT register_ifstub(wine_mars
>     }
>     else
>     {
> -        TRACE("constructing new stub manager\n");
> +        struct apartment *apt;
>
> -        manager = new_stub_manager(COM_ApartmentFromOXID(mid->oxid), obj);
> +        TRACE("constructing new stub manager\n");
> +
> +        apt = COM_ApartmentFromOXID(mid->oxid, TRUE);
> +        manager = new_stub_manager(apt, obj);
> +        COM_ApartmentRelease(apt);
>         if (!manager) return E_OUTOFMEMORY;
>
> if (!tablemarshal) stub_manager_ref(manager, 1);
> --- dlls/ole32/rpc.c  (revision 67)
> +++ dlls/ole32/rpc.c  (local)
> @@ -308,8 +308,6 @@ PipeBuf_Release(LPRPCCHANNELBUFFER iface
>     if (ref)
>   return ref;
>
> -    FIXME("Free all stuff\n");
> -
>     memcpy(&header.mid, &This->mid, sizeof(wine_marshal_id));
>
> pipe = PIPE_FindByMID(&This->mid);
> @@ -892,7 +890,7 @@ static DWORD WINAPI apartment_listener_t
>     HANDLE		listenPipe;
>     APARTMENT          *apt = (APARTMENT *) param;
>
> -    /* we must join the marshalling threads apartment */
> +    /* we must join the marshalling threads apartment. we already have a ref here */
>     NtCurrentTeb()->ReservedForOle = apt;
>
> sprintf(pipefn,OLESTUBMGR"_%08lx%08lx", (DWORD)(apt->oxid >> 32), (DWORD)(apt->oxid));
> @@ -926,7 +924,7 @@ static DWORD WINAPI apartment_listener_t
> void start_apartment_listener_thread()
> {
>     APARTMENT *apt = COM_CurrentApt();
> -
> +    
>     assert( apt );
>
> TRACE("apt->listenertid=%ld\n", apt->listenertid);
> --- dlls/ole32/stubmanager.c  (revision 67)
> +++ dlls/ole32/stubmanager.c  (local)
> @@ -41,6 +41,7 @@
>
> WINE_DEFAULT_DEBUG_CHANNEL(ole);
>
> +/* this refs the apartment on success, otherwise there is no ref */
> struct stub_manager *new_stub_manager(APARTMENT *apt, IUnknown *object)
> {
>     struct stub_manager *sm;
> @@ -71,6 +72,7 @@ struct stub_manager *new_stub_manager(AP
>     list_add_head(&apt->stubmgrs, &sm->entry);
>     LeaveCriticalSection(&apt->cs);
>
> +    COM_ApartmentAddRef(apt);
>     TRACE("Created new stub manager (oid=%s) at %p for object with IUnknown %p\n", wine_dbgstr_longlong(sm->oid), sm, object);
>
> return sm;
> @@ -82,7 +84,7 @@ struct stub_manager *get_stub_manager_fr
>     struct list         *cursor;
>     APARTMENT           *apt;
>
> -    if (!(apt = COM_ApartmentFromOXID(oxid)))
> +    if (!(apt = COM_ApartmentFromOXID(oxid, TRUE)))
>     {
>         WARN("Could not map OXID %s to apartment object\n", wine_dbgstr_longlong(oxid));
>         return NULL;
> @@ -101,6 +103,8 @@ struct stub_manager *get_stub_manager_fr
>     }
>     LeaveCriticalSection(&apt->cs);
>
> +    COM_ApartmentRelease(apt);
> +    
>     TRACE("found %p from object %p\n", result, object);
>
> return result;    
> @@ -112,14 +116,13 @@ struct stub_manager *get_stub_manager(OX
>     struct list         *cursor;
>     APARTMENT           *apt;
>
> -    if (!(apt = COM_ApartmentFromOXID(oxid)))
> +    if (!(apt = COM_ApartmentFromOXID(oxid, TRUE)))
>     {
>         WARN("Could not map OXID %s to apartment object\n", wine_dbgstr_longlong(oxid));
>         return NULL;
>     }
>
> EnterCriticalSection(&apt->cs);
> -    
>     LIST_FOR_EACH( cursor, &apt->stubmgrs )
>     {
>         struct stub_manager *m = LIST_ENTRY( cursor, struct stub_manager, entry );
> @@ -130,9 +133,10 @@ struct stub_manager *get_stub_manager(OX
>             break;
>         }
>     }
> -    
>     LeaveCriticalSection(&apt->cs);
>
> +    COM_ApartmentRelease(apt);
> +
>     TRACE("found %p from oid %s\n", result, wine_dbgstr_longlong(oid));
>
> return result;

Rob

[OLE #21] Make apartment access thread safe, rework OLE TLS management (take 2)

[Mike Hearn]

On Tue, 2005-01-04 at 12:21 -0600, Robert Shearman wrote:
> I can't see the need to make this non-static at the moment.

Right, I made it non-static when the lock was taken inside an inlined
function earlier, but I got rid of that at the end. It can go back to
being static.
> I don't really like functions like this doing special things on certain 
> input values. I don't think it is that big a deal to require the few 
> callers of this function to go to the effort of getting the apartment 
> themselves.

It was done that way so it's inside the lock, ie the TEB apt field
should be cleared before the apartment is freed, otherwise there's a
time when it's pointing to garbage.

But thinking about it, as that's TLS it probably doesn't matter. So I'll
remove this feature.
> I think it would be cleaner to put the ref-count increment inside the 
> apartment creation function. That way, no callers of the 
> COM_CreateApartment function can make the mistake of creating an 
> apartment without incrementing its refcount.

OK, done.
> > HRESULT WINAPI CoGetState(IUnknown ** ppv)
> > {
> > -	APARTMENT * apt = COM_CurrentInfo();
> > +    HRESULT    hr  = E_FAIL;
> >
> > -	FIXME("\n");
> > +    FIXME("possible stub\n");
>
> I believe the function does everything that is required of it - the only 
> problem is that we don't currently do anything with the state. I'm not 
> sure if we should though.

Do we actually have any docs on this function? I agree the "possible
stub" fixme is annoying though so I'll remove it.
> This should be moved into the TLS struct, not removed entirely.

inits has been subsumed into the refcount, each CoInitialize adds a ref.
Why is it still needed?
> > LPVOID ErrorInfo;        /* thread error info */
>
> You can remove this - it should no longer be needed.

Right, done.
> > +    if (!NtCurrentTeb()->ReservedForOle)     
> > +        NtCurrentTeb()->ReservedForOle = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct oletls));
>
> I guess are ignoring failures from HeapAlloc then. There isn't really 
> much we can do about it anyway though.

Indeed, we're a bit hosed in this situation. I guess the relevant parts
of the code should return an appropriate error code for that specific
API though.
> Nothing should reference it directly as the offset is OS dependent.

Hehe, whatever :) I've seen too many "neat tricks" based on referencing
stuff at that offset. Still I took the comment out as we don't know the
format of that object anyway, so it's a bit useless ....

Here's the updated patch.

Mike Hearn <mh(a)codeweavers.com>
- Make apartment access thread-safe by introducing refcounting and wider
  usage of the apartment lock
- Rework OLE TLS management to eliminate uninitialised apartments and
  parent chaining

[OLE #21] Make apartment access thread safe, rework OLE TLS management (take 2)

[Robert Shearman]

Mike Hearn wrote:
> On Tue, 2005-01-04 at 12:21 -0600, Robert Shearman wrote:
> > > HRESULT WINAPI CoGetState(IUnknown ** ppv)
> > > {
> > > -	APARTMENT * apt = COM_CurrentInfo();
> > > +    HRESULT    hr  = E_FAIL;
> > >
> > > -	FIXME("\n");
> > > +    FIXME("possible stub\n");
> >
> > I believe the function does everything that is required of it - the only 
> > problem is that we don't currently do anything with the state. I'm not 
> > sure if we should though.
>
> Do we actually have any docs on this function? I agree the "possible
> stub" fixme is annoying though so I'll remove it.

No. All I know is that native oleaut32 uses it for some purpose.
> > This should be moved into the TLS struct, not removed entirely.
>
> inits has been subsumed into the refcount, each CoInitialize adds a ref.
> Why is it still needed?

Because inits should be thread-local, not apartment scoped. 
NtCurrentTeb()->ReservedForOle should be set to NULL when the matching 
CoUninitialize is called, not when the apartment is destroyed.
> Here's the updated patch.
>
> Mike Hearn <mh(a)codeweavers.com>
> - Make apartment access thread-safe by introducing refcounting and wider
>  usage of the apartment lock
> - Rework OLE TLS management to eliminate uninitialised apartments and
>  parent chaining

Rob

[OLE #21] Make apartment access thread safe, rework OLE TLS management (take 2)

[Mike Hearn]

On Tue, 2005-01-04 at 15:27 -0600, Robert Shearman wrote:
> Because inits should be thread-local, not apartment scoped. 
> NtCurrentTeb()->ReservedForOle should be set to NULL when the matching
> CoUninitialize is called, not when the apartment is destroyed.

I was under the impression that CoSetState/*ErrorInfo weren't tied to
apartments at all, hence the whole uninitialised apartments thing. So
it's not right to set ReservedForOle to NULL when CoUninitialise is
called with this patch because ReservedForOle now points to a struct
oletls not an apartment (which should be refcounted so CoUninitialise in
the MTA doesn't trigger its destruction).

That does leave the question of how to free the OLE TLS data though.
Presumably in a THREAD_DETACH notification.

[OLE #21] Make apartment access thread safe, rework OLE TLS management (take 2)

[Robert Shearman]

Mike Hearn wrote:
> On Tue, 2005-01-04 at 15:27 -0600, Robert Shearman wrote:
> > Because inits should be thread-local, not apartment scoped. 
> > NtCurrentTeb()->ReservedForOle should be set to NULL when the matching
> > CoUninitialize is called, not when the apartment is destroyed.
>
> I was under the impression that CoSetState/*ErrorInfo weren't tied to
> apartments at all, hence the whole uninitialised apartments thing. So
> it's not right to set ReservedForOle to NULL when CoUninitialise is
> called with this patch because ReservedForOle now points to a struct
> oletls not an apartment (which should be refcounted so CoUninitialise in
> the MTA doesn't trigger its destruction).

You are right. My tests show that the TLS state struct isn't be freed 
when the final CoUninitialize is called. However, it should still detach 
from the apartment. I believe we should do something like this in 
CoUninitialize:

if (!--COM_CurrentInfo()->inits)
{
    COM_ApartmentRelease(COM_CurrentInfo()->apt);
    COM_CurrentInfo()->apt = NULL;
}

and obviously increment COM_CurrentInfo()->inits in CoInitializeEx. And 
we should only add a reference to the apartment once in CoInitializeEx.
> That does leave the question of how to free the OLE TLS data though.
> Presumably in a THREAD_DETACH notification.

That sounds like the best plan, otherwise we will leak a small amount of 
memory for each thread that uses COM functions.

Rob

[OLE #21] Make apartment access thread safe, rework OLE TLS management (take 2)

[Mike Hearn]

On Tue, 04 Jan 2005 16:35:15 -0600, Robert Shearman wrote:
> and obviously increment COM_CurrentInfo()->inits in CoInitializeEx. And 
> we should only add a reference to the apartment once in CoInitializeEx.

What does that buy us that a 1:1 mapping between CoInitializeEx and
apartment refs doesn't? I'm not sure why refcounting the OLETLS structure
is useful.

[OLE #21] Make apartment access thread safe, rework OLE TLS management (take 2)

[Robert Shearman]

Mike Hearn wrote:
> On Tue, 04 Jan 2005 16:35:15 -0600, Robert Shearman wrote:
> > and obviously increment COM_CurrentInfo()->inits in CoInitializeEx. And 
> > we should only add a reference to the apartment once in CoInitializeEx.
>
> What does that buy us that a 1:1 mapping between CoInitializeEx and
> apartment refs doesn't? I'm not sure why refcounting the OLETLS structure
> is useful.

If you refcount the apartment then you could get the following situation:
Thread1: CoInitialize(MTA)
Thread2: CoInitialize(MTA) - note both threads now have a pointer to the 
same apt
Thread1: CoUninitialize() - still has pointer to MTA
Thread2: CoUninitialize() - detects apartment being destroyed and 
removes pointer to MTA
Thread1: Some marshalling function - BOOM! using free'd memory.

If you remove the pointer to the apartment on every CoUninitialize then 
you will break applications depending on it not separating from the 
apartment until the last CoUninitialize.

Rob