ole32 : wrong check in compobj.c

[Paul Vriens]

Hi,

while looking through the code I saw:

if (model & COINIT_MULTITHREADED)

this will never work as COINIT_MULTITHREADED = 0.

The attached patch fixes the 3 occurrences of a check against COINIT_*.

This patch however makes the COM_CreateApartment in compobj.c:

545   if (!(apt = COM_CurrentInfo()->apt))
    546   {
    547     apt = COM_CreateApartment(dwCoInit);
    548     if (!apt) return E_OUTOFMEMORY;
    549   }

return E_OUTOFMEMORY, always (?). And that makes sure that we have
a mismatch between CoInitialize and CoUninitialize.

So there's definitely something else wrong as well.

Any idea ?

Cheers,

Paul.

[Robert Shearman]

Paul Vriens wrote:
> Hi,
>
> while looking through the code I saw:
>
> if (model & COINIT_MULTITHREADED)
>
> this will never work as COINIT_MULTITHREADED = 0.

Good catch.
> The attached patch fixes the 3 occurrences of a check against COINIT_*.
>
> This patch however makes the COM_CreateApartment in compobj.c:
>
> 545   if (!(apt = COM_CurrentInfo()->apt))
>    546   {
>    547     apt = COM_CreateApartment(dwCoInit);
>    548     if (!apt) return E_OUTOFMEMORY;
>    549   }
>
> return E_OUTOFMEMORY, always (?). And that makes sure that we have
> a mismatch between CoInitialize and CoUninitialize.
>
> So there's definitely something else wrong as well.
>
> Any idea ?

The parameter to CoInitializeEx is of type COINIT and so the caller can also specify another flag like COINIT_DISABLE_OLE1DDE along with the apartment flag.
So we either need to convert the values to booleans before we call the lower level COM_CreateApartment or we need to be more careful and change the comparisons to this:
if ((dwCoInit & (COINIT_APARTMENTTHREADED|COINIT_MULTITHREADED) == COINIT_MULTITHREADED) ...
> Index: dlls/ole32/compobj.c
>
> RCS file: /home/wine/wine/dlls/ole32/compobj.c,v
> retrieving revision 1.121
> diff -u -r1.121 compobj.c
> --- dlls/ole32/compobj.c	6 Jan 2005 19:39:07 -0000	1.121
> +++ dlls/ole32/compobj.c	10 Jan 2005 16:41:00 -0000
> @@ -252,13 +252,13 @@
> if (!apt)
> {
> -
>    if (model & COINIT_MULTITHREADED)
> -
>    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));
>
> @@ -277,7 +277,7 @@
>     /* we don't ref the apartment as CoInitializeEx will do it for us */
>
> -
>    if (model & COINIT_APARTMENTTHREADED)
> -
>    if (model == COINIT_APARTMENTTHREADED)
>   {
>       /* FIXME: how does windoze create OXIDs? */
>       apt->oxid = MTA.oxid | GetCurrentThreadId();
>
> @@ -316,7 +316,7 @@
>     MARSHAL_Disconnect_Proxies(apt);
>
> -
>    if ((apt->model & COINIT_APARTMENTTHREADED) && apt->win) DestroyWindow(apt->win);
> -
>    if ((apt->model == COINIT_APARTMENTTHREADED) && apt->win) DestroyWindow(apt->win);

This change can be simplified to just check for the non-null apt->win.

Rob

[Paul Vriens]

On Mon, 2005-01-10 at 18:02, Robert Shearman wrote:
> Paul Vriens wrote:
> > Hi,
> >
> > while looking through the code I saw:
> >
> > if (model & COINIT_MULTITHREADED)
> >
> > this will never work as COINIT_MULTITHREADED = 0.
>
> Good catch.
> > The attached patch fixes the 3 occurrences of a check against COINIT_*.
> >
> > This patch however makes the COM_CreateApartment in compobj.c:
> >
> > 545   if (!(apt = COM_CurrentInfo()->apt))
> >    546   {
> >    547     apt = COM_CreateApartment(dwCoInit);
> >    548     if (!apt) return E_OUTOFMEMORY;
> >    549   }
> >
> > return E_OUTOFMEMORY, always (?). And that makes sure that we have
> > a mismatch between CoInitialize and CoUninitialize.
> >
> > So there's definitely something else wrong as well.
> >
> > Any idea ?
>
> The parameter to CoInitializeEx is of type COINIT and so the caller can also specify another flag like COINIT_DISABLE_OLE1DDE along with the apartment flag.
> So we either need to convert the values to booleans before we call the lower level COM_CreateApartment or we need to be more careful and change the comparisons to this:
> if ((dwCoInit & (COINIT_APARTMENTTHREADED|COINIT_MULTITHREADED) == COINIT_MULTITHREADED) ...
> > -
> >    if ((apt->model & COINIT_APARTMENTTHREADED) && apt->win) DestroyWindow(apt->win);
> > -
> >    if ((apt->model == COINIT_APARTMENTTHREADED) && apt->win) DestroyWindow(apt->win);
>
> This change can be simplified to just check for the non-null apt->win.
>
> Rob

Any volunteers for this stuff? It's way over my head already :-). 
I will try as much as I can, but I'm sure the real men have this nailed
in a sec.

Cheers,

Paul.

[Robert Shearman]

Paul Vriens wrote:
> On Mon, 2005-01-10 at 18:02, Robert Shearman wrote:
> > The parameter to CoInitializeEx is of type COINIT and so the caller can also specify another flag like COINIT_DISABLE_OLE1DDE along with the apartment flag.
> > So we either need to convert the values to booleans before we call the lower level COM_CreateApartment or we need to be more careful and change the comparisons to this:
> > if ((dwCoInit & (COINIT_APARTMENTTHREADED|COINIT_MULTITHREADED) == COINIT_MULTITHREADED) ...
>
> Any volunteers for this stuff? It's way over my head already :-). 
> I will try as much as I can, but I'm sure the real men have this nailed
> in a sec.

I'll knock something up later on today if I have time, or tomorrow 
otherwise.

Rob

[Bill Medland]

On January 10, 2005 11:40 am, Robert Shearman wrote:
> Paul Vriens wrote:
> > On Mon, 2005-01-10 at 18:02, Robert Shearman wrote:
> > > The parameter to CoInitializeEx is of type COINIT and so the caller can
> > > also specify another flag like COINIT_DISABLE_OLE1DDE along with the
> > > apartment flag. So we either need to convert the values to booleans
> > > before we call the lower level COM_CreateApartment or we need to be more
> > > careful and change the comparisons to this: if ((dwCoInit &
> > > (COINIT_APARTMENTTHREADED|COINIT_MULTITHREADED) == COINIT_MULTITHREADED)
> > > ...
> >
> > Any volunteers for this stuff? It's way over my head already :-).
> > I will try as much as I can, but I'm sure the real men have this nailed
> > in a sec.
>
> I'll knock something up later on today if I have time, or tomorrow
> otherwise.
>
> Rob

I've done it;
(I wonder if the optimizer will replace it with "not apartment threaded" for 
us)