Re: [PATCH 2/2] wmp: Implement IConnectionPoint[Container] and add _WMPOCXEvents

[Anton Romanov]

> - while(cConnections--) {
> -
>    while(This->iter < This->cp->sinks_size &&
>
> !This->cp->sinks[This->iter])
> -
>        This->iter++;
> -
>    if(This->iter == This->cp->sinks_size)
> -
>        break;
> -
> -
>    pgcd[cnt].pUnk = (IUnknown*)This->cp->sinks[This->iter];
>
>
> I think you're missing IUnknown_AddRef() here.

It does look like it should addref those. I will doublecheck.
Also, all of this code is for the most part a copy of
dlls/ieframe/events.c . That means that it might also be impacted by
this.
> +void call_sink(ConnectionPoint *This, DISPID dispid, DISPPARAMS
> *dispparams)
> +{
> - DWORD i;
> -
> - for(i=0; i<This->sinks_size; i++) {
> -
>    if(This->sinks[i])
> -
>        IDispatch_Invoke(This->sinks[i], dispid, &IID_NULL,
>
> LOCALE_SYSTEM_DEFAULT,
> -
>                         DISPATCH_METHOD, dispparams, NULL, NULL,
>
> NULL);
> - }
>
> +}
>
> You don't use it in this patch, so it's a dead code. Please remove it from
> this patch (and introduce it with a patch that uses it).

Sure. I'll move this part to patch where I start using it.
> HRESULT WINAPI WMPFactory_CreateInstance(IClassFactory *iface, IUnknown
> *outer,
>          REFIID riid, void **ppv)
>  {
> @@ -941,22 +894,25 @@ HRESULT WINAPI WMPFactory_CreateInstance(IClassFactory
> *iface, IUnknown *outer,
>      wmp->IProvideClassInfo2_iface.lpVtbl = &ProvideClassInfo2Vtbl;
>      wmp->IPersistStreamInit_iface.lpVtbl = &PersistStreamInitVtbl;
>      wmp->IOleInPlaceObjectWindowless_iface.lpVtbl =
> &OleInPlaceObjectWindowlessVtbl;
> - wmp->IConnectionPointContainer_iface.lpVtbl =
>
> &ConnectionPointContainerVtbl;
>      wmp->IOleControl_iface.lpVtbl = &OleControlVtbl;
>  wmp->ref = 1;
>
> - init_player_ifaces(wmp);
> -
> - hdc = GetDC(0);
> - dpi_x = GetDeviceCaps(hdc, LOGPIXELSX);
> - dpi_y = GetDeviceCaps(hdc, LOGPIXELSY);
> - ReleaseDC(0, hdc);
> - hres = init_player(wmp);
> - if (hres == S_OK) {
> -
>    ConnectionPointContainer_Init(wmp);
> -
>    hdc = GetDC(0);
> -
>    dpi_x = GetDeviceCaps(hdc, LOGPIXELSX);
> -
>    dpi_y = GetDeviceCaps(hdc, LOGPIXELSY);
> -
>    ReleaseDC(0, hdc);
>
> - wmp->extent.cx = MulDiv(192, 2540, dpi_x);
> - wmp->extent.cy = MulDiv(192, 2540, dpi_y);
> -
>    wmp->extent.cx = MulDiv(192, 2540, dpi_x);
> -
>    wmp->extent.cy = MulDiv(192, 2540, dpi_y);
>
> - hres = IOleObject_QueryInterface(&wmp->IOleObject_iface, riid, ppv);
> - IOleObject_Release(&wmp->IOleObject_iface);
> -
>    hres = IOleObject_QueryInterface(&wmp->IOleObject_iface, riid,
>
> ppv);
> -
>    IOleObject_Release(&wmp->IOleObject_iface);
> - }
> - if(hres != S_OK)
> -
>    destroy_player(wmp);
> return hres;
>
> Error handling looks wrong here. If QueryInterface() fails, you'd both
> release wmp and call destroy_wmp. Also init_player can't fail, so it should
> return void.

Thanks, you are right. I broke that logic and it may lead to double free :(
And I'll change init to void until later patch where it may actually fail, sure.