From: Jacek Caban Subject: Re: ieframe: Don't release the advise sink in IOleObject::SetClientSite. Message-Id: <56571A73.2060202@codeweavers.com> Date: Thu, 26 Nov 2015 15:42:59 +0100 In-Reply-To: <1448531612-27786-1-git-send-email-hans@codeweavers.com> References: <1448531612-27786-1-git-send-email-hans@codeweavers.com> 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

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