[PATCH 0/4] MR4054: mshtml: HTMLWindow behavior improvements.
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> This is also necessary to prevent leaks when the inner window will hold ref to the outer. Despite the CC handling this, it's not enough, because the script engine has to be detached (it also holds a ref). Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlwindow.c | 45 +++++++++++++++++- dlls/mshtml/mshtml_private.h | 1 + dlls/mshtml/oleobj.c | 14 +++++- dlls/mshtml/tests/events.c | 92 ++++++++++++++++++++++++++++++++++++ dlls/mshtml/tests/htmldoc.c | 36 +++++++++++++- dlls/mshtml/tests/script.c | 17 +++++++ 6 files changed, 200 insertions(+), 5 deletions(-) diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index 2034ae8487a..cd888fc3e45 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -2906,7 +2906,7 @@ static HRESULT WINAPI HTMLPrivateWindow_GetAddressBarUrl(IHTMLPrivateWindow *ifa if(!url) return E_INVALIDARG; - *url = SysAllocString(This->outer_window->url); + *url = SysAllocString(This->outer_window->url ? This->outer_window->url : L"about:blank"); return S_OK; } @@ -4525,6 +4525,49 @@ HRESULT create_pending_window(HTMLOuterWindow *outer_window, nsChannelBSC *chann return S_OK; } +void set_window_uninitialized(HTMLOuterWindow *window, HTMLDocumentNode *doc_node) +{ + nsIDOMDOMImplementation *implementation; + nsIDOMDocument *nsdoc; + nsAString nsstr; + nsresult nsres; + HRESULT hres; + + window->readystate = READYSTATE_UNINITIALIZED; + set_current_uri(window, NULL); + if(window->mon) { + IMoniker_Release(window->mon); + window->mon = NULL; + } + + hres = create_pending_window(window, NULL); + if(FAILED(hres)) + return; + + nsres = nsIDOMDocument_GetImplementation(doc_node->dom_document, &implementation); + if(NS_FAILED(nsres)) + return; + + nsAString_InitDepend(&nsstr, L""); + nsres = nsIDOMDOMImplementation_CreateHTMLDocument(implementation, &nsstr, &nsdoc); + nsIDOMDOMImplementation_Release(implementation); + nsAString_Finish(&nsstr); + if(NS_FAILED(nsres)) + return; + + hres = create_document_node(nsdoc, window->browser, window->pending_window, COMPAT_MODE_QUIRKS, &window->pending_window->doc); + nsIDOMDocument_Release(nsdoc); + if(FAILED(hres)) + return; + window->pending_window->doc->doc_obj = NULL; + window->pending_window->doc->cp_container.forward_container = NULL; + + if(window->base.inner_window) + detach_inner_window(window->base.inner_window); + window->base.inner_window = window->pending_window; + window->pending_window = NULL; +} + HRESULT update_window_doc(HTMLInnerWindow *window) { HTMLOuterWindow *outer_window = window->base.outer_window; diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index 7515fbfd2cd..407e047fb9b 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -984,6 +984,7 @@ HRESULT create_document_node(nsIDOMDocument*,GeckoBrowser*,HTMLInnerWindow*, HRESULT create_doctype_node(HTMLDocumentNode*,nsIDOMNode*,HTMLDOMNode**); HRESULT create_outer_window(GeckoBrowser*,mozIDOMWindowProxy*,HTMLOuterWindow*,HTMLOuterWindow**); +void set_window_uninitialized(HTMLOuterWindow*,HTMLDocumentNode*); HRESULT update_window_doc(HTMLInnerWindow*); HTMLOuterWindow *mozwindow_to_window(const mozIDOMWindowProxy*); void get_top_window(HTMLOuterWindow*,HTMLOuterWindow**); diff --git a/dlls/mshtml/oleobj.c b/dlls/mshtml/oleobj.c index 870500ff0ab..e4d7f49e99e 100644 --- a/dlls/mshtml/oleobj.c +++ b/dlls/mshtml/oleobj.c @@ -3412,8 +3412,18 @@ static ULONG WINAPI HTMLDocumentObj_Release(IUnknown *iface) if(!ref) { if(This->doc_node) { - This->doc_node->doc_obj = NULL; - IHTMLDOMNode_Release(&This->doc_node->node.IHTMLDOMNode_iface); + HTMLDocumentNode *doc_node = This->doc_node; + + /* Protect against re-entry by grabbing it here */ + This->ref++; + set_window_uninitialized(This->window, doc_node); + + This->doc_node = NULL; + doc_node->doc_obj = NULL; + IHTMLDOMNode_Release(&doc_node->node.IHTMLDOMNode_iface); + + /* Since we grabbed it, releasing it here will take care of freeing it */ + return HTMLDocumentObj_Release(&This->IUnknown_inner); } if(This->window) IHTMLWindow2_Release(&This->window->base.IHTMLWindow2_iface); diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index f972c2a8316..a4b50b91fa6 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -5999,6 +5999,97 @@ static void test_empty_document(void) IHTMLDocument2_Release(doc); } +static void test_document_close(void) +{ + IHTMLPrivateWindow *priv_window; + IHTMLDocument2 *doc, *doc_node; + IHTMLDocument3 *doc3; + IHTMLWindow2 *window; + IHTMLElement *elem; + HRESULT hres; + BSTR bstr; + + doc = create_document_with_origin(input_doc_str); + if(!doc) + return; + + hres = IHTMLDocument2_get_parentWindow(doc, &window); + ok(hres == S_OK, "get_parentWindow failed: %08lx\n", hres); + + hres = IHTMLWindow2_get_document(window, &doc_node); + ok(hres == S_OK, "get_document failed: %08lx\n", hres); + + hres = IHTMLWindow2_QueryInterface(window, &IID_IHTMLPrivateWindow, (void**)&priv_window); + ok(hres == S_OK, "Could not get IHTMLPrivateWindow) interface: %08lx\n", hres); + hres = IHTMLPrivateWindow_GetAddressBarUrl(priv_window, &bstr); + ok(hres == S_OK, "GetAddressBarUrl failed: %08lx\n", hres); + ok(!wcscmp(bstr, L"http://winetest.example.org/"), "unexpected address bar: %s\n", wine_dbgstr_w(bstr)); + IHTMLPrivateWindow_Release(priv_window); + SysFreeString(bstr); + + elem = get_elem_id(doc_node, L"inputid"); + IHTMLElement_Release(elem); + + set_client_site(doc, FALSE); + IHTMLDocument2_Release(doc); + + hres = IHTMLWindow2_get_document(window, &doc); + ok(hres == S_OK, "get_document failed: %08lx\n", hres); + ok(doc != doc_node, "doc == doc_node\n"); + + hres = IHTMLDocument2_get_readyState(doc_node, &bstr); + ok(hres == S_OK, "get_readyState failed: %08lx\n", hres); + ok(!wcscmp(bstr, L"uninitialized"), "readyState = %s\n", wine_dbgstr_w(bstr)); + SysFreeString(bstr); + + hres = IHTMLDocument2_get_readyState(doc, &bstr); + ok(hres == S_OK, "get_readyState failed: %08lx\n", hres); + ok(!wcscmp(bstr, L"uninitialized"), "readyState = %s\n", wine_dbgstr_w(bstr)); + SysFreeString(bstr); + + hres = IHTMLWindow2_QueryInterface(window, &IID_IHTMLPrivateWindow, (void**)&priv_window); + ok(hres == S_OK, "Could not get IHTMLPrivateWindow) interface: %08lx\n", hres); + hres = IHTMLPrivateWindow_GetAddressBarUrl(priv_window, &bstr); + ok(hres == S_OK, "GetAddressBarUrl failed: %08lx\n", hres); + ok(!wcscmp(bstr, L"about:blank"), "unexpected address bar: %s\n", wine_dbgstr_w(bstr)); + IHTMLPrivateWindow_Release(priv_window); + SysFreeString(bstr); + + bstr = SysAllocString(L"inputid"); + doc3 = get_doc3_iface((IUnknown*)doc); + hres = IHTMLDocument3_getElementById(doc3, bstr, &elem); + ok(hres == S_OK, "getElementById returned: %08lx\n", hres); + ok(elem == NULL, "elem != NULL\n"); + IHTMLDocument3_Release(doc3); + SysFreeString(bstr); + + IHTMLDocument2_Release(doc_node); + IHTMLDocument2_Release(doc); + IHTMLWindow2_Release(window); + + doc = create_document(); + if(!doc) + return; + set_client_site(doc, TRUE); + + hres = IHTMLDocument2_get_parentWindow(doc, &window); + ok(hres == S_OK, "get_parentWindow failed: %08lx\n", hres); + + hres = IHTMLWindow2_get_document(window, &doc_node); + ok(hres == S_OK, "get_document failed: %08lx\n", hres); + + set_client_site(doc, FALSE); + IHTMLDocument2_Release(doc); + + hres = IHTMLWindow2_get_document(window, &doc); + ok(hres == S_OK, "get_document failed: %08lx\n", hres); + ok(doc != doc_node, "doc == doc_node\n"); + + IHTMLDocument2_Release(doc_node); + IHTMLDocument2_Release(doc); + IHTMLWindow2_Release(window); +} + static void test_storage_events(const char *doc_str) { static struct { @@ -6365,6 +6456,7 @@ START_TEST(events) } test_empty_document(); + test_document_close(); test_storage_events(empty_doc_str); test_sync_xhr_events(empty_doc_str); if(is_ie9plus) { diff --git a/dlls/mshtml/tests/htmldoc.c b/dlls/mshtml/tests/htmldoc.c index 9874540e973..4db4777dca4 100644 --- a/dlls/mshtml/tests/htmldoc.c +++ b/dlls/mshtml/tests/htmldoc.c @@ -8461,10 +8461,12 @@ static void test_doc_domain(IHTMLDocument2 *doc) static void test_HTMLDocument_http(BOOL with_wbapp) { + IHTMLDocument2 *doc, *doc_node; + IHTMLWindow2 *window; IMoniker *http_mon; - IHTMLDocument2 *doc; - ULONG ref; HRESULT hres; + ULONG ref; + BSTR bstr; trace("Testing HTMLDocument (http%s)...\n", with_wbapp ? " with IWebBrowserApp" : ""); @@ -8529,12 +8531,42 @@ static void test_HTMLDocument_http(BOOL with_wbapp) test_IsDirty(doc, S_FALSE); test_GetCurMoniker((IUnknown*)doc, NULL, prev_url, support_wbapp); + hres = IHTMLDocument2_get_parentWindow(doc, &window); + ok(hres == S_OK, "get_parentWindow failed: %08lx\n", hres); + + hres = IHTMLWindow2_get_document(window, &doc_node); + ok(hres == S_OK, "get_document failed: %08lx\n", hres); + + hres = IHTMLDocument2_get_readyState(doc_node, &bstr); + ok(hres == S_OK, "get_readyState failed: %08lx\n", hres); + todo_wine_if(support_wbapp) + ok(!wcscmp(bstr, support_wbapp ? L"interactive" : L"complete"), "readyState = %s\n", wine_dbgstr_w(bstr)); + SysFreeString(bstr); + if(view) IOleDocumentView_Release(view); view = NULL; release_document(doc); + hres = IHTMLWindow2_get_document(window, &doc); + ok(hres == S_OK, "get_document failed: %08lx\n", hres); + ok(doc != doc_node, "doc == doc_node\n"); + + hres = IHTMLDocument2_get_readyState(doc_node, &bstr); + ok(hres == S_OK, "get_readyState failed: %08lx\n", hres); + ok(!wcscmp(bstr, L"uninitialized"), "readyState = %s\n", wine_dbgstr_w(bstr)); + SysFreeString(bstr); + + hres = IHTMLDocument2_get_readyState(doc, &bstr); + ok(hres == S_OK, "get_readyState failed: %08lx\n", hres); + ok(!wcscmp(bstr, L"uninitialized"), "readyState = %s\n", wine_dbgstr_w(bstr)); + SysFreeString(bstr); + + IHTMLDocument2_Release(doc_node); + IHTMLDocument2_Release(doc); + IHTMLWindow2_Release(window); + ref = IMoniker_Release(http_mon); ok(!ref, "ref=%ld, expected 0\n", ref); } diff --git a/dlls/mshtml/tests/script.c b/dlls/mshtml/tests/script.c index dc5dac98ee7..de1fd056169 100644 --- a/dlls/mshtml/tests/script.c +++ b/dlls/mshtml/tests/script.c @@ -4525,7 +4525,10 @@ static void test_exec_script(IHTMLDocument2 *doc, const WCHAR *codew, const WCHA static void test_simple_script(void) { + IHTMLDocument2 *doc_node; + IHTMLWindow2 *window; IHTMLDocument2 *doc; + HRESULT hres; doc = create_document(); if(!doc) @@ -4596,6 +4599,12 @@ static void test_simple_script(void) if(window_dispex) IDispatchEx_Release(window_dispex); + hres = IHTMLDocument2_get_parentWindow(doc, &window); + ok(hres == S_OK, "get_parentWindow failed: %08lx\n", hres); + + hres = IHTMLWindow2_get_document(window, &doc_node); + ok(hres == S_OK, "get_document failed: %08lx\n", hres); + SET_EXPECT(SetScriptState_DISCONNECTED); SET_EXPECT(Close); SET_EXPECT(Close2); @@ -4605,6 +4614,14 @@ static void test_simple_script(void) CHECK_CALLED(SetScriptState_DISCONNECTED); CHECK_CALLED(Close); CHECK_CALLED(Close2); + + hres = IHTMLWindow2_get_document(window, &doc); + ok(hres == S_OK, "get_document failed: %08lx\n", hres); + ok(doc != doc_node, "doc == doc_node\n"); + + IHTMLDocument2_Release(doc_node); + IHTMLDocument2_Release(doc); + IHTMLWindow2_Release(window); } static void run_from_moniker(IMoniker *mon) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4054
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlwindow.c | 2 +- dlls/mshtml/navigate.c | 3 +++ dlls/mshtml/tests/events.c | 22 +++++++++++++++++++++- 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index cd888fc3e45..3fc798287bc 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -2832,7 +2832,7 @@ static HRESULT WINAPI HTMLPrivateWindow_SuperNavigate(IHTMLPrivateWindow *iface, FIXME("unimplemented flags %lx\n", flags & ~2); if(!window || !window->browser) - return E_UNEXPECTED; + return E_FAIL; if(window->browser->doc->hostui) { hres = IDocHostUIHandler_TranslateUrl(window->browser->doc->hostui, 0, url, &translated_url); diff --git a/dlls/mshtml/navigate.c b/dlls/mshtml/navigate.c index 8172abd8040..cefb6624b67 100644 --- a/dlls/mshtml/navigate.c +++ b/dlls/mshtml/navigate.c @@ -2678,6 +2678,9 @@ HRESULT navigate_url(HTMLOuterWindow *window, const WCHAR *new_url, IUri *base_u BSTR display_uri; HRESULT hres; + if(!window->browser) + return E_UNEXPECTED; + if(new_url && base_uri) hres = CoInternetCombineUrlEx(base_uri, new_url, URL_ESCAPE_SPACES_ONLY|URL_DONT_ESCAPE_EXTRA_INFO, &nav_uri, 0); diff --git a/dlls/mshtml/tests/events.c b/dlls/mshtml/tests/events.c index a4b50b91fa6..06c077d996d 100644 --- a/dlls/mshtml/tests/events.c +++ b/dlls/mshtml/tests/events.c @@ -6003,11 +6003,13 @@ static void test_document_close(void) { IHTMLPrivateWindow *priv_window; IHTMLDocument2 *doc, *doc_node; + IHTMLLocation *location; IHTMLDocument3 *doc3; IHTMLWindow2 *window; IHTMLElement *elem; + BSTR bstr, bstr2; HRESULT hres; - BSTR bstr; + VARIANT v; doc = create_document_with_origin(input_doc_str); if(!doc) @@ -6087,6 +6089,24 @@ static void test_document_close(void) IHTMLDocument2_Release(doc_node); IHTMLDocument2_Release(doc); + + bstr = SysAllocString(L"about:blank"); + hres = IHTMLWindow2_get_location(window, &location); + ok(hres == S_OK, "get_location failed: %08lx\n", hres); + hres = IHTMLLocation_put_href(location, bstr); + ok(hres == E_UNEXPECTED, "put_href returned: %08lx\n", hres); + IHTMLLocation_Release(location); + + V_VT(&v) = VT_EMPTY; + bstr2 = SysAllocString(L""); + hres = IHTMLWindow2_QueryInterface(window, &IID_IHTMLPrivateWindow, (void**)&priv_window); + ok(hres == S_OK, "Could not get IHTMLPrivateWindow) interface: %08lx\n", hres); + hres = IHTMLPrivateWindow_SuperNavigate(priv_window, bstr, bstr2, NULL, NULL, &v, &v, 0); + ok(hres == E_FAIL, "SuperNavigate returned: %08lx\n", hres); + IHTMLPrivateWindow_Release(priv_window); + SysFreeString(bstr2); + SysFreeString(bstr); + IHTMLWindow2_Release(window); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4054
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Accessing the outer window from an inner window (such as its event target) should always be valid, even if the outer window isn't currently referring to the same inner window, because it's supposed to proxy all requests to the "current" inner window. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlwindow.c | 42 ++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index 3fc798287bc..3783f246cae 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -134,12 +134,6 @@ 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); - } } static inline HTMLWindow *impl_from_IHTMLWindow2(IHTMLWindow2 *iface) @@ -3930,6 +3924,8 @@ static void HTMLWindow_traverse(DispatchEx *dispex, nsCycleCollectionTraversalCa HTMLOuterWindow *child; traverse_event_target(&This->event_target, cb); + if(This->base.outer_window) + note_cc_edge((nsISupports*)&This->base.outer_window->base.IHTMLWindow2_iface, "outer_window", cb); LIST_FOR_EACH_ENTRY(child, &This->children, HTMLOuterWindow, sibling_entry) note_cc_edge((nsISupports*)&child->base.IHTMLWindow2_iface, "child", cb); if(This->doc) @@ -3966,6 +3962,11 @@ static void HTMLWindow_unlink(DispatchEx *dispex) unlink_ref(&This->console); detach_inner_window(This); + if(This->base.outer_window) { + HTMLOuterWindow *outer_window = This->base.outer_window; + This->base.outer_window = NULL; + IHTMLWindow2_Release(&outer_window->base.IHTMLWindow2_iface); + } if(This->doc) { HTMLDocumentNode *doc = This->doc; This->doc->window = NULL; @@ -4349,16 +4350,19 @@ static nsresult NSAPI outer_window_unlink(void *p) } if(window->pending_window) { HTMLInnerWindow *pending_window = window->pending_window; - abort_window_bindings(pending_window); - pending_window->base.outer_window = NULL; window->pending_window = NULL; + detach_inner_window(pending_window); IHTMLWindow2_Release(&pending_window->base.IHTMLWindow2_iface); } set_current_mon(window, NULL, 0); set_current_uri(window, NULL); - if(window->base.inner_window) - detach_inner_window(window->base.inner_window); + if(window->base.inner_window) { + HTMLInnerWindow *inner_window = window->base.inner_window; + window->base.inner_window = NULL; + detach_inner_window(inner_window); + IHTMLWindow2_Release(&inner_window->base.IHTMLWindow2_iface); + } if(window->location) { HTMLLocation *location = window->location; window->location = NULL; @@ -4434,6 +4438,7 @@ static HRESULT create_inner_window(HTMLOuterWindow *outer_window, IMoniker *mon, window->base.outer_window = outer_window; window->base.inner_window = window; + IHTMLWindow2_AddRef(&outer_window->base.IHTMLWindow2_iface); EventTarget_Init(&window->event_target, &HTMLWindow_dispex, COMPAT_MODE_NONE); @@ -4517,7 +4522,6 @@ 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; IHTMLWindow2_Release(&outer_window->pending_window->base.IHTMLWindow2_iface); } @@ -4562,8 +4566,12 @@ void set_window_uninitialized(HTMLOuterWindow *window, HTMLDocumentNode *doc_nod window->pending_window->doc->doc_obj = NULL; window->pending_window->doc->cp_container.forward_container = NULL; - if(window->base.inner_window) - detach_inner_window(window->base.inner_window); + if(window->base.inner_window) { + HTMLInnerWindow *inner_window = window->base.inner_window; + window->base.inner_window = NULL; + detach_inner_window(inner_window); + IHTMLWindow2_Release(&inner_window->base.IHTMLWindow2_iface); + } window->base.inner_window = window->pending_window; window->pending_window = NULL; } @@ -4600,8 +4608,12 @@ HRESULT update_window_doc(HTMLInnerWindow *window) return S_OK; } - if(outer_window->base.inner_window) - detach_inner_window(outer_window->base.inner_window); + if(outer_window->base.inner_window) { + HTMLInnerWindow *inner_window = outer_window->base.inner_window; + outer_window->base.inner_window = NULL; + detach_inner_window(inner_window); + IHTMLWindow2_Release(&inner_window->base.IHTMLWindow2_iface); + } outer_window->base.inner_window = window; outer_window->pending_window = NULL; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4054
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Detached inner windows now always hold valid pointers to the outer window, until they're unlinked. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmldoc.c | 2 +- dlls/mshtml/htmlform.c | 2 +- dlls/mshtml/htmlwindow.c | 48 +++++++++++++++------------------------ dlls/mshtml/mutation.c | 4 ++-- dlls/mshtml/omnavigator.c | 2 +- dlls/mshtml/script.c | 2 +- 6 files changed, 24 insertions(+), 36 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index d347ecb93c7..8bd9aec6944 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -3622,7 +3622,7 @@ static HRESULT WINAPI HTMLDocument7_get_defaultView(IHTMLDocument7 *iface, IHTML TRACE("(%p)->(%p)\n", This, p); - if(This->window && This->window->base.outer_window) { + if(This->window) { *p = &This->window->base.outer_window->base.IHTMLWindow2_iface; IHTMLWindow2_AddRef(*p); }else { diff --git a/dlls/mshtml/htmlform.c b/dlls/mshtml/htmlform.c index 61bf686865f..921b36619da 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) this_window = doc->window->base.outer_window; } if(!this_window) { diff --git a/dlls/mshtml/htmlwindow.c b/dlls/mshtml/htmlwindow.c index 3783f246cae..b3a59dd460e 100644 --- a/dlls/mshtml/htmlwindow.c +++ b/dlls/mshtml/htmlwindow.c @@ -577,7 +577,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))) { @@ -610,7 +610,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))) { @@ -704,7 +704,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; @@ -839,7 +839,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; } @@ -946,7 +946,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 == '_') { @@ -1303,7 +1303,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); @@ -1470,7 +1470,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; @@ -2389,9 +2389,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; @@ -2825,7 +2822,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) { @@ -3634,6 +3631,7 @@ static HRESULT WINAPI WindowDispEx_GetDispID(IDispatchEx *iface, BSTR bstrName, { HTMLWindow *This = impl_from_IDispatchEx(iface); HTMLInnerWindow *window = This->inner_window; + HTMLOuterWindow *frame; HRESULT hres; TRACE("(%p)->(%s %lx %p)\n", This, debugstr_w(bstrName), grfdex, pid); @@ -3646,20 +3644,16 @@ 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; + 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; + 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) { @@ -3892,7 +3886,7 @@ static HRESULT WINAPI HTMLWindowSP_QueryService(IServiceProvider *iface, REFGUID TRACE("(%p)->(%s %s %p)\n", This, debugstr_mshtml_guid(guidService), debugstr_mshtml_guid(riid), ppv); - if(!This->outer_window || !This->outer_window->browser) + if(!This->outer_window->browser) return E_NOINTERFACE; return IServiceProvider_QueryService(&This->outer_window->browser->doc->IServiceProvider_iface, @@ -4124,9 +4118,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; @@ -4586,9 +4577,6 @@ HRESULT update_window_doc(HTMLInnerWindow *window) assert(!window->doc); - if(!outer_window) - return E_UNEXPECTED; - nsres = nsIDOMWindow_GetDocument(outer_window->nswindow, &nsdoc); if(NS_FAILED(nsres) || !nsdoc) { ERR("GetDocument failed: %08lx\n", nsres); diff --git a/dlls/mshtml/mutation.c b/dlls/mshtml/mutation.c index a1438debd6f..45ad3eec91b 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 ? get_max_compat_mode(doc->window->base.outer_window->uri) : COMPAT_MODE_IE11; if(max_compat_mode < document_mode) { @@ -896,7 +896,7 @@ static void NSAPI nsDocumentObserver_BindToDocument(nsIDocumentObserver *iface, but it is not set by default on native, and the behavior is still different. This was tested by removing all iexplore.exe values from any FeatureControl subkeys, and renaming the test executable to iexplore.exe, which changed its default compat mode in such cases. */ - if(This->window && This->window->base.outer_window && is_iexplore()) { + if(This->window && is_iexplore()) { HTMLOuterWindow *window = This->window->base.outer_window; DWORD zone; HRESULT hres; diff --git a/dlls/mshtml/omnavigator.c b/dlls/mshtml/omnavigator.c index 58cd0c78fb2..ce80fb965f0 100644 --- a/dlls/mshtml/omnavigator.c +++ b/dlls/mshtml/omnavigator.c @@ -644,7 +644,7 @@ static HRESULT WINAPI OmHistory_get_length(IOmHistory *iface, short *p) TRACE("(%p)->(%p)\n", This, p); - if(This->window && This->window->base.outer_window) + if(This->window) browser = This->window->base.outer_window->browser; *p = browser && browser->doc->travel_log diff --git a/dlls/mshtml/script.c b/dlls/mshtml/script.c index ad44a1ace41..e476ba1c426 100644 --- a/dlls/mshtml/script.c +++ b/dlls/mshtml/script.c @@ -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) return E_UNEXPECTED; *phwnd = This->window->base.outer_window->browser->doc->hwnd; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4054
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=138541 Your paranoid android. === build (build log) === error: patch failed: dlls/mshtml/htmlwindow.c:2832 error: patch failed: dlls/mshtml/htmlwindow.c:2825 Task: Patch failed to apply
Accessing the outer window from an inner window (such as its event target) should always be valid, even if the outer window isn't currently referring to the same inner window, because it's supposed to proxy all requests to the "current" inner window.
Why would we want non-current inner window to be able to initiate events? Generally previous HTML windows shouldn't be messing current one and detaching them seems like a nice way of enforcing it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4054#note_48029
On Mon Oct 9 15:39:26 2023 +0000, Jacek Caban wrote:
Accessing the outer window from an inner window (such as its event target) should always be valid, even if the outer window isn't currently referring to the same inner window, because it's supposed to proxy all requests to the "current" inner window. Why would we want non-current inner window to be able to initiate events? Generally previous HTML windows shouldn't be messing current one and detaching them seems like a nice way of enforcing it. From the point of view of a script, there is only one window, so the event target is the outer window's (which forwards to the current inner window), and we already have tests for it.
There's other places that use the inner window though, which makes it inconsistent with this, so it solves that problem. I also have other patches later that will get it in line with this, and it's also cleaner, IMO. We also don't have to deal with the potential NULL outer window everywhere (which I suspect isn't even correct, we don't have tests for it, so I guess it was added just to prevent implementation detail crashes? pretty sure native would just return outer window ref in the first place). Anyway the point I'm trying to say is that the inner window shouldn't, in practice, be "available" as a reference, so if we hold it anywhere (like we do internally), it *must* act like it's on the outer window. This patch does that. Scripts will need to obtain ref to the outer window at some point, but like I said those patches will come later. So in this case it wouldn't make a difference and this patch is just a simplification + consistency with implementation details, so we can continue using inner window internally without issue. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4054#note_48030
On Mon Oct 9 15:39:26 2023 +0000, Gabriel Ivăncescu wrote:
From the point of view of a script, there is only one window, so the event target is the outer window's (which forwards to the current inner window), and we already have tests for it. There's other places that use the inner window though, which makes it inconsistent with this, so it solves that problem. I also have other patches later that will get it in line with this, and it's also cleaner, IMO. We also don't have to deal with the potential NULL outer window everywhere (which I suspect isn't even correct, we don't have tests for it, so I guess it was added just to prevent implementation detail crashes? pretty sure native would just return outer window ref in the first place). Anyway the point I'm trying to say is that the inner window shouldn't, in practice, be "available" as a reference, so if we hold it anywhere (like we do internally), it *must* act like it's on the outer window. This patch does that. Scripts will need to obtain ref to the outer window at some point, but like I said those patches will come later. So in this case it wouldn't make a difference and this patch is just a simplification + consistency with implementation details, so we can continue using inner window internally without issue. If native doesn't have inner windows, then how can you claim how it must act? As I said, previous windows shouldn't affect current window, so I'm curious why exactly you need it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4054#note_48034
On Mon Oct 9 16:14:49 2023 +0000, Jacek Caban wrote:
If native doesn't have inner windows, then how can you claim how it must act? As I said, previous windows shouldn't affect current window, so I'm curious why exactly you need it. I meant that I have patches (and tests, currently some are todo_wine) where the window from jscript is the outer window, not inner window. Hence, they must be affecting the current window already (since they'll go through the outer window), so this patch can't be wrong in this aspect. I was just addressing your concerns about it, because eventually we'll end up there anyway.
Of course this patch is a requirement to have consistency there. I'll also get the other stuff referenced from inner window to hold ref to it now that we have the CC (e.g. navigator, constructors, etc). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4054#note_48035
On Mon Oct 9 16:24:30 2023 +0000, Gabriel Ivăncescu wrote:
I meant that I have patches (and tests, currently some are todo_wine) where the window from jscript is the outer window, not inner window. Hence, they must be affecting the current window already (since they'll go through the outer window), so this patch can't be wrong in this aspect. I was just addressing your concerns about it, because eventually we'll end up there anyway. Of course this patch is a requirement to have consistency there. I'll also get the other stuff referenced from inner window to hold ref to it now that we have the CC (e.g. navigator, constructors, etc). Again, orphaned windows should not generally affect current window, in which exact case do you think that's not true?
Scripts hosts are terminated when window changes, so it should take care of that aspects. But there are many more aspects of MSHTML than just scripts and current approach ensures that some in-progress operations can't affect it by design. Changing that would need a stronger argument, saying that we can stop releasing some references, so let's stop releasing all references for "consistency" is not really convincing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4054#note_48181
On Tue Oct 10 09:07:00 2023 +0000, Jacek Caban wrote:
Again, orphaned windows should not generally affect current window, in which exact case do you think that's not true? Scripts hosts are terminated when window changes, so it should take care of that aspects. But there are many more aspects of MSHTML than just scripts and current approach ensures that some in-progress operations can't affect it by design. Changing that would need a stronger argument, saying that we can stop releasing some references, so let's stop releasing all references for "consistency" is not really convincing. OK so let me try another perspective. What do you think about the fourth patch? Obviously, if nothing is holding the inner windows while they're detached, then it should be good to go right?
I mean, right now I know that some things can still hold the inner window, mostly because we still pass it to external callers instead of the outer window. But after that's fixed, do you think the fourth patch would be acceptable, then? Tbh I'd want it mainly because I'm interested to see if anything still holds inner window (it shouldn't, in theory). Otherwise, none of that behavior is tested and would be hard to debug if something relies on it. So for now I think I should just drop the 3rd and 4th patch and resend fourth after I have all external callers obtain the outer window. Is that fine? Who knows maybe we find out then that the 3rd is needed after all. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4054#note_48206
participants (3)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Marvin