Bug 87783 – [WK2] window.internals settings are not reset between tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexey Proskuryakov]

This setting leaks between tests, causing lots of pseudo-flaky tests. E.g.:

--- /Volumes/Data/slave/lion-intel-release-tests-wk2/build/layout-test-results/fast/xpath/4XPath/Borrowed/od_20000608-expected.txt
+++ /Volumes/Data/slave/lion-intel-release-tests-wk2/build/layout-test-results/fast/xpath/4XPath/Borrowed/od_20000608-actual.txt
@@ -1,6 +1,6 @@
-PASS nodeset.snapshotLength is 0
-PASS nodeset.snapshotLength is 12
-PASS successfullyParsed is true
+CONSOLE MESSAGE: Synchronous XMLHttpRequests cannot be made in the current window context.
+CONSOLE MESSAGE: line 67: INVALID_ACCESS_ERR: DOM Exception 15: A parameter or an operation was not supported by the underlying object.
+FAIL successfullyParsed should be true (of type boolean). Was undefined (of type undefined).
 
 TEST COMPLETE

[Mihai Parparita]

Oops, will investigate.

[Mihai Parparita]

I'm not able to reproduce this when building/running with  Chromium's DRT. Does this affect any other bots besides the WebKit2 one?  I did add the resetting logic to InternalSettings::restoreTo (http://trac.webkit.org/changeset/118599/trunk/Source/WebCore/testing/InternalSettings.cpp), just like all the other settings.

[Alexey Proskuryakov]

Yes, this seems specific to WebKitTestRunner as far as I can tell.

[Mihai Parparita]

I'm not familiar with how WebKitTestRunner is set up, but it seems to be missing a call to WebCoreTestSupport::resetInternalsObject. Where is its equivalent to resetWebViewToConsistentStateBeforeTesting?

[Alexey Proskuryakov]

TestController::resetStateToConsistentValues is the function. Yes, it doesn't appear to call resetInternalsObject (whoa!)

[Mihai Parparita]

(In reply to comment #5)
> TestController::resetStateToConsistentValues is the function. Yes, it doesn't appear to call resetInternalsObject (whoa!)

As far as I can tell, the call wouldn't be right there, since this needs to happen in the web process. However, that does send a "reset" message to the process, which eventually ends up in InjectBundlePage::reset. Doing the reset there seems to make sense, but it crashes with:

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000000
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   libWebCoreTestSupport.dylib   	0x0000000106704b07 WebCore::toInternals(JSC::JSValue) + 23 (JSCell.h:195)
1   libWebCoreTestSupport.dylib   	0x0000000106712a89 WebCoreTestSupport::resetInternalsObject(OpaqueJSContext const*) + 265 (WebCoreTestSupport.cpp:55)
2   WebKitTestRunnerInjectedBundle	0x000000010655f103 WTR::InjectedBundlePage::reset() + 185
3   WebKitTestRunnerInjectedBundle	0x000000010655d637 WTR::InjectedBundle::beginTesting(OpaqueWKDictionary const*) + 491 (InjectedBundle.cpp:235)
4   WebKitTestRunnerInjectedBundle	0x000000010655d719 WTR::InjectedBundle::didReceiveMessage(OpaqueWKString const*, void const*) + 193 (WKRetainPtr.h:85)
5   com.apple.WebKit2             	0x000000010022082c WebKit::InjectedBundleClient::didReceiveMessage(WebKit::InjectedBundle*, WTF::String const&, WebKit::APIObject*) + 78 (PassRefPtr.h:51)

[Mihai Parparita]

The actual resetting code that I'm adding is:

    WKBundleFrameRef frame = WKBundlePageGetMainFrame(m_page);
    JSGlobalContextRef context = WKBundleFrameGetJavaScriptContext(frame);
    WebCoreTestSupport::resetInternalsObject(context);

[Mihai Parparita]

Created attachment 144867 [details]
Patch

[Mihai Parparita]

The patch doesn't fix the underlying issue (WebKitTestRunner not resetting internals correctly), since I'm not where to begin. However, it does fix the flakiness caused by the test that I added.

[Alexey Proskuryakov]

Can someone comment on what it would take to make WebKitTestRunner reset internals state? It's very surprising that it hasn't been implemented ages ago.

[Alexey Proskuryakov]

Also CC'ing Dominic, who implemented window.internals for WebKit2 in <http://trac.webkit.org/changeset/89530>, and Mark who reviewed the patch.

[Alexey Proskuryakov]

window.internals is an extremely poor mechanism for changing settings:

1. Changing a setting in one window.internals.settings object affects all frames in a page, but does not work across secondary windows. This is inconsistent.

2. If a test navigates to a different URL (with waitUntilDone/notifyDone), settings are restored from last document's InternalSettings instead of first one's, so changes made in first one just leak to future tests.

3. As seen in this bug, it's just completely broken in WebKit2.

[Alexey Proskuryakov]

Created attachment 144965 [details]
reset internals in WTR

Just fixes the WTR issue, not the cluster of Internals bugs.

The reason why Mihai's code was crashing is that on first iteration, reset() is called before there is an internals object injected. We just need to do that after running a test, not before.

[Mihai Parparita]

Comment on attachment 144965 [details]
reset internals in WTR

View in context: https://bugs.webkit.org/attachment.cgi?id=144965&action=review

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:354
> +    WebCoreTestSupport::resetInternalsObject(context);

FWIW, the Qt port would appear to need a DumpRenderTreeSupportQ::resetInternalsObject call, based on the injectInternalsObject callsite.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/119127>.

I'll file a new bug for the more general Internals issues. Mihai, there are good changes in your patch, would you be willing to file a new bug for those, minus test change?

[Csaba Osztrogonác]

It made 2 tests crash on Qt WK2. Could you check it, please?
The new bug report is https://bugs.webkit.org/show_bug.cgi?id=88064

[Mihai Parparita]

(In reply to comment #15)
> Mihai, there are good changes in your patch, would you be willing to file a new bug for those, minus test change?

Filed bug 88032 and added the patch.