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