Re: ieframe: Don't release the advise sink in IOleObject::SetClientSite.

[Jacek Caban]

Hi Hans,

On 11/26/15 10:53, Hans Leidekker wrote:
> diff --git a/dlls/ieframe/tests/webbrowser.c b/dlls/ieframe/tests/webbrowser.c
> index c8dfa5b..f62a943 100644
> --- a/dlls/ieframe/tests/webbrowser.c
> +++ b/dlls/ieframe/tests/webbrowser.c
> @@ -148,6 +148,7 @@ DEFINE_EXPECT(ControlSite_TranslateAccelerator);
>  DEFINE_EXPECT(OnFocus_TRUE);
>  DEFINE_EXPECT(OnFocus_FALSE);
>  DEFINE_EXPECT(GetExternal);
> +DEFINE_EXPECT(AdviseSink_Release);
>
> static const WCHAR wszItem[] = {'i','t','e','m',0};
>
> @@ -3955,6 +3956,7 @@ static ULONG WINAPI sink_AddRef(IAdviseSink *iface)
>
> static ULONG WINAPI sink_Release(IAdviseSink *iface)
>  {
> +    CHECK_EXPECT2(AdviseSink_Release);
>      return 1;
>  }

IMO checking GetAdvise result after SetClientSite() call would be
cleaner than checking Release calls.
> @@ -4003,6 +4005,7 @@ static void test_SetAdvise(void)
>      IWebBrowser2 *browser;
>      IViewObject2 *view;
>      IAdviseSink *sink;
> +    IOleObject *oleobj;
>      DWORD aspects, flags;
>
> if (!(browser = create_webbrowser())) return;
> @@ -4031,9 +4034,22 @@ static void test_SetAdvise(void)
>      ok(!flags, "got %08x\n", aspects);
>      ok(sink == &test_sink, "got %p\n", sink);
>
> +    hr = IWebBrowser2_QueryInterface(browser, &IID_IOleObject, (void **)&oleobj);
> +    ok(hr == S_OK, "got %08x\n", hr);
> +
> +    SET_EXPECT(GetContainer);
> +    SET_EXPECT(Site_GetWindow);
> +    SET_EXPECT(Invoke_AMBIENT_OFFLINEIFNOTCONNECTED);
> +    SET_EXPECT(Invoke_AMBIENT_SILENT);
> +    hr = IOleObject_SetClientSite(oleobj, &ClientSite);
> +    ok(hr == S_OK, "got %08x\n", hr);

You can't do it that way. All SET_EXPECT()s need CHECK_CALLED() or
alike. Otherwise you leave tests in an inconsistent state.
> +
> +    SET_EXPECT(AdviseSink_Release);
>      hr = IViewObject2_SetAdvise(view, 0, 0, NULL);
>      ok(hr == S_OK, "got %08x\n", hr);
> +    CHECK_CALLED(AdviseSink_Release);

It would be also interesting to see what happens in SetClientSite(NULL)
call.


Thanks,
Jacek