[OLE #21] Make apartment access thread safe, rework OLE TLS management
Robert Shearman
rob at codeweavers.com
Tue Jan 4 12:21:47 CST 2005
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 at 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
More information about the wine-devel
mailing list