[PATCH v5 0/8] 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. -- v5: mshtml/tests: Fix XHR leak in test_window_refs. mshtml: Get rid of outer_window member in HTMLDocumentNode. mshtml/tests: Improve the iframe navigation test. mshtml: Remove the inner window ref from the doc only when it is actually mshtml: Don't check for NULL outer_window from within HTMLWindow* methods. mshtml: Don't attempt to send storage events after outer window is destroyed. mshtml: Keep the inner window's outer_window pointer alive until it is 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 540bbbd1626..3ace8e88ec2 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -1287,7 +1287,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/htmlform.c | 2 +- dlls/mshtml/htmlwindow.c | 25 ++++++++++++++++++++----- dlls/mshtml/mshtml_private.h | 8 ++++++++ dlls/mshtml/mutation.c | 2 +- dlls/mshtml/navigate.c | 8 ++++---- dlls/mshtml/script.c | 4 ++-- dlls/mshtml/tests/events.c | 1 - 7 files changed, 36 insertions(+), 14 deletions(-) diff --git a/dlls/mshtml/htmlform.c b/dlls/mshtml/htmlform.c index fc30742d540..0f084ef9341 100644 --- a/dlls/mshtml/htmlform.c +++ b/dlls/mshtml/htmlform.c @@ -570,7 +570,7 @@ static HRESULT WINAPI HTMLFormElement_submit(IHTMLFormElement *iface) if(This->element.node.doc) { HTMLDocumentNode *doc = This->element.node.doc; - if(doc->window && doc->window->base.outer_window) + if(doc->window && !is_detached_window(doc->window)) this_window = doc->window->base.outer_window; } if(!this_window) { diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index fc0a03e3bf8..e4bee24129d 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -110,6 +110,10 @@ static void detach_inner_window(HTMLInnerWindow *window) HTMLOuterWindow *outer_window = window->base.outer_window; HTMLDocumentNode *doc = window->doc; + /* Check if already detached */ + if(!list_empty(&window->outer_window_entry)) + return; + while(!list_empty(&window->children)) { HTMLOuterWindow *child = LIST_ENTRY(list_tail(&window->children), HTMLOuterWindow, sibling_entry); @@ -134,11 +138,13 @@ static void detach_inner_window(HTMLInnerWindow *window) abort_window_bindings(window); remove_target_tasks(window->task_magic); release_script_hosts(window); - window->base.outer_window = NULL; - if(outer_window && outer_window->base.inner_window == window) { - outer_window->base.inner_window = NULL; - IHTMLWindow2_Release(&window->base.IHTMLWindow2_iface); + if(outer_window) { + list_add_tail(&outer_window->detached_inner_windows, &window->outer_window_entry); + if(outer_window->base.inner_window == window) { + outer_window->base.inner_window = NULL; + IHTMLWindow2_Release(&window->base.IHTMLWindow2_iface); + } } } @@ -4019,6 +4025,7 @@ static void HTMLWindow_destructor(DispatchEx *dispex) HTMLInnerWindow *This = impl_from_DispatchEx(dispex); unsigned i; + list_remove(&This->outer_window_entry); VariantClear(&This->performance); for(i = 0; i < This->global_prop_cnt; i++) @@ -4370,6 +4377,12 @@ static nsresult NSAPI outer_window_unlink(void *p) unlink_ref(&window->window_proxy); wine_rb_remove(&window_map, &window->entry); } + while(!list_empty(&window->detached_inner_windows)) { + HTMLInnerWindow *inner_window = LIST_ENTRY(list_head(&window->detached_inner_windows), HTMLInnerWindow, outer_window_entry); + list_remove(&inner_window->outer_window_entry); + list_init(&inner_window->outer_window_entry); + inner_window->base.outer_window = NULL; + } return NS_OK; } @@ -4428,6 +4441,7 @@ static HRESULT create_inner_window(HTMLOuterWindow *outer_window, IMoniker *mon, list_init(&window->script_hosts); list_init(&window->bindings); list_init(&window->script_queue); + list_init(&window->outer_window_entry); window->base.outer_window = outer_window; window->base.inner_window = window; @@ -4462,6 +4476,7 @@ HRESULT create_outer_window(GeckoBrowser *browser, mozIDOMWindowProxy *mozwindow window->base.inner_window = NULL; window->browser = browser; list_add_head(&browser->outer_windows, &window->browser_entry); + list_init(&window->detached_inner_windows); ccref_init(&window->ccref, 1); mozIDOMWindowProxy_AddRef(mozwindow); @@ -4514,7 +4529,7 @@ HRESULT create_pending_window(HTMLOuterWindow *outer_window, nsChannelBSC *chann if(outer_window->pending_window) { abort_window_bindings(outer_window->pending_window); - outer_window->pending_window->base.outer_window = NULL; + list_add_tail(&outer_window->detached_inner_windows, &outer_window->pending_window->outer_window_entry); IHTMLWindow2_Release(&outer_window->pending_window->base.IHTMLWindow2_iface); } diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index 40dcb0ebfab..cc42ce1738f 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -573,6 +573,7 @@ struct HTMLOuterWindow { struct list sibling_entry; struct wine_rb_entry entry; + struct list detached_inner_windows; }; struct HTMLInnerWindow { @@ -634,6 +635,8 @@ struct HTMLInnerWindow { ULONGLONG load_event_start_time; ULONGLONG load_event_end_time; ULONGLONG first_paint_time; + + struct list outer_window_entry; }; typedef enum { @@ -1130,6 +1133,11 @@ static inline BOOL is_main_content_window(HTMLOuterWindow *window) return window->browser && window == window->browser->content_window; } +static inline BOOL is_detached_window(HTMLInnerWindow *window) +{ + return !list_empty(&window->outer_window_entry) || !window->base.outer_window; +} + struct HTMLAttributeCollection { DispatchEx dispex; IHTMLAttributeCollection IHTMLAttributeCollection_iface; diff --git a/dlls/mshtml/mutation.c b/dlls/mshtml/mutation.c index a1438debd6f..7117e0f4740 100644 --- a/dlls/mshtml/mutation.c +++ b/dlls/mshtml/mutation.c @@ -437,7 +437,7 @@ static void set_document_mode(HTMLDocumentNode *doc, compat_mode_t document_mode TRACE("%p: %d\n", doc, document_mode); - max_compat_mode = doc->window && doc->window->base.outer_window + max_compat_mode = doc->window && !is_detached_window(doc->window) ? get_max_compat_mode(doc->window->base.outer_window->uri) : COMPAT_MODE_IE11; if(max_compat_mode < document_mode) { diff --git a/dlls/mshtml/navigate.c b/dlls/mshtml/navigate.c index 90ca22ea8b1..f05bf0be9c6 100644 --- a/dlls/mshtml/navigate.c +++ b/dlls/mshtml/navigate.c @@ -578,7 +578,7 @@ static HRESULT WINAPI BindCallbackRedirect_Redirect(IBindCallbackRedirect *iface TRACE("(%p)->(%s %p)\n", This, debugstr_w(url), vbCancel); - if(This->window && This->window->base.outer_window && (browser = This->window->base.outer_window->browser) + if(This->window && !is_detached_window(This->window) && (browser = This->window->base.outer_window->browser) && browser->doc->doc_object_service) { if(is_main_content_window(This->window->base.outer_window)) { hres = IHTMLWindow2_get_name(&This->window->base.IHTMLWindow2_iface, &frame_name); @@ -1269,7 +1269,7 @@ static nsresult NSAPI nsAsyncVerifyRedirectCallback_OnRedirectVerifyCallback(nsI ERR("AddRequest failed: %08lx\n", nsres); } - if(This->bsc->is_doc_channel && This->bsc->bsc.window && This->bsc->bsc.window->base.outer_window) { + if(This->bsc->is_doc_channel && This->bsc->bsc.window && !is_detached_window(This->bsc->bsc.window)) { IUri *uri = nsuri_get_uri(This->nschannel->uri); if(uri) { @@ -1445,7 +1445,7 @@ static void handle_navigation_error(nsChannelBSC *This, DWORD result) BSTR unk; HRESULT hres; - if(!This->is_doc_channel || !This->bsc.window || !This->bsc.window->base.outer_window + if(!This->is_doc_channel || !This->bsc.window || is_detached_window(This->bsc.window) || !This->bsc.window->base.outer_window->browser) return; @@ -1644,7 +1644,7 @@ static void handle_extern_mime_navigation(nsChannelBSC *This) VARIANT flags; HRESULT hres; - if(!This->bsc.window || !This->bsc.window->base.outer_window || !This->bsc.window->base.outer_window->browser) + if(!This->bsc.window || is_detached_window(This->bsc.window) || !This->bsc.window->base.outer_window->browser) return; doc_obj = This->bsc.window->base.outer_window->browser->doc; diff --git a/dlls/mshtml/script.c b/dlls/mshtml/script.c index cad0f17671a..514b2a9c140 100644 --- a/dlls/mshtml/script.c +++ b/dlls/mshtml/script.c @@ -374,7 +374,7 @@ static HRESULT WINAPI ActiveScriptSite_GetItemInfo(IActiveScriptSite *iface, LPC if(wcscmp(pstrName, L"window")) return DISP_E_MEMBERNOTFOUND; - if(!This->window || !This->window->base.outer_window) + if(!This->window || is_detached_window(This->window)) return E_FAIL; /* FIXME: Return proxy object */ @@ -518,7 +518,7 @@ static HRESULT WINAPI ActiveScriptSiteWindow_GetWindow(IActiveScriptSiteWindow * TRACE("(%p)->(%p)\n", This, phwnd); - if(!This->window || !This->window->base.outer_window) + if(!This->window || is_detached_window(This->window)) return E_UNEXPECTED; *phwnd = This->window->base.outer_window->browser->doc->hwnd; diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index b76e7982638..87ad00ef5a3 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -3321,7 +3321,6 @@ static void test_window_refs(IHTMLDocument2 *doc) hres = IOmHistory_get_length(history, &length); ok(hres == S_OK, "get_length failed: %08lx\n", hres); - todo_wine ok(length == 42, "length = %d\n", length); IOmHistory_Release(history); } -- 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 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/mshtml/htmlstorage.c b/dlls/mshtml/htmlstorage.c index efd4b2d88ae..d514537fe66 100644 --- a/dlls/mshtml/htmlstorage.c +++ b/dlls/mshtml/htmlstorage.c @@ -322,6 +322,10 @@ static HRESULT send_storage_event(HTMLStorage *storage, BSTR key, BSTR old_value HRESULT hres = S_OK; ctx.url = NULL; + if(!window->base.outer_window) + goto done; + + /* Events are actually sent to the current window on native, even if we're detached. */ 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 e4bee24129d..09207ec406e 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -590,7 +590,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))) { @@ -623,7 +623,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))) { @@ -717,7 +717,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; @@ -852,7 +852,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; } @@ -959,7 +959,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 == '_') { @@ -1316,7 +1316,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); @@ -1483,7 +1483,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; @@ -2402,9 +2402,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; @@ -2838,7 +2835,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) { @@ -3647,6 +3644,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); @@ -3659,38 +3659,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; @@ -4127,9 +4116,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> Not on every detach. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmldoc.c | 14 +++++++------- dlls/mshtml/tests/events.c | 5 +++++ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index 3ace8e88ec2..cd31b1b25f6 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -5707,12 +5707,6 @@ void detach_document_node(HTMLDocumentNode *doc) { unsigned i; - if(doc->window) { - HTMLInnerWindow *window = doc->window; - doc->window = NULL; - IHTMLWindow2_Release(&window->base.IHTMLWindow2_iface); - } - while(!list_empty(&doc->plugin_hosts)) detach_plugin_host(LIST_ENTRY(list_head(&doc->plugin_hosts), PluginHost, entry)); @@ -5884,14 +5878,20 @@ static void HTMLDocumentNode_traverse(DispatchEx *dispex, nsCycleCollectionTrave static void HTMLDocumentNode_unlink(DispatchEx *dispex) { HTMLDocumentNode *This = impl_from_DispatchEx(dispex); + HTMLInnerWindow *window = This->window; HTMLDOMNode_unlink(dispex); + if(window) { + This->window = NULL; + IHTMLWindow2_Release(&window->base.IHTMLWindow2_iface); + } + if(This->dom_document) { release_document_mutation(This); detach_document_node(This); This->dom_document = NULL; This->html_document = NULL; - }else if(This->window) { + }else if(window) { detach_document_node(This); } } diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index 87ad00ef5a3..b62f023ba7e 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -3574,7 +3574,12 @@ static void test_doc_obj(IHTMLDocument2 *doc) ok(hres == S_OK, "get_document failed: %08lx\n", hres); ok(doc_node != doc_node2, "doc_node == doc_node2\n"); IHTMLDocument2_Release(doc_node2); + + hres = IHTMLDocument2_get_parentWindow(doc_node, &window2); + ok(hres == S_OK, "get_parentWindow failed: %08lx\n", hres); + ok(window == window2, "window != window2\n"); IHTMLDocument2_Release(doc_node); + IHTMLWindow2_Release(window2); hres = IHTMLWindow2_get_location(window, &location2); ok(hres == S_OK, "get_location failed: %08lx\n", hres); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4380
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> It was confusing before since it made it seem like it might use the outer window, while in fact the document is unchanged on native. Now the "new" doc is used for navigating, since it's already checked to be the same as the iframes_doc (but that test fails in wine and is todo_wine). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/tests/events.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index b62f023ba7e..0ff93aeba5b 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -173,16 +173,16 @@ static const char iframe_doc_str[] = static void navigate(IHTMLDocument2*,const WCHAR*); -static BOOL iface_cmp(IUnknown *iface1, IUnknown *iface2) +static BOOL iface_cmp(void *iface1, void *iface2) { IUnknown *unk1, *unk2; if(iface1 == iface2) return TRUE; - IUnknown_QueryInterface(iface1, &IID_IUnknown, (void**)&unk1); + IUnknown_QueryInterface((IUnknown *)iface1, &IID_IUnknown, (void**)&unk1); IUnknown_Release(unk1); - IUnknown_QueryInterface(iface2, &IID_IUnknown, (void**)&unk2); + IUnknown_QueryInterface((IUnknown *)iface2, &IID_IUnknown, (void**)&unk2); IUnknown_Release(unk2); return unk1 == unk2; @@ -3170,8 +3170,8 @@ static IHTMLDocument2 *get_iframe_doc(IHTMLIFrameElement *iframe) static void test_iframe_connections(IHTMLDocument2 *doc) { + IHTMLDocument2 *iframes_doc, *new_doc; IHTMLIFrameElement *iframe; - IHTMLDocument2 *iframes_doc; DWORD cookie; IConnectionPoint *cp; IHTMLElement *element; @@ -3185,7 +3185,6 @@ static void test_iframe_connections(IHTMLDocument2 *doc) IHTMLElement_Release(element); iframes_doc = get_iframe_doc(iframe); - IHTMLIFrameElement_Release(iframe); cookie = register_cp((IUnknown*)iframes_doc, &IID_IDispatch, (IUnknown*)&div_onclick_disp); @@ -3215,23 +3214,38 @@ static void test_iframe_connections(IHTMLDocument2 *doc) ok(hres == S_OK, "put_URL failed: %08lx\n", hres); SysFreeString(str); + new_doc = get_iframe_doc(iframe); + ok(iface_cmp(new_doc, iframes_doc), "new_doc != iframes_doc\n"); + IHTMLDocument2_Release(new_doc); + SET_EXPECT(iframe_onload); pump_msgs(&called_iframe_onload); CHECK_CALLED(iframe_onload); + new_doc = get_iframe_doc(iframe); + todo_wine + ok(iface_cmp(new_doc, iframes_doc), "new_doc != iframes_doc\n"); + str = SysAllocString(L"about:test"); - hres = IHTMLDocument2_put_URL(iframes_doc, str); + hres = IHTMLDocument2_put_URL(new_doc, str); ok(hres == S_OK, "put_URL failed: %08lx\n", hres); + IHTMLDocument2_Release(new_doc); SysFreeString(str); SET_EXPECT(iframe_onload); pump_msgs(&called_iframe_onload); CHECK_CALLED(iframe_onload); + + new_doc = get_iframe_doc(iframe); + todo_wine + ok(iface_cmp(new_doc, iframes_doc), "new_doc != iframes_doc\n"); + IHTMLDocument2_Release(new_doc); }else { win_skip("Skipping iframe onload tests on IE older than 9.\n"); } IHTMLDocument2_Release(iframes_doc); + IHTMLIFrameElement_Release(iframe); } static void test_window_refs(IHTMLDocument2 *doc) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4380
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Since we keep the base's outer_window valid until it's unlinked now, we do not need it anymore. Furthermore, it was wrong because it didn't even hold any sort of ref to the outer window... Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/editor.c | 6 ++--- dlls/mshtml/htmlanchor.c | 6 ++--- dlls/mshtml/htmldoc.c | 44 ++++++++++++++++++------------------ dlls/mshtml/htmlframe.c | 6 ++--- dlls/mshtml/mshtml_private.h | 1 - dlls/mshtml/mutation.c | 2 +- dlls/mshtml/nsevents.c | 10 ++++---- dlls/mshtml/olecmd.c | 4 ++-- dlls/mshtml/pluginhost.c | 4 ++-- dlls/mshtml/secmgr.c | 8 +++---- 10 files changed, 45 insertions(+), 46 deletions(-) diff --git a/dlls/mshtml/editor.c b/dlls/mshtml/editor.c index e4a16956e46..1173e8e68fc 100644 --- a/dlls/mshtml/editor.c +++ b/dlls/mshtml/editor.c @@ -146,7 +146,7 @@ static DWORD query_ns_edit_status(HTMLDocumentNode *doc, const char *nscmd) nsICommandParams *nsparam; cpp_bool b = FALSE; - if(doc->browser->usermode != EDITMODE || doc->outer_window->readystate < READYSTATE_INTERACTIVE) + if(doc->browser->usermode != EDITMODE || doc->window->base.outer_window->readystate < READYSTATE_INTERACTIVE) return OLECMDF_SUPPORTED; if(nscmd) { @@ -180,7 +180,7 @@ static DWORD query_align_status(HTMLDocumentNode *doc, const WCHAR *align) cpp_bool b; nsresult nsres; - if(doc->browser->usermode != EDITMODE || doc->outer_window->readystate < READYSTATE_INTERACTIVE) + if(doc->browser->usermode != EDITMODE || doc->window->base.outer_window->readystate < READYSTATE_INTERACTIVE) return OLECMDF_SUPPORTED; if(!doc->html_document) { @@ -669,7 +669,7 @@ static HRESULT query_justify(HTMLDocumentNode *doc, OLECMD *cmd) case IDM_JUSTIFYLEFT: TRACE("(%p) IDM_JUSTIFYLEFT\n", doc); /* FIXME: We should set OLECMDF_LATCHED only if it's set explicitly. */ - if(doc->browser->usermode != EDITMODE || doc->outer_window->readystate < READYSTATE_INTERACTIVE) + if(doc->browser->usermode != EDITMODE || doc->window->base.outer_window->readystate < READYSTATE_INTERACTIVE) cmd->cmdf = OLECMDF_SUPPORTED; else cmd->cmdf = OLECMDF_SUPPORTED | OLECMDF_ENABLED; diff --git a/dlls/mshtml/htmlanchor.c b/dlls/mshtml/htmlanchor.c index 5398f87ce55..5f8a98faf17 100644 --- a/dlls/mshtml/htmlanchor.c +++ b/dlls/mshtml/htmlanchor.c @@ -49,11 +49,11 @@ static HRESULT navigate_href_new_window(HTMLElement *element, nsAString *href_st HRESULT hres; nsAString_GetData(href_str, &href); - hres = create_relative_uri(element->node.doc->outer_window, href, &uri); + hres = create_relative_uri(element->node.doc->window->base.outer_window, href, &uri); if(FAILED(hres)) return hres; - hres = navigate_new_window(element->node.doc->outer_window, uri, target, NULL, NULL); + hres = navigate_new_window(element->node.doc->window->base.outer_window, uri, target, NULL, NULL); IUri_Release(uri); return hres; } @@ -110,7 +110,7 @@ static HRESULT navigate_href(HTMLElement *element, nsAString *href_str, nsAStrin const PRUnichar *href; HRESULT hres; - window = get_target_window(element->node.doc->outer_window, target_str, &use_new_window); + window = get_target_window(element->node.doc->window->base.outer_window, target_str, &use_new_window); if(!window) { if(use_new_window) { const PRUnichar *target; diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index cd31b1b25f6..02857be3bed 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -905,7 +905,7 @@ static HRESULT WINAPI HTMLDocument_get_readyState(IHTMLDocument2 *iface, BSTR *p if(!p) return E_POINTER; - return get_readystate_string(This->outer_window ? This->outer_window->readystate : 0, p); + return get_readystate_string(This->window && This->window->base.outer_window ? This->window->base.outer_window->readystate : 0, p); } static HRESULT WINAPI HTMLDocument_get_frames(IHTMLDocument2 *iface, IHTMLFramesCollection2 **p) @@ -914,11 +914,11 @@ static HRESULT WINAPI HTMLDocument_get_frames(IHTMLDocument2 *iface, IHTMLFrames TRACE("(%p)->(%p)\n", This, p); - if(!This->outer_window) { + if(!This->window || !This->window->base.outer_window) { /* Not implemented by IE */ return E_NOTIMPL; } - return IHTMLWindow2_get_frames(&This->outer_window->base.IHTMLWindow2_iface, p); + return IHTMLWindow2_get_frames(&This->window->base.outer_window->base.IHTMLWindow2_iface, p); } static HRESULT WINAPI HTMLDocument_get_embeds(IHTMLDocument2 *iface, IHTMLElementCollection **p) @@ -1094,12 +1094,12 @@ static HRESULT WINAPI HTMLDocument_put_URL(IHTMLDocument2 *iface, BSTR v) TRACE("(%p)->(%s)\n", This, debugstr_w(v)); - if(!This->outer_window) { + if(!This->window || !This->window->base.outer_window) { FIXME("No window available\n"); return E_FAIL; } - return navigate_url(This->outer_window, v, This->outer_window->uri, BINDING_NAVIGATED); + return navigate_url(This->window->base.outer_window, v, This->window->base.outer_window->uri, BINDING_NAVIGATED); } static HRESULT WINAPI HTMLDocument_get_URL(IHTMLDocument2 *iface, BSTR *p) @@ -1108,7 +1108,8 @@ static HRESULT WINAPI HTMLDocument_get_URL(IHTMLDocument2 *iface, BSTR *p) TRACE("(%p)->(%p)\n", iface, p); - *p = SysAllocString(This->outer_window && This->outer_window->url ? This->outer_window->url : L"about:blank"); + *p = SysAllocString(This->window && This->window->base.outer_window && This->window->base.outer_window->url ? + This->window->base.outer_window->url : L"about:blank"); return *p ? S_OK : E_OUTOFMEMORY; } @@ -1149,12 +1150,12 @@ static HRESULT WINAPI HTMLDocument_get_domain(IHTMLDocument2 *iface, BSTR *p) return E_NOTIMPL; } - if(This->outer_window && !This->outer_window->uri) + if(This->window && This->window->base.outer_window && !This->window->base.outer_window->uri) return E_FAIL; nsAString_Init(&nsstr, NULL); nsres = nsIDOMHTMLDocument_GetDomain(This->html_document, &nsstr); - if(NS_SUCCEEDED(nsres) && This->outer_window && This->outer_window->uri) { + if(NS_SUCCEEDED(nsres) && This->window && This->window->base.outer_window && This->window->base.outer_window->uri) { const PRUnichar *str; HRESULT hres; @@ -1162,7 +1163,7 @@ static HRESULT WINAPI HTMLDocument_get_domain(IHTMLDocument2 *iface, BSTR *p) if(!*str) { TRACE("Gecko returned empty string, fallback to loaded URL.\n"); nsAString_Finish(&nsstr); - hres = IUri_GetHost(This->outer_window->uri, p); + hres = IUri_GetHost(This->window->base.outer_window->uri, p); return FAILED(hres) ? hres : S_OK; } } @@ -1177,10 +1178,10 @@ static HRESULT WINAPI HTMLDocument_put_cookie(IHTMLDocument2 *iface, BSTR v) TRACE("(%p)->(%s)\n", This, debugstr_w(v)); - if(!This->outer_window) + if(!This->window || !This->window->base.outer_window) return S_OK; - bret = InternetSetCookieExW(This->outer_window->url, NULL, v, 0, 0); + bret = InternetSetCookieExW(This->window->base.outer_window->url, NULL, v, 0, 0); if(!bret) { FIXME("InternetSetCookieExW failed: %lu\n", GetLastError()); return HRESULT_FROM_WIN32(GetLastError()); @@ -1197,13 +1198,13 @@ static HRESULT WINAPI HTMLDocument_get_cookie(IHTMLDocument2 *iface, BSTR *p) TRACE("(%p)->(%p)\n", This, p); - if(!This->outer_window) { + if(!This->window || !This->window->base.outer_window) { *p = NULL; return S_OK; } size = 0; - bret = InternetGetCookieExW(This->outer_window->url, NULL, NULL, &size, 0, NULL); + bret = InternetGetCookieExW(This->window->base.outer_window->url, NULL, NULL, &size, 0, NULL); if(!bret && GetLastError() != ERROR_INSUFFICIENT_BUFFER) { WARN("InternetGetCookieExW failed: %lu\n", GetLastError()); *p = NULL; @@ -1219,7 +1220,7 @@ static HRESULT WINAPI HTMLDocument_get_cookie(IHTMLDocument2 *iface, BSTR *p) if(!*p) return E_OUTOFMEMORY; - bret = InternetGetCookieExW(This->outer_window->url, NULL, *p, &size, 0, NULL); + bret = InternetGetCookieExW(This->window->base.outer_window->url, NULL, *p, &size, 0, NULL); if(!bret) { ERR("InternetGetCookieExW failed: %lu\n", GetLastError()); return E_FAIL; @@ -1448,7 +1449,7 @@ static HRESULT WINAPI HTMLDocument_open(IHTMLDocument2 *iface, BSTR url, VARIANT *pomWindowResult = NULL; - if(!This->outer_window) + if(!This->window || !This->window->base.outer_window) return E_FAIL; if(!This->dom_document) { @@ -1475,8 +1476,8 @@ static HRESULT WINAPI HTMLDocument_open(IHTMLDocument2 *iface, BSTR url, VARIANT if(tmp) nsISupports_Release(tmp); - *pomWindowResult = (IDispatch*)&This->outer_window->base.IHTMLWindow2_iface; - IHTMLWindow2_AddRef(&This->outer_window->base.IHTMLWindow2_iface); + *pomWindowResult = (IDispatch*)&This->window->base.outer_window->base.IHTMLWindow2_iface; + IHTMLWindow2_AddRef(&This->window->base.outer_window->base.IHTMLWindow2_iface); return S_OK; } @@ -2349,7 +2350,7 @@ static HRESULT WINAPI HTMLDocument3_get_documentElement(IHTMLDocument3 *iface, I TRACE("(%p)->(%p)\n", This, p); - if(This->outer_window && This->outer_window->readystate == READYSTATE_UNINITIALIZED) { + if(This->window && This->window->base.outer_window && This->window->base.outer_window->readystate == READYSTATE_UNINITIALIZED) { *p = NULL; return S_OK; } @@ -4876,7 +4877,7 @@ static void HTMLDocumentNode_on_advise(IUnknown *iface, cp_static_data_t *cp) { HTMLDocumentNode *This = CONTAINING_RECORD((IHTMLDocument2*)iface, HTMLDocumentNode, IHTMLDocument2_iface); - if(This->outer_window) + if(This->window && This->window->base.outer_window) update_doc_cp_events(This, cp); } @@ -6070,10 +6071,10 @@ static HRESULT HTMLDocumentNode_location_hook(DispatchEx *dispex, WORD flags, DI { HTMLDocumentNode *This = impl_from_DispatchEx(dispex); - if(!(flags & DISPATCH_PROPERTYPUT) || !This->outer_window) + if(!(flags & DISPATCH_PROPERTYPUT) || !This->window || !This->window->base.outer_window) return S_FALSE; - return IDispatchEx_InvokeEx(&This->outer_window->base.IDispatchEx_iface, DISPID_IHTMLWINDOW2_LOCATION, + return IDispatchEx_InvokeEx(&This->window->base.outer_window->base.IDispatchEx_iface, DISPID_IHTMLWINDOW2_LOCATION, 0, flags, dp, res, ei, caller); } @@ -6187,7 +6188,6 @@ static HTMLDocumentNode *alloc_doc_node(HTMLDocumentObj *doc_obj, HTMLInnerWindo doc->IDocumentRange_iface.lpVtbl = &DocumentRangeVtbl; doc->doc_obj = doc_obj; - doc->outer_window = window ? window->base.outer_window : NULL; doc->window = window; if(window) diff --git a/dlls/mshtml/htmlframe.c b/dlls/mshtml/htmlframe.c index 1a78dc3cc07..28bf42e2569 100644 --- a/dlls/mshtml/htmlframe.c +++ b/dlls/mshtml/htmlframe.c @@ -50,7 +50,7 @@ static HRESULT set_frame_doc(HTMLFrameBase *frame, nsIDOMDocument *nsdoc) window = mozwindow_to_window(mozwindow); if(!window && frame->element.node.doc->browser) { hres = create_outer_window(frame->element.node.doc->browser, mozwindow, - frame->element.node.doc->outer_window, &window); + frame->element.node.doc->window->base.outer_window, &window); /* Don't hold ref to the created window; the parent keeps ref to it */ if(SUCCEEDED(hres)) @@ -132,7 +132,7 @@ static HRESULT WINAPI HTMLFrameBase_put_src(IHTMLFrameBase *iface, BSTR v) TRACE("(%p)->(%s)\n", This, debugstr_w(v)); - if(!This->content_window || !This->element.node.doc || !This->element.node.doc->outer_window) { + if(!This->content_window || !This->element.node.doc || !This->element.node.doc->window || !This->element.node.doc->window->base.outer_window) { nsAString nsstr; nsresult nsres; @@ -150,7 +150,7 @@ static HRESULT WINAPI HTMLFrameBase_put_src(IHTMLFrameBase *iface, BSTR v) return S_OK; } - return navigate_url(This->content_window, v, This->element.node.doc->outer_window->uri, BINDING_NAVIGATED); + return navigate_url(This->content_window, v, This->element.node.doc->window->base.outer_window->uri, BINDING_NAVIGATED); } static HRESULT WINAPI HTMLFrameBase_get_src(IHTMLFrameBase *iface, BSTR *p) diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index cc42ce1738f..a9e9c93ccff 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -943,7 +943,6 @@ struct HTMLDocumentNode { nsIDocumentObserver nsIDocumentObserver_iface; ConnectionPointContainer cp_container; - HTMLOuterWindow *outer_window; HTMLInnerWindow *window; HTMLDocumentObj *doc_obj; diff --git a/dlls/mshtml/mutation.c b/dlls/mshtml/mutation.c index 7117e0f4740..66dcece301b 100644 --- a/dlls/mshtml/mutation.c +++ b/dlls/mshtml/mutation.c @@ -314,7 +314,7 @@ static nsresult run_end_load(HTMLDocumentNode *This, nsISupports *arg1, nsISuppo if(This->window == window) { window->dom_interactive_time = get_time_stamp(); - set_ready_state(This->outer_window, READYSTATE_INTERACTIVE); + set_ready_state(This->window->base.outer_window, READYSTATE_INTERACTIVE); } IHTMLWindow2_Release(&window->base.IHTMLWindow2_iface); return NS_OK; diff --git a/dlls/mshtml/nsevents.c b/dlls/mshtml/nsevents.c index 8c6631626d1..84c06f6fe0a 100644 --- a/dlls/mshtml/nsevents.c +++ b/dlls/mshtml/nsevents.c @@ -336,7 +336,7 @@ static nsresult handle_load(HTMLDocumentNode *doc, nsIDOMEvent *event) TRACE("(%p)\n", doc); - if(!doc->outer_window) + if(!doc->window || !doc->window->base.outer_window) return NS_ERROR_FAILURE; if(doc->doc_obj && doc->doc_obj->doc_node == doc) { doc_obj = doc->doc_obj; @@ -348,7 +348,7 @@ static nsresult handle_load(HTMLDocumentNode *doc, nsIDOMEvent *event) handle_docobj_load(doc_obj); doc->window->dom_complete_time = get_time_stamp(); - set_ready_state(doc->outer_window, READYSTATE_COMPLETE); + set_ready_state(doc->window->base.outer_window, READYSTATE_COMPLETE); if(doc_obj) { if(doc_obj->view_sink) @@ -358,9 +358,9 @@ static nsresult handle_load(HTMLDocumentNode *doc, nsIDOMEvent *event) update_title(doc_obj); - if(doc_obj->doc_object_service && !(doc->outer_window->load_flags & BINDING_REFRESH)) + if(doc_obj->doc_object_service && !(doc->window->base.outer_window->load_flags & BINDING_REFRESH)) IDocObjectService_FireDocumentComplete(doc_obj->doc_object_service, - &doc->outer_window->base.IHTMLWindow2_iface, 0); + &doc->window->base.outer_window->base.IHTMLWindow2_iface, 0); IUnknown_Release(doc_obj->outer_unk); } @@ -516,7 +516,7 @@ static nsIDOMEventTarget *get_default_document_target(HTMLDocumentNode *doc) nsISupports *target_iface; nsresult nsres; - target_iface = doc->window ? (nsISupports*)doc->outer_window->nswindow : (nsISupports*)doc->dom_document; + target_iface = doc->window && doc->window->base.outer_window ? (nsISupports*)doc->window->base.outer_window->nswindow : (nsISupports*)doc->dom_document; nsres = nsISupports_QueryInterface(target_iface, &IID_nsIDOMEventTarget, (void**)&target); return NS_SUCCEEDED(nsres) ? target : NULL; } diff --git a/dlls/mshtml/olecmd.c b/dlls/mshtml/olecmd.c index 76ed84341cf..650c44c9d2b 100644 --- a/dlls/mshtml/olecmd.c +++ b/dlls/mshtml/olecmd.c @@ -490,10 +490,10 @@ static HRESULT exec_refresh(HTMLDocumentNode *doc, DWORD nCmdexecopt, VARIANT *p } } - if(!doc->outer_window) + if(!doc->window || !doc->window->base.outer_window) return E_UNEXPECTED; - return reload_page(doc->outer_window); + return reload_page(doc->window->base.outer_window); } static HRESULT exec_stop(HTMLDocumentNode *doc, DWORD nCmdexecopt, VARIANT *pvaIn, VARIANT *pvaOut) diff --git a/dlls/mshtml/pluginhost.c b/dlls/mshtml/pluginhost.c index f910600de18..5f9e3588239 100644 --- a/dlls/mshtml/pluginhost.c +++ b/dlls/mshtml/pluginhost.c @@ -2227,12 +2227,12 @@ static HRESULT WINAPI PHServiceProvider_QueryService(IServiceProvider *iface, RE TRACE("(%p)->(%s %s %p)\n", This, debugstr_guid(guidService), debugstr_guid(riid), ppv); - if(!This->doc || !This->doc->outer_window) { + if(!This->doc || !This->doc->window || !This->doc->window->base.outer_window) { *ppv = NULL; return E_NOINTERFACE; } - return IServiceProvider_QueryService(&This->doc->outer_window->base.IServiceProvider_iface, + return IServiceProvider_QueryService(&This->doc->window->base.outer_window->base.IServiceProvider_iface, guidService, riid, ppv); } diff --git a/dlls/mshtml/secmgr.c b/dlls/mshtml/secmgr.c index 24c7ef2c817..ea53166f354 100644 --- a/dlls/mshtml/secmgr.c +++ b/dlls/mshtml/secmgr.c @@ -76,10 +76,10 @@ static HRESULT WINAPI InternetHostSecurityManager_ProcessUrlAction(IInternetHost TRACE("(%p)->(%ld %p %ld %p %ld %lx %lx)\n", This, dwAction, pPolicy, cbPolicy, pContext, cbContext, dwFlags, dwReserved); - if(!This->outer_window) + if(!This->window || !This->window->base.outer_window) return E_UNEXPECTED; - url = This->outer_window->url ? This->outer_window->url : L"about:blank"; + url = This->window->base.outer_window->url ? This->window->base.outer_window->url : L"about:blank"; return IInternetSecurityManager_ProcessUrlAction(get_security_manager(), url, dwAction, pPolicy, cbPolicy, pContext, cbContext, dwFlags, dwReserved); @@ -181,10 +181,10 @@ static HRESULT WINAPI InternetHostSecurityManager_QueryCustomPolicy(IInternetHos TRACE("(%p)->(%s %p %p %p %ld %lx)\n", This, debugstr_guid(guidKey), ppPolicy, pcbPolicy, pContext, cbContext, dwReserved); - if(!This->outer_window) + if(!This->window || !This->window->base.outer_window) return E_UNEXPECTED; - url = This->outer_window->url ? This->outer_window->url : L"about:blank"; + url = This->window->base.outer_window->url ? This->window->base.outer_window->url : L"about:blank"; hres = IInternetSecurityManager_QueryCustomPolicy(get_security_manager(), url, guidKey, ppPolicy, pcbPolicy, pContext, cbContext, dwReserved); -- 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 0ff93aeba5b..9bbe1ae210a 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
Sorry I forgot to mention, I still kept it in the latest push, I only got rid of the hacky storage events tests. If you really feel strongly against it and don't want to review it at all (what I mean is: to see whether it changes behavior compared to before, if I missed something, but I hope I didn't since I did it pretty thorough), then please tell me that as last confirmation because, well, waiting for nothing on my end is not really productive, especially with code freeze in less than 2 weeks. I'll still get rid of document's `outer_window` since it's broken, and I can get rid of the outer window change if you dislike it so strongly, with the risk of introducing more regressions (because it changes existing behavior, unlike my patch is supposed not to). I can't say I'd be too happy about it, given I've spent time on it (and was suggested as solution last time), but I'd like to move it forward too. That said, it was similar with postMessage being a hook, then rewrote it not to be, and now I had to make it a hook again (for the caller SP) to obtain the source, can't say I'm super thrilled about constantly rewriting what I had originally… I mean if we ever find something depending on the outer window being held, but honestly, it will be a nightmare to figure out if it does, and I'm far more worried about that, but it's pointless if it won't convince you. Anyway let me know what I should do to get this moving. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4380#note_54129
It's not my fault that I keep finding problems both with patches themselves (see XHR problem) and tests (see above and !4312 earlier). I think that you too easily ignore signs of problems with tests. Given current findings, for all we know, native may have similar problems of using non-owned references. Your claims that it's one and the only right solution is simply not backed by convincing arguments. Note that we hide a lot of complexity behind Gecko, but native complexity surely needs more complex data structures, just like Gecko does. It means that we can't blindly extrapolate `get_parentWindow` test result to all outer_window use cases. The big picture is that inner window is not the same thing as the outer window and pretending that inner window becomes another inner window after navigation (when `inner_window->outer_window->inner_window != inner_window`) is likely a sign of a problem. The fact that your message source implementation depends on it is worrying, for example.
I mean it's known to be full of bugs and security exploits, to the point of becoming a meme.
It sure is, but you can't use it as an excuse to ignore signs of problems with tests. In this particular case, I'd be very surprised if you could reproduce that crash in pure JS.
actually, can't event handlers still be called after navigation on the old doc/window?
I'd hope that you'd know the answer before signing off patches like this (or !4312 for that matter). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4380#note_54163
It's not my fault that I keep finding problems both with patches themselves (see XHR problem) and tests (see above and !4312 earlier). I think that you too easily ignore signs of problems with tests. Given current findings, for all we know, native may have similar problems of using non-owned references. Your claims that it's one and the only right solution is simply not backed by convincing arguments.
I'm not saying it's the perfect or right solution, and I doubt we care about reproducing crashes like native does (right?). It is, however, more correct than what we have, because *at least* one extra test passes. Isn't that the point? It's not like I can write a perfect Windows implementation in one patch. And of course I'd be happy to change it as long as the test still passes. This isn't like I'm refusing to change my patches in any way, as long as the tests will still pass. You haven't suggested an alternative other than just saying you don't find it worth it (despite the failing test currently).
Note that we hide a lot of complexity behind Gecko, but native complexity surely needs more complex data structures, just like Gecko does. It means that we can't blindly extrapolate `get_parentWindow` test result to all outer_window use cases.
I don't mean to extrapolate anything. I specifically made it so that all other usages of the outer window from the inner are **same as before except** for the stuff I tested. This includes get_parentWindow, storage events, and the omHistory. But that's it. What was not tested **I made sure it kept previous behavior** (and if I didn't, it was a mistake, not intentional, so I was hoping for a review on it so I can fix it, that's what I wanted…). What is so wrong with this? I'm not assuming anything here.
The big picture is that inner window is not the same thing as the outer window and pretending that inner window becomes another inner window after navigation (when `inner_window->outer_window->inner_window != inner_window`) is likely a sign of a problem. The fact that your message source implementation depends on it is worrying, for example.
Why is that so strange? Even according to [MDN](https://developer.mozilla.org/en-US/docs/Glossary/WindowProxy), the underlying "window" changes on navigation, despite the same object being used to access it (outer window -> new inner window). Isn't that the point?
I'd hope that you'd know the answer before signing off patches like this (or !4312 for that matter).
But I wasn't necessarily looking into fixing *that* (it's only after I tested them that I tried to fix it), ergo it shouldn't have changed its behavior. But if I test it, I doubt that be enough of an argument anyway, so it's a bit frustrating wasting time for no reason. EDIT: BTW please let's just ignore the storage events for now; not because it doesn't matter but because it still fixes the case without JS, and it's absolutely not anything more complex to implement *as long as* the patch with outer window exists. If it doesn't, then the code will be the same (literally) just different behavior, so I don't think that should be an argument *against* the patch at the very least (even if it's not an argument *for* the patch). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4380#note_54273
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)