[PATCH 2/6] ole32: Allow more functions to use the implicit MTA.

[Zebediah Figura]

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