Bug 87783

Summary: [WK2] window.internals settings are not reset between tests
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Tools / TestsAssignee: Mihai Parparita <mihaip>
Status: RESOLVED FIXED    
Severity: Major CC: abarth, cshu, dglazkov, dominicc, eric, jberlin, mihaip, mrowe, ossy, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88064    
Bug Blocks: 87540    
Attachments:
Description Flags
Patch
none
reset internals in WTR mihaip: review+

Description Alexey Proskuryakov 2012-05-29 14:50:23 PDT
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
Comment 1 Mihai Parparita 2012-05-29 14:52:26 PDT
Oops, will investigate.
Comment 2 Mihai Parparita 2012-05-29 15:48:19 PDT
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.
Comment 3 Alexey Proskuryakov 2012-05-29 16:25:54 PDT
Yes, this seems specific to WebKitTestRunner as far as I can tell.
Comment 4 Mihai Parparita 2012-05-29 17:19:24 PDT
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?
Comment 5 Alexey Proskuryakov 2012-05-29 17:29:24 PDT
TestController::resetStateToConsistentValues is the function. Yes, it doesn't appear to call resetInternalsObject (whoa!)
Comment 6 Mihai Parparita 2012-05-29 17:48:16 PDT
(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)
Comment 7 Mihai Parparita 2012-05-29 17:48:34 PDT
The actual resetting code that I'm adding is:

    WKBundleFrameRef frame = WKBundlePageGetMainFrame(m_page);
    JSGlobalContextRef context = WKBundleFrameGetJavaScriptContext(frame);
    WebCoreTestSupport::resetInternalsObject(context);
Comment 8 Mihai Parparita 2012-05-30 10:57:31 PDT
Created attachment 144867 [details]
Patch
Comment 9 Mihai Parparita 2012-05-30 10:58:47 PDT
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.
Comment 10 Alexey Proskuryakov 2012-05-30 11:11:54 PDT
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.
Comment 11 Alexey Proskuryakov 2012-05-30 11:37:33 PDT
Also CC'ing Dominic, who implemented window.internals for WebKit2 in <http://trac.webkit.org/changeset/89530>, and Mark who reviewed the patch.
Comment 12 Alexey Proskuryakov 2012-05-30 16:51:51 PDT
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.
Comment 13 Alexey Proskuryakov 2012-05-30 18:17:29 PDT
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.
Comment 14 Mihai Parparita 2012-05-30 21:01:55 PDT
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.
Comment 15 Alexey Proskuryakov 2012-05-31 11:13:42 PDT
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?
Comment 16 Csaba Osztrogonác 2012-06-01 01:54:55 PDT
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
Comment 17 Mihai Parparita 2012-06-01 12:34:25 PDT
(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.