[PATCH v7 0/4] MR4380: mshtml: Keep inner window's link to the outer window until it's destroyed.
Here's a list of usages of `base.outer_window` that may be impacted by the change and my analysis/fixes. Note that I specifically did not look for `document->outer_window` since that one was already holding it forever, so it's out of scope for this MR (whether it's correct or not, the behavior is the same after this MR). I also didn't consider some cases that already **did not check** for it being NULL or not, because they would have crashed earlier before anyway (they'd still crash now if outer window gets killed, but it's out of scope of the MR as I said). For those that says "Handled" it means I've added checks so that the behavior is same as before, i.e. detached inner windows behave as if outer window is NULL. * `htmldoc.c/HTMLDocument_get_mimeType`: Changed so it doesn't rely on outer window. * `htmldoc.c/HTMLDocument7_get_defaultView`: This is actually fixed by these patches. * `htmlform.c/HTMLFormElement_submit`: Handled. * `htmlstorage.c/send_storage_event`: Fixed in a separate patch with tests, since it follows the outer window. * `htmlwindow.c/check_target_origin`: This (along with many other methods) can't have it NULL anymore or detached, since the caller must hold a ref to the outer window, and they're not called from our objects holding ref to inner window only. * `htmlwindow.c/HTMLWindowSP_QueryService`: This uses the outer window to obtain the browser and the doc obj, so it's fine. * `mutation.c/set_document_mode`: Handled. * `navigate.c/BindCallbackRedirect_Redirect`: Handled. * `navigate.c/nsAsyncVerifyRedirectCallback_OnRedirectVerifyCallback`: Handled. * `navigate.c/nsChannelBSC_init_bindinfo`: Uses the outer window to obtain the browser and doc obj to set a flag, so it's harmless and probably correct this way. * `navigate.c/handle_navigation_error`: Handled. * `navigate.c/handle_extern_mime_navigation`: Handled. * `omnavigator.c/OmHistory_get_length`: This is actually fixed by these patches. * `script.c/ActiveScriptSite_GetItemInfo`: Handled. * `script.c/ActiveScriptSiteWindow_GetWindow`: Handled. -- v7: mshtml/tests: Fix XHR leak in test_window_refs. mshtml: Don't check for NULL outer_window from within HTMLWindow* methods. mshtml: Don't attempt to send storage events after outer window is detached. mshtml: Don't rely on the outer_window in document.mimeType. https://gitlab.winehq.org/wine/wine/-/merge_requests/4380
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> This would have crashed on detached windows, and checking the navigation_start time has the same effect except it's decoupled from the outer window. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmldoc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index 002d2553e4f..e37f04fee0b 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -1296,7 +1296,7 @@ static HRESULT WINAPI HTMLDocument_get_mimeType(IHTMLDocument2 *iface, BSTR *p) *p = NULL; - if(This->window && This->window->base.outer_window->readystate == READYSTATE_UNINITIALIZED) + if(This->window && !This->window->navigation_start_time) return (*p = SysAllocString(L"")) ? S_OK : E_FAIL; nsAString_InitDepend(&nsstr, NULL); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4380
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlstorage.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/mshtml/htmlstorage.c b/dlls/mshtml/htmlstorage.c index efd4b2d88ae..06d16e4e4aa 100644 --- a/dlls/mshtml/htmlstorage.c +++ b/dlls/mshtml/htmlstorage.c @@ -322,6 +322,11 @@ static HRESULT send_storage_event(HTMLStorage *storage, BSTR key, BSTR old_value HRESULT hres = S_OK; ctx.url = NULL; + + /* FIXME: Events are actually sent to the current window on native, even if we're detached. */ + if(!window->base.outer_window) + goto done; + if(window->base.outer_window->uri_nofrag) { hres = IUri_GetDisplayUri(window->base.outer_window->uri_nofrag, &ctx.url); if(hres != S_OK) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4380
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> These methods are not called from any of our objects that hold ref to the inner window only, and since we return outer windows to external callers now, they *must* have a ref to the outer window, which effectively means these cannot be NULL. And some other places didn't check for NULL either (e.g. get_parent). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlwindow.c | 68 ++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 41 deletions(-) diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index fc0a03e3bf8..fa99c453989 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -584,7 +584,7 @@ static HRESULT WINAPI HTMLWindow2_alert(IHTMLWindow2 *iface, BSTR message) TRACE("(%p)->(%s)\n", This, debugstr_w(message)); - if(!This->outer_window || !This->outer_window->browser) + if(!This->outer_window->browser) return E_UNEXPECTED; if(!LoadStringW(get_shdoclc(), IDS_MESSAGE_BOX_TITLE, title, ARRAY_SIZE(title))) { @@ -617,7 +617,7 @@ static HRESULT WINAPI HTMLWindow2_confirm(IHTMLWindow2 *iface, BSTR message, if(!confirmed) return E_INVALIDARG; - if(!This->outer_window || !This->outer_window->browser) + if(!This->outer_window->browser) return E_UNEXPECTED; if(!LoadStringW(get_shdoclc(), IDS_MESSAGE_BOX_TITLE, wszTitle, ARRAY_SIZE(wszTitle))) { @@ -711,7 +711,7 @@ static HRESULT WINAPI HTMLWindow2_prompt(IHTMLWindow2 *iface, BSTR message, TRACE("(%p)->(%s %s %p)\n", This, debugstr_w(message), debugstr_w(dststr), textdata); - if(!This->outer_window || !This->outer_window->browser) + if(!This->outer_window->browser) return E_UNEXPECTED; if(textdata) V_VT(textdata) = VT_NULL; @@ -846,7 +846,7 @@ static HRESULT WINAPI HTMLWindow2_close(IHTMLWindow2 *iface) TRACE("(%p)\n", This); - if(!window || !window->browser) { + if(!window->browser) { FIXME("No document object\n"); return E_FAIL; } @@ -953,7 +953,7 @@ static HRESULT WINAPI HTMLWindow2_open(IHTMLWindow2 *iface, BSTR url, BSTR name, if(replace) FIXME("unsupported relace argument\n"); - if(!window || !window->browser || !window->uri_nofrag) + if(!window->browser || !window->uri_nofrag) return E_UNEXPECTED; if(name && *name == '_') { @@ -1310,7 +1310,7 @@ static HRESULT WINAPI HTMLWindow2_focus(IHTMLWindow2 *iface) TRACE("(%p)->()\n", This); - if(!This->outer_window || !This->outer_window->browser) + if(!This->outer_window->browser) return E_UNEXPECTED; SetFocus(This->outer_window->browser->doc->hwnd); @@ -1477,7 +1477,7 @@ static HRESULT WINAPI HTMLWindow2_get_external(IHTMLWindow2 *iface, IDispatch ** TRACE("(%p)->(%p)\n", This, p); - if(!This->outer_window || !This->outer_window->browser) + if(!This->outer_window->browser) return E_UNEXPECTED; *p = NULL; @@ -2396,9 +2396,6 @@ static HRESULT WINAPI HTMLWindow7_getComputedStyle(IHTMLWindow7 *iface, IHTMLDOM TRACE("(%p)->(%p %s %p)\n", This, node, debugstr_w(pseudo_elt), p); - if(!This->outer_window || !This->inner_window) - return E_UNEXPECTED; - hres = IHTMLDOMNode_QueryInterface(node, &IID_IHTMLElement, (void**)&elem); if(FAILED(hres)) return hres; @@ -2832,7 +2829,7 @@ static HRESULT WINAPI HTMLPrivateWindow_SuperNavigate(IHTMLPrivateWindow *iface, if(flags & ~2) FIXME("unimplemented flags %lx\n", flags & ~2); - if(!window || !window->browser) + if(!window->browser) return E_FAIL; if(window->browser->doc->hostui) { @@ -3641,6 +3638,9 @@ static HRESULT WINAPI WindowDispEx_GetDispID(IDispatchEx *iface, BSTR bstrName, { HTMLWindow *This = impl_from_IDispatchEx(iface); HTMLInnerWindow *window = This->inner_window; + HTMLOuterWindow *frame; + global_prop_t *prop; + IHTMLElement *elem; HRESULT hres; TRACE("(%p)->(%s %lx %p)\n", This, debugstr_w(bstrName), grfdex, pid); @@ -3653,38 +3653,27 @@ static HRESULT WINAPI WindowDispEx_GetDispID(IDispatchEx *iface, BSTR bstrName, if(hres != DISP_E_UNKNOWNNAME) return hres; - if(This->outer_window) { - HTMLOuterWindow *frame; - - hres = get_frame_by_name(This->outer_window, bstrName, FALSE, &frame); - if(SUCCEEDED(hres) && frame) { - global_prop_t *prop; - - prop = alloc_global_prop(window, GLOBAL_FRAMEVAR, bstrName); - if(!prop) - return E_OUTOFMEMORY; + hres = get_frame_by_name(This->outer_window, bstrName, FALSE, &frame); + if(SUCCEEDED(hres) && frame) { + prop = alloc_global_prop(window, GLOBAL_FRAMEVAR, bstrName); + if(!prop) + return E_OUTOFMEMORY; - *pid = prop_to_dispid(window, prop); - return S_OK; - } + *pid = prop_to_dispid(window, prop); + return S_OK; } - if(window->doc) { - global_prop_t *prop; - IHTMLElement *elem; - - hres = IHTMLDocument3_getElementById(&window->base.inner_window->doc->IHTMLDocument3_iface, - bstrName, &elem); - if(SUCCEEDED(hres) && elem) { - IHTMLElement_Release(elem); + hres = IHTMLDocument3_getElementById(&window->base.inner_window->doc->IHTMLDocument3_iface, + bstrName, &elem); + if(SUCCEEDED(hres) && elem) { + IHTMLElement_Release(elem); - prop = alloc_global_prop(window, GLOBAL_ELEMENTVAR, bstrName); - if(!prop) - return E_OUTOFMEMORY; + prop = alloc_global_prop(window, GLOBAL_ELEMENTVAR, bstrName); + if(!prop) + return E_OUTOFMEMORY; - *pid = prop_to_dispid(window, prop); - return S_OK; - } + *pid = prop_to_dispid(window, prop); + return S_OK; } return DISP_E_UNKNOWNNAME; @@ -4120,9 +4109,6 @@ static HRESULT HTMLWindow_invoke(DispatchEx *dispex, DISPID id, LCID lcid, WORD return E_NOTIMPL; } case GLOBAL_FRAMEVAR: - if(!This->base.outer_window) - return E_UNEXPECTED; - switch(flags) { case DISPATCH_PROPERTYGET: { HTMLOuterWindow *frame; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4380
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/tests/events.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index 4b775ceb8b8..4732236c52c 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -3320,6 +3320,7 @@ static void test_window_refs(IHTMLDocument2 *doc) hres = IHTMLXMLHttpRequestFactory_create(xhr_factory, &xhr); ok(hres == S_OK, "create failed: %08lx\n", hres); IHTMLXMLHttpRequestFactory_Release(xhr_factory); + IHTMLXMLHttpRequest_Release(xhr); hres = IHTMLImageElementFactory_create(image_factory, vempty, vempty, &img_elem); ok(hres == S_OK, "create failed: %08lx\n", hres); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4380
I don't think "avoiding invalid memory access" is lack of a proper justification though, even if the reason was different.
There are many ways to fix that and for choosing the right is the tricky part. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4380#note_54878
This merge request was approved by Jacek Caban. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4380
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)