Merge r228983 - Various crashes in WebKitTestRunner, especially when …

…system is under heavy load https://bugs.webkit.org/show_bug.cgi?id=183109 Reviewed by Tim Horton. WebKitTestRunner had many places where it sent messages to WebContent with a timeout, but it didn't handle the timeout when it did occur. Nearly all of those would result in logic errors and failing tests, and most would even result in stack corruption, as the response handler modified local variables. There is only one timeout scenario that we actually mean to handle in WKTR. That's when a test freezes after it is done (e.g. an infinite loop in beforeunload) - we don't want to blame the next test for freezing, so we silently relaunch WebContent. Everything else is cargo cult code that never worked. This patch addresses the crashes, and actually makes tests pass a lot more on an overloaded system. * WebKitTestRunner/TestController.cpp: (WTR::TestController::resetStateToConsistentValues): Moved m_doneResetting assignment to where it's actually needed, for clarity. (WTR::TestController::reattachPageToWebProcess): This function used to always hit and ignore message timeout, as m_doneResetting is only updated by navigation callback when the state is Resetting. This change makes it faster. (WTR::TestController::platformResetStateToConsistentValues): Style fix. (WTR::TestController::clearServiceWorkerRegistrations): Timing out here wasn't handled in a meaningful manner, and would even corrupt the stack. (WTR::TestController::clearDOMCache): Ditto. (WTR::TestController::clearDOMCaches): Ditto. (WTR::TestController::hasDOMCache): Ditto. (WTR::TestController::domCacheSize): Ditto. (WTR::TestController::isStatisticsPrevalentResource): Ditto. (WTR::TestController::isStatisticsRegisteredAsSubFrameUnder): Ditto. (WTR::TestController::isStatisticsRegisteredAsRedirectingTo): Ditto. (WTR::TestController::isStatisticsHasHadUserInteraction): Ditto. (WTR::TestController::isStatisticsGrandfathered): Ditto. (WTR::TestController::statisticsUpdateCookiePartitioning): Ditto. (WTR::TestController::statisticsSetShouldPartitionCookiesForHost): Ditto. (WTR::TestController::statisticsClearInMemoryAndPersistentStore): Ditto. (WTR::TestController::statisticsClearInMemoryAndPersistentStoreModifiedSinceHours): Ditto. (WTR::TestController::statisticsClearThroughWebsiteDataRemoval): Ditto. * WebKitTestRunner/TestInvocation.cpp: (WTR::TestInvocation::shortTimeout const): Made shortTimeout shorter (on a hunch). (WTR::TestInvocation::invoke): Removed a timeout waiting for initial response. There is never a logical reason for such a timeout, as we always have a new or responsive WebContent process here. (WTR::TestInvocation::dumpResults): Removed another timeout that we don't know how to properly handle. (WTR::TestInvocation::didReceiveMessageFromInjectedBundle): Removed assignment to m_errorMessage, which had no effect in this context. * WebKitTestRunner/TestInvocation.h: Removed no longer used code. * WebKitTestRunner/cocoa/TestControllerCocoa.mm: (WTR::TestController::cocoaResetStateToConsistentValues): Use a named constant for no timeout. * WebKitTestRunner/ios/TestControllerIOS.mm: (WTR::TestController::platformConfigureViewForTest): Removed a useless timeout. Not sure if timing out here would corrupt the stack or not, but there is no reason to impose arbitrary limits on individual steps of a test. * WebKitTestRunner/mac/TestControllerMac.mm: (WTR::TestController::platformConfigureViewForTest): Use a named constant for no timeout.
WebKit · Mar 5, 2018 · 3e8f20b · 3e8f20b
1 parent 0820d5c
commit 3e8f20b
Show file tree

Hide file tree

Showing 7 changed files with 95 additions and 68 deletions.
diff --git a/Tools/ChangeLog b/Tools/ChangeLog
@@ -1,3 +1,72 @@
+2018-02-25  Alexey Proskuryakov  <ap@apple.com>
+
+        Various crashes in WebKitTestRunner, especially when system is under heavy load
+        https://bugs.webkit.org/show_bug.cgi?id=183109
+
+        Reviewed by Tim Horton.
+
+        WebKitTestRunner had many places where it sent messages to WebContent with a timeout,
+        but it didn't handle the timeout when it did occur. Nearly all of those would result
+        in logic errors and failing tests, and most would even result in stack corruption,
+        as the response handler modified local variables.
+
+        There is only one timeout scenario that we actually mean to handle in WKTR. That's
+        when a test freezes after it is done (e.g. an infinite loop in beforeunload) - we don't
+        want to blame the next test for freezing, so we silently relaunch WebContent.
+        Everything else is cargo cult code that never worked.
+
+        This patch addresses the crashes, and actually makes tests pass a lot more on an
+        overloaded system.
+
+        * WebKitTestRunner/TestController.cpp:
+        (WTR::TestController::resetStateToConsistentValues): Moved m_doneResetting assignment
+        to where it's actually needed, for clarity.
+        (WTR::TestController::reattachPageToWebProcess): This function used to always hit
+        and ignore message timeout, as m_doneResetting is only updated by navigation callback
+        when the state is Resetting. This change makes it faster.
+        (WTR::TestController::platformResetStateToConsistentValues): Style fix.
+        (WTR::TestController::clearServiceWorkerRegistrations): Timing out here wasn't
+        handled in a meaningful manner, and would even corrupt the stack.
+        (WTR::TestController::clearDOMCache): Ditto.
+        (WTR::TestController::clearDOMCaches): Ditto.
+        (WTR::TestController::hasDOMCache): Ditto.
+        (WTR::TestController::domCacheSize): Ditto.
+        (WTR::TestController::isStatisticsPrevalentResource): Ditto.
+        (WTR::TestController::isStatisticsRegisteredAsSubFrameUnder): Ditto.
+        (WTR::TestController::isStatisticsRegisteredAsRedirectingTo): Ditto.
+        (WTR::TestController::isStatisticsHasHadUserInteraction): Ditto.
+        (WTR::TestController::isStatisticsGrandfathered): Ditto.
+        (WTR::TestController::statisticsUpdateCookiePartitioning): Ditto.
+        (WTR::TestController::statisticsSetShouldPartitionCookiesForHost): Ditto.
+        (WTR::TestController::statisticsClearInMemoryAndPersistentStore): Ditto.
+        (WTR::TestController::statisticsClearInMemoryAndPersistentStoreModifiedSinceHours): Ditto.
+        (WTR::TestController::statisticsClearThroughWebsiteDataRemoval): Ditto.
+
+        * WebKitTestRunner/TestInvocation.cpp:
+        (WTR::TestInvocation::shortTimeout const): Made shortTimeout shorter (on a hunch).
+        (WTR::TestInvocation::invoke): Removed a timeout waiting for initial response. There
+        is never a logical reason for such a timeout, as we always have a new or responsive
+        WebContent process here.
+        (WTR::TestInvocation::dumpResults): Removed another timeout that we don't know how to
+        properly handle.
+        (WTR::TestInvocation::didReceiveMessageFromInjectedBundle): Removed assignment to
+        m_errorMessage, which had no effect in this context.
+
+        * WebKitTestRunner/TestInvocation.h: Removed no longer used code.
+
+        * WebKitTestRunner/cocoa/TestControllerCocoa.mm:
+        (WTR::TestController::cocoaResetStateToConsistentValues): Use a named constant for
+        no timeout.
+
+        * WebKitTestRunner/ios/TestControllerIOS.mm:
+        (WTR::TestController::platformConfigureViewForTest): Removed a useless timeout.
+        Not sure if timing out here would corrupt the stack or not, but there is no reason
+        to impose arbitrary limits on individual steps of a test.
+
+        * WebKitTestRunner/mac/TestControllerMac.mm:
+        (WTR::TestController::platformConfigureViewForTest): Use a named constant for
+        no timeout.
+
 2018-02-22  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         Remove currentTime() / currentTimeMS()

diff --git a/Tools/WebKitTestRunner/TestController.cpp b/Tools/WebKitTestRunner/TestController.cpp
@@ -851,9 +851,6 @@ bool TestController::resetStateToConsistentValues(const TestOptions& options)
 
     platformResetStateToConsistentValues();
 
-    // Reset main page back to about:blank
-    m_doneResetting = false;
-
     m_shouldDecideNavigationPolicyAfterDelay = false;
 
     setNavigationGesturesEnabled(false);
@@ -864,6 +861,8 @@ bool TestController::resetStateToConsistentValues(const TestOptions& options)
 
     statisticsResetToConsistentState();
 
+    // Reset main page back to about:blank
+    m_doneResetting = false;
     WKPageLoadURL(m_mainWebView->page(), blankURL());
     runUntil(m_doneResetting, m_currentInvocation->shortTimeout());
     return m_doneResetting;
@@ -877,9 +876,10 @@ void TestController::terminateWebContentProcess()
 void TestController::reattachPageToWebProcess()
 {
     // Loading a web page is the only way to reattach an existing page to a process.
+    SetForScope<State> changeState(m_state, Resetting);
     m_doneResetting = false;
     WKPageLoadURL(m_mainWebView->page(), blankURL());
-    runUntil(m_doneResetting, m_currentInvocation->shortTimeout());
+    runUntil(m_doneResetting, noTimeout);
 }
 
 const char* TestController::webProcessName()
@@ -2325,7 +2325,6 @@ WKContextRef TestController::platformAdjustContext(WKContextRef context, WKConte
 
 void TestController::platformResetStateToConsistentValues()
 {
-
 }
 
 unsigned TestController::imageCountInGeneralPasteboard() const
@@ -2365,9 +2364,7 @@ void TestController::clearServiceWorkerRegistrations()
     ClearServiceWorkerRegistrationsCallbackContext context(*this);
 
     WKWebsiteDataStoreRemoveAllServiceWorkerRegistrations(websiteDataStore, &context, clearServiceWorkerRegistrationsCallback);
-
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
 #endif
 }
 
@@ -2398,9 +2395,7 @@ void TestController::clearDOMCache(WKStringRef origin)
 
     auto cacheOrigin = adoptWK(WKSecurityOriginCreateFromString(origin));
     WKWebsiteDataStoreRemoveFetchCacheForOrigin(websiteDataStore, cacheOrigin.get(), &context, clearDOMCacheCallback);
-
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
 #endif
 }
 
@@ -2411,8 +2406,7 @@ void TestController::clearDOMCaches()
     ClearDOMCacheCallbackContext context(*this);
 
     WKWebsiteDataStoreRemoveAllFetchCaches(websiteDataStore, &context, clearDOMCacheCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
 #endif
 }
 
@@ -2449,8 +2443,7 @@ bool TestController::hasDOMCache(WKStringRef origin)
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     FetchCacheOriginsCallbackContext context(*this, origin);
     WKWebsiteDataStoreGetFetchCacheOrigins(dataStore, &context, fetchCacheOriginsCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     return context.result;
 }
 
@@ -2479,8 +2472,7 @@ uint64_t TestController::domCacheSize(WKStringRef origin)
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     FetchCacheSizeForOriginCallbackContext context(*this);
     WKWebsiteDataStoreGetFetchCacheSizeForOrigin(dataStore, origin, &context, fetchCacheSizeForOriginCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     return context.result;
 }
 
@@ -2527,8 +2519,7 @@ bool TestController::isStatisticsPrevalentResource(WKStringRef host)
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreIsStatisticsPrevalentResource(dataStore, host, &context, resourceStatisticsBooleanResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     return context.result;
 }
 
@@ -2537,8 +2528,7 @@ bool TestController::isStatisticsRegisteredAsSubFrameUnder(WKStringRef subFrameH
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreIsStatisticsRegisteredAsSubFrameUnder(dataStore, subFrameHost, topFrameHost, &context, resourceStatisticsBooleanResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     return context.result;
 }
 
@@ -2547,8 +2537,7 @@ bool TestController::isStatisticsRegisteredAsRedirectingTo(WKStringRef hostRedir
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreIsStatisticsRegisteredAsRedirectingTo(dataStore, hostRedirectedFrom, hostRedirectedTo, &context, resourceStatisticsBooleanResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     return context.result;
 }
 
@@ -2569,8 +2558,7 @@ bool TestController::isStatisticsHasHadUserInteraction(WKStringRef host)
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreIsStatisticsHasHadUserInteraction(dataStore, host, &context, resourceStatisticsBooleanResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     return context.result;
 }
 
@@ -2585,8 +2573,7 @@ bool TestController::isStatisticsGrandfathered(WKStringRef host)
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreIsStatisticsGrandfathered(dataStore, host, &context, resourceStatisticsBooleanResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     return context.result;
 }
 
@@ -2649,8 +2636,7 @@ void TestController::statisticsUpdateCookiePartitioning()
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreStatisticsUpdateCookiePartitioning(dataStore, &context, resourceStatisticsVoidResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     m_currentInvocation->didSetPartitionOrBlockCookiesForHost();
 }
 
@@ -2659,8 +2645,7 @@ void TestController::statisticsSetShouldPartitionCookiesForHost(WKStringRef host
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreSetStatisticsShouldPartitionCookiesForHost(dataStore, host, value, &context, resourceStatisticsVoidResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     m_currentInvocation->didSetPartitionOrBlockCookiesForHost();
 }
 
@@ -2717,8 +2702,7 @@ void TestController::statisticsClearInMemoryAndPersistentStore()
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreStatisticsClearInMemoryAndPersistentStore(dataStore, &context, resourceStatisticsVoidResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     m_currentInvocation->didClearStatisticsThroughWebsiteDataRemoval();
 }
 
@@ -2727,8 +2711,7 @@ void TestController::statisticsClearInMemoryAndPersistentStoreModifiedSinceHours
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreStatisticsClearInMemoryAndPersistentStoreModifiedSinceHours(dataStore, hours, &context, resourceStatisticsVoidResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     m_currentInvocation->didClearStatisticsThroughWebsiteDataRemoval();
 }
 
@@ -2737,8 +2720,7 @@ void TestController::statisticsClearThroughWebsiteDataRemoval()
     auto* dataStore = WKContextGetWebsiteDataStore(platformContext());
     ResourceStatisticsCallbackContext context(*this);
     WKWebsiteDataStoreStatisticsClearThroughWebsiteDataRemoval(dataStore, &context, resourceStatisticsVoidResultCallback);
-    if (!context.done)
-        runUntil(context.done, m_currentInvocation->shortTimeout());
+    runUntil(context.done, noTimeout);
     m_currentInvocation->didClearStatisticsThroughWebsiteDataRemoval();
 }
 

diff --git a/Tools/WebKitTestRunner/TestInvocation.cpp b/Tools/WebKitTestRunner/TestInvocation.cpp
@@ -109,7 +109,7 @@ double TestInvocation::shortTimeout() const
     // but it currently does. There is no way to know what a normal test's timeout is, as webkitpy only passes timeouts
     // for each test individually.
     // But there shouldn't be any observable negative consequences from this.
-    return m_timeout / 1000. / 2;
+    return m_timeout / 1000. / 4;
 }
 
 bool TestInvocation::shouldLogFrameLoadDelegates() const
@@ -167,12 +167,7 @@ void TestInvocation::invoke()
 
     bool shouldOpenExternalURLs = false;
 
-    TestController::singleton().runUntil(m_gotInitialResponse, shortTimeout());
-    if (!m_gotInitialResponse) {
-        m_errorMessage = "Timed out waiting for initial response from web process\n";
-        m_webProcessIsUnresponsive = true;
-        goto end;
-    }
+    TestController::singleton().runUntil(m_gotInitialResponse, TestController::noTimeout);
     if (m_error)
         goto end;
 
@@ -190,9 +185,7 @@ void TestInvocation::invoke()
         WKInspectorClose(WKPageGetInspector(TestController::singleton().mainWebView()->page()));
 #endif // !PLATFORM(IOS)
 
-    if (m_webProcessIsUnresponsive)
-        dumpWebProcessUnresponsiveness();
-    else if (TestController::singleton().resetStateToConsistentValues(m_options))
+    if (TestController::singleton().resetStateToConsistentValues(m_options))
         return;
 
     // The process is unresponsive, so let's start a new one.
@@ -201,11 +194,6 @@ void TestInvocation::invoke()
     TestController::singleton().reattachPageToWebProcess();
 }
 
-void TestInvocation::dumpWebProcessUnresponsiveness()
-{
-    dumpWebProcessUnresponsiveness(m_errorMessage.c_str());
-}
-
 void TestInvocation::dumpWebProcessUnresponsiveness(const char* errorMessage)
 {
     fprintf(stderr, "%s", errorMessage);
@@ -271,13 +259,7 @@ void TestInvocation::dumpResults()
         else if (m_pixelResultIsPending) {
             m_gotRepaint = false;
             WKPageForceRepaint(TestController::singleton().mainWebView()->page(), this, TestInvocation::forceRepaintDoneCallback);
-            TestController::singleton().runUntil(m_gotRepaint, shortTimeout());
-            if (!m_gotRepaint) {
-                m_errorMessage = "Timed out waiting for pre-pixel dump repaint\n";
-                m_webProcessIsUnresponsive = true;
-                return;
-            }
-
+            TestController::singleton().runUntil(m_gotRepaint, TestController::noTimeout);
             dumpPixelsAndCompareWithExpected(SnapshotResultType::WebView, m_repaintRects.get());
         }
     }
@@ -325,7 +307,6 @@ void TestInvocation::didReceiveMessageFromInjectedBundle(WKStringRef messageName
         m_gotInitialResponse = true;
         m_gotFinalMessage = true;
         m_error = true;
-        m_errorMessage = "FAIL\n";
         TestController::singleton().notifyDone();
         return;
     }

diff --git a/Tools/WebKitTestRunner/TestInvocation.h b/Tools/WebKitTestRunner/TestInvocation.h
@@ -60,7 +60,6 @@ class TestInvocation : public UIScriptContextDelegate {
     void didReceiveMessageFromInjectedBundle(WKStringRef messageName, WKTypeRef messageBody);
     WKRetainPtr<WKTypeRef> didReceiveSynchronousMessageFromInjectedBundle(WKStringRef messageName, WKTypeRef messageBody);
 
-    void dumpWebProcessUnresponsiveness();
     static void dumpWebProcessUnresponsiveness(const char* errorMessage);
     void outputText(const WTF::String&);
 
@@ -119,13 +118,11 @@ class TestInvocation : public UIScriptContextDelegate {
 
     bool m_dumpPixels { false };
     bool m_pixelResultIsPending { false };
-    bool m_webProcessIsUnresponsive { false };
 
     StringBuilder m_textOutput;
     WKRetainPtr<WKDataRef> m_audioResult;
     WKRetainPtr<WKImageRef> m_pixelResult;
     WKRetainPtr<WKArrayRef> m_repaintRects;
-    std::string m_errorMessage;
 
     std::unique_ptr<UIScriptContext> m_UIScriptContext;
     UIScriptInvocationData* m_pendingUIScriptInvocationData { nullptr };

diff --git a/Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm b/Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm
@@ -195,7 +195,7 @@ void initializeWebViewConfiguration(const char* libraryPath, WKStringRef injecte
     [[_WKUserContentExtensionStore defaultStore] removeContentExtensionForIdentifier:@"TestContentExtensions" completionHandler:^(NSError *error) {
         doneRemoving = true;
     }];
-    platformRunUntil(doneRemoving, 0);
+    platformRunUntil(doneRemoving, noTimeout);
     [[_WKUserContentExtensionStore defaultStore] _removeAllContentExtensions];
 
     if (PlatformWebView* webView = mainWebView())

diff --git a/Tools/WebKitTestRunner/ios/TestControllerIOS.mm b/Tools/WebKitTestRunner/ios/TestControllerIOS.mm
@@ -119,9 +119,7 @@
             doneResizing = true;
         }];
 
-        platformRunUntil(doneResizing, 10);
-        if (!doneResizing)
-            WTFLogAlways("Timed out waiting for view resize to complete in platformConfigureViewForTest()");
+        platformRunUntil(doneResizing, noTimeout);
     }
 
     // We also pass data to InjectedBundle::beginTesting() to have it call

diff --git a/Tools/WebKitTestRunner/mac/TestControllerMac.mm b/Tools/WebKitTestRunner/mac/TestControllerMac.mm
@@ -149,7 +149,7 @@ static PlatformWindow wtr_NSApplication_keyWindow(id self, SEL _cmd)
             NSLog(@"%@", [error helpAnchor]);
         doneCompiling = true;
     }];
-    platformRunUntil(doneCompiling, 0);
+    platformRunUntil(doneCompiling, noTimeout);
 #endif
 }