TestRunnerBindings::Install called twice without window object being cleared |
||||||
Issue descriptionWhen running a layout-test (e.g. using --run-layout-test), it appears that TestRunnerBindings::Install is called multiple times without the window object being cleared. robertma@ created an example patch at https://chromium-review.googlesource.com/c/chromium/src/+/703594 which shows this. Calling this function multiple times with an uncleared window object currently has no adverse effects (afaik). The function does two main things: 1. Hooks up window.testRunner and window.layoutTestController to point to a TestRunnerBindings object. Doing this twice just overrides the existing bindings and I think gin clears up the old one. 2. Adds a MutationObserver listener to support ref-test wait in WPT tests. When this is done twice you do end up with two MutationObservers firing, but it is safe to call 'window.testRunner.notifyDone();' twice so no harm is done (afaik). However, we should not be calling this twice imo - it's bad practice and will cause bugs in the future. I grabbed the two call stacks from gdb and got: Breakpoint 1, test_runner::TestRunner::Install (this=0x145109759d60, frame=0x1f2eca9a1e98, view_test_runner=base::WeakPtr((uintptr_t)22338285048896)) at ../../content/shell/test_runner/test_runner.cc:1646 1646 TestRunnerBindings::Install(weak_factory_.GetWeakPtr(), view_test_runner, (rr) bt #0 test_runner::TestRunner::Install (this=0x145109759d60, frame=0x1f2eca9a1e98, view_test_runner=base::WeakPtr((uintptr_t)22338285048896)) at ../../content/shell/test_runner/test_runner.cc:1646 #1 0x00007f82d51befea in test_runner::TestRunnerForSpecificView::Install (this=0x1451098b9440, frame=0x1f2eca9a1e98) at ../../content/shell/test_runner/test_runner_for_specific_view.cc:86 #2 0x00007f82d51fe088 in test_runner::WebViewTestProxyBase::BindTo (this=0x1451099b75b0, frame=0x1f2eca9a1e98) at ../../content/shell/test_runner/web_view_test_proxy.cc:60 #3 0x00007f82d51f97ee in test_runner::WebFrameTestClient::DidClearWindowObject (this=0x145109f21da0) at ../../content/shell/test_runner/web_frame_test_client.cc:706 #4 0x00000000010530b9 in test_runner::WebFrameTestProxy<content::RenderFrameImpl, content::RenderFrameImpl::CreateParams const&>::DidClearWindowObject (this=0x1451099d9620) at ../../content/shell/test_runner/web_frame_test_proxy.h:273 #5 0x00007f82d9806451 in blink::LocalFrameClientImpl::DispatchDidClearWindowObjectInMainWorld (this=0x3714360a1a88) at ../../third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp:165 #6 0x00007f82da1414cb in blink::FrameLoader::DispatchDidClearWindowObjectInMainWorld (this=0x1f2eca9a20c0) at ../../third_party/WebKit/Source/core/loader/FrameLoader.cpp:1589 #7 0x00007f82d8f4ec2b in blink::LocalWindowProxy::Initialize (this=0x1f2eca9a2228) at ../../third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:188 #8 0x00007f82d8f50ed5 in blink::LocalWindowProxy::UpdateDocument (this=0x1f2eca9a2228) at ../../third_party/WebKit/Source/bindings/core/v8/LocalWindowProxy.cpp:397 #9 0x00007f82d8f64ef3 in blink::ScriptController::UpdateDocument (this=0x3714360a1ae8) at ../../third_party/WebKit/Source/bindings/core/v8/ScriptController.cpp:209 #10 0x00007f82d98c905a in blink::LocalDOMWindow::InstallNewDocument (this=0x1f2eca9b6fe8, Traceback (most recent call last): ... aat ../../third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:328 #0 test_runner::TestRunner::Install (this=0x145109759d60, frame=0x1f2eca9a1e98, view_test_runner=base::WeakPtr((uintptr_t)22338285048896)) at ../../content/shell/test_runner/test_runner.cc:1646 #1 0x00007f82d51befea in test_runner::TestRunnerForSpecificView::Install (this=0x1451098b9440, frame=0x1f2eca9a1e98) at ../../content/shell/test_runner/test_runner_for_specific_view.cc:86 #2 0x00007f82d51fe088 in test_runner::WebViewTestProxyBase::BindTo (this=0x1451099b75b0, frame=0x1f2eca9a1e98) at ../../content/shell/test_runner/web_view_test_proxy.cc:60 #3 0x00007f82d51f97ee in test_runner::WebFrameTestClient::DidClearWindowObject (this=0x145109f21da0) at ../../content/shell/test_runner/web_frame_test_client.cc:706 #4 0x00000000010530b9 in test_runner::WebFrameTestProxy<content::RenderFrameImpl, content::RenderFrameImpl::CreateParams const&>::DidClearWindowObject (this=0x1451099d9620) at ../../content/shell/test_runner/web_frame_test_proxy.h:273 #5 0x00007f82d9806451 in blink::LocalFrameClientImpl::DispatchDidClearWindowObjectInMainWorld (this=0x3714360a1a88) at ../../third_party/WebKit/Source/core/exported/LocalFrameClientImpl.cpp:165 #6 0x00007f82da141335 in blink::FrameLoader::DispatchDidClearDocumentOfWindowObject (this=0x1f2eca9a20c0) at ../../third_party/WebKit/Source/core/loader/FrameLoader.cpp:1577 #7 0x00007f82da119b2f in blink::DocumentLoader::InstallNewDocument (this=0x1f2eca9b5b30, Traceback (most recent call last): ... at ../../third_party/WebKit/Source/core/loader/DocumentLoader.cpp:1134 I'm not immediately sure what the correct fix is. blink::FrameLoader::DispatchDidClearDocumentOfWindowObject , which *only* the second call goes through, has the delightful comment: // We just cleared the document, not the entire window object, but for the // embedder that's close enough. Client()->DispatchDidClearWindowObjectInMainWorld() My personal feeling is that this is wrong, but there are a lot of implementors of WebFrameClient::DidClearWindowObject() that may be expecting this behavior.
,
Oct 9 2017
we also invoke this callback when we swap out the document, e.g., when navigating from about:blank to some URL, we will reuse the window, but not the document. why are you trying to execute script at that point in time?
,
Oct 9 2017
Thanks for the explanation, jochen@. I was trying to inject some JS that would execute once per document to do some experiments (related to issue 507054). And I found this example, JS shim in that method for WPT reftest-wait (which some others and I wrongly supposed it would only execute once per document). If this is not the right way to do it, where shall we execute per-document scripts from test runner? On Oct 9, 2017 08:25, "jochen via monorail" <monorail+v2.113376574@chromiu m.org> wrote:
,
Oct 9 2017
I'd recommend adding something like testRunner.doneWhenFontsLoaded() that internally would do the same as testRunner.waitUntilDone() and then when fonts are loaded, it triggers notifyDone() (without injecting any javascript)
,
Oct 9 2017
If I understand correctly, you are suggesting adding a method in the binding, and tests still have to explicitly call this JS method. Correct? We are trying not to modify the hundreds of tests, and wait for the font loading by default in reftests. Besides, we aren't 100% sure this is the right fix, which is why I said I was "experimenting" to see how it'd work on a larger scale. Lastly, if this is not the best place to inject JS, we might also want to move the existing WPT shim to a more appropriate place.
,
Oct 10 2017
"we also invoke this callback when we swap out the document"
Outside of the question of solving robertma@'s immediate issue, my question here is why do we do invoke the callback in that situation? The callback in WebFrameClient is called 'DidClearWindowObject', and it's documentation clearly states:
// The window object for the frame has been cleared of any extra
// properties that may have been set by script from the previously
// loaded document.
virtual void DidClearWindowObject() {}
Based on that I think it is bad practice to call this when the window object has not been cleared. Some future sub-class of WebFrameClient may very well presume that window has been cleared, and do something that turns out to be buggy.
,
Oct 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/407734c29e19c15f6ca5b29e78389e96dd70376d commit 407734c29e19c15f6ca5b29e78389e96dd70376d Author: Jochen Eisinger <jochen@chromium.org> Date: Thu Oct 19 09:22:17 2017 Clarify API comment for WebLocalFrame::DidClearWindowObject R=dcheng@chromium.org Bug: 772386 Change-Id: Ic352ad8d20838eab6e759f3a53f1d56d7bfb76b7 Reviewed-on: https://chromium-review.googlesource.com/720374 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Jochen Eisinger <jochen@chromium.org> Cr-Commit-Position: refs/heads/master@{#510043} [modify] https://crrev.com/407734c29e19c15f6ca5b29e78389e96dd70376d/third_party/WebKit/public/web/WebFrameClient.h
,
Nov 4 2017
From looking at the snippet for WPT, my impression is that this would be better implemented in C++. We'd need a WebMutationObserver, but I think that's in line with the Web Agents work
,
Nov 6 2017
The underlying issue is that we have too many callbacks that get called around a window creation... We need to clean them up.
,
Dec 15 2017
robertma@, looks like there's a connection to issue 507054 . Does this block that? Please move back to untriaged if you're not likely to ever look at this.
,
Dec 15 2017
This issue (whether it's just a documentation error, or some real issue on callbacks as comment 9 suggests) is accidentally found when investigating issue 507054 , but it's not really blocking issue 507054 . I'm not likely to work on this.
,
Dec 17
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by robertma@chromium.org
, Oct 6 2017