On 28/03/18 03:43, Huw Davies wrote:
On Sun, Mar 25, 2018 at 12:33:58PM -0500, Zebediah Figura wrote:
It's necessary to introduce the being_destroyed field since a destroyed apartment may end up releasing proxy objects, which will require calling various functions, e.g. CoGetPSClsid(), potentially triggering an infinite recursion otherwise.
Signed-off-by: Zebediah Figura z.figura12@gmail.com
dlls/ole32/compobj.c | 165 +++++++++++++++++++++++++------------------ dlls/ole32/compobj_private.h | 1 + dlls/ole32/tests/compobj.c | 23 ++++++ 3 files changed, 120 insertions(+), 69 deletions(-)
diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c index cd0e67f..61a8ae6 100644 --- a/dlls/ole32/compobj.c +++ b/dlls/ole32/compobj.c @@ -717,6 +717,44 @@ static inline BOOL apartment_is_model(const APARTMENT *apt, DWORD model) return (apt->multi_threaded == !(model & COINIT_APARTMENTTHREADED)); }
+/* gets the multi-threaded apartment if it exists. The caller must
- release the reference from the apartment as soon as the apartment pointer
- is no longer required. */
+static APARTMENT *apartment_find_multi_threaded(void) +{
- APARTMENT *result = NULL;
- struct list *cursor;
- EnterCriticalSection(&csApartment);
- LIST_FOR_EACH( cursor, &apts )
- {
struct apartment *apt = LIST_ENTRY( cursor, struct apartment, entry );if (apt->multi_threaded){result = apt;apartment_addref(result);break;}- }
I know this function was copied from below, but there's the MTA variable (protected by csApartment) that holds the MTA if it exists. So you could simply return that rather than loop through every apt.
- LeaveCriticalSection(&csApartment);
- return result;
+}
+/* Return the current apartment if it exists, or, failing that, the MTA. Caller
- must free the returned apartment in either case. */
+static APARTMENT *apartment_get_current_or_mta(void) +{
- APARTMENT *apt = COM_CurrentApt();
- if (apt)
- {
apartment_addref(apt);return apt;- }
- return apartment_find_multi_threaded();
+}
So this patch is doing a couple of things. It's taking a ref on the current apt if one exists or using the MTA if it doesn't. Since this code is so fragile, I'd like these split. Something like this would work:
2.1 Add apartment_addref()s / apartment_release()s to CoGetClassObject(), CoLockObjectExternal(), etc. i.e. implement the 'taking a ref on the current apt' bit.
2.2 Move apartment_find_multi_threaded() higher up in the file and rewrite using the MTA variable. Also why not rename this to apartment_find_mta()
2.3 Add apartment_get_current_or_mta() and use it in CoGetClassObject() etc.
Also, I'm not yet 100% convinced about the being_destroyed thing. I'm wondering whether there's a better way of doing that. Hopefully splitting this up will help with that.
Huw.
Thanks, that sounds like a good idea.
I think I considered locking the apartment using its critical section instead and didn't due to concerns with marshalling, but I'll take a closer look and see if that should be possible.