Log Message
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.
Modified Paths
- trunk/Tools/ChangeLog
- trunk/Tools/WebKitTestRunner/TestController.cpp
- trunk/Tools/WebKitTestRunner/TestInvocation.cpp
- trunk/Tools/WebKitTestRunner/TestInvocation.h
- trunk/Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm
- trunk/Tools/WebKitTestRunner/ios/TestControllerIOS.mm
- trunk/Tools/WebKitTestRunner/mac/TestControllerMac.mm
Diff
Modified: trunk/Tools/ChangeLog (228982 => 228983)
--- trunk/Tools/ChangeLog 2018-02-26 02:13:09 UTC (rev 228982)
+++ trunk/Tools/ChangeLog 2018-02-26 02:18:24 UTC (rev 228983)
@@ -1,3 +1,72 @@
+2018-02-25 Alexey Proskuryakov <[email protected]>
+
+ 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-25 Ali Juma <[email protected]>
Unreviewed. Change my status to committer.
Modified: trunk/Tools/WebKitTestRunner/TestController.cpp (228982 => 228983)
--- trunk/Tools/WebKitTestRunner/TestController.cpp 2018-02-26 02:13:09 UTC (rev 228982)
+++ trunk/Tools/WebKitTestRunner/TestController.cpp 2018-02-26 02:18:24 UTC (rev 228983)
@@ -853,9 +853,6 @@
platformResetStateToConsistentValues();
- // Reset main page back to about:blank
- m_doneResetting = false;
-
m_shouldDecideNavigationPolicyAfterDelay = false;
setNavigationGesturesEnabled(false);
@@ -866,6 +863,8 @@
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;
@@ -879,9 +878,10 @@
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()
@@ -2367,7 +2367,6 @@
void TestController::platformResetStateToConsistentValues()
{
-
}
unsigned TestController::imageCountInGeneralPasteboard() const
@@ -2407,9 +2406,7 @@
ClearServiceWorkerRegistrationsCallbackContext context(*this);
WKWebsiteDataStoreRemoveAllServiceWorkerRegistrations(websiteDataStore, &context, clearServiceWorkerRegistrationsCallback);
-
- if (!context.done)
- runUntil(context.done, m_currentInvocation->shortTimeout());
+ runUntil(context.done, noTimeout);
#endif
}
@@ -2440,9 +2437,7 @@
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
}
@@ -2453,8 +2448,7 @@
ClearDOMCacheCallbackContext context(*this);
WKWebsiteDataStoreRemoveAllFetchCaches(websiteDataStore, &context, clearDOMCacheCallback);
- if (!context.done)
- runUntil(context.done, m_currentInvocation->shortTimeout());
+ runUntil(context.done, noTimeout);
#endif
}
@@ -2491,8 +2485,7 @@
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;
}
@@ -2521,8 +2514,7 @@
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;
}
@@ -2569,8 +2561,7 @@
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;
}
@@ -2579,8 +2570,7 @@
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;
}
@@ -2589,8 +2579,7 @@
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;
}
@@ -2611,8 +2600,7 @@
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;
}
@@ -2627,8 +2615,7 @@
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;
}
@@ -2691,8 +2678,7 @@
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();
}
@@ -2701,8 +2687,7 @@
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();
}
@@ -2759,8 +2744,7 @@
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();
}
@@ -2769,8 +2753,7 @@
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();
}
@@ -2779,8 +2762,7 @@
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();
}
Modified: trunk/Tools/WebKitTestRunner/TestInvocation.cpp (228982 => 228983)
--- trunk/Tools/WebKitTestRunner/TestInvocation.cpp 2018-02-26 02:13:09 UTC (rev 228982)
+++ trunk/Tools/WebKitTestRunner/TestInvocation.cpp 2018-02-26 02:18:24 UTC (rev 228983)
@@ -109,7 +109,7 @@
// 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 @@
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 @@
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 @@
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 @@
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 @@
m_gotInitialResponse = true;
m_gotFinalMessage = true;
m_error = true;
- m_errorMessage = "FAIL\n";
TestController::singleton().notifyDone();
return;
}
Modified: trunk/Tools/WebKitTestRunner/TestInvocation.h (228982 => 228983)
--- trunk/Tools/WebKitTestRunner/TestInvocation.h 2018-02-26 02:13:09 UTC (rev 228982)
+++ trunk/Tools/WebKitTestRunner/TestInvocation.h 2018-02-26 02:18:24 UTC (rev 228983)
@@ -60,7 +60,6 @@
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 @@
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 };
Modified: trunk/Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm (228982 => 228983)
--- trunk/Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm 2018-02-26 02:13:09 UTC (rev 228982)
+++ trunk/Tools/WebKitTestRunner/cocoa/TestControllerCocoa.mm 2018-02-26 02:18:24 UTC (rev 228983)
@@ -195,7 +195,7 @@
[[_WKUserContentExtensionStore defaultStore] removeContentExtensionForIdentifier:@"TestContentExtensions" completionHandler:^(NSError *error) {
doneRemoving = true;
}];
- platformRunUntil(doneRemoving, 0);
+ platformRunUntil(doneRemoving, noTimeout);
[[_WKUserContentExtensionStore defaultStore] _removeAllContentExtensions];
if (PlatformWebView* webView = mainWebView())
Modified: trunk/Tools/WebKitTestRunner/ios/TestControllerIOS.mm (228982 => 228983)
--- trunk/Tools/WebKitTestRunner/ios/TestControllerIOS.mm 2018-02-26 02:13:09 UTC (rev 228982)
+++ trunk/Tools/WebKitTestRunner/ios/TestControllerIOS.mm 2018-02-26 02:18:24 UTC (rev 228983)
@@ -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
Modified: trunk/Tools/WebKitTestRunner/mac/TestControllerMac.mm (228982 => 228983)
--- trunk/Tools/WebKitTestRunner/mac/TestControllerMac.mm 2018-02-26 02:13:09 UTC (rev 228982)
+++ trunk/Tools/WebKitTestRunner/mac/TestControllerMac.mm 2018-02-26 02:18:24 UTC (rev 228983)
@@ -149,7 +149,7 @@
NSLog(@"%@", [error helpAnchor]);
doneCompiling = true;
}];
- platformRunUntil(doneCompiling, 0);
+ platformRunUntil(doneCompiling, noTimeout);
#endif
}
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
