[WKTR] Move test timeout handling to the UIProcess
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 27 Apr 2019 22:45:38 +0000 (22:45 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 27 Apr 2019 22:45:38 +0000 (22:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197333

Reviewed by Darin Adler.

Move test timeout handling in WebKitTestRunner to the UIProcess to play nicely with PSON. Previously,
we'd start the timeout timer in the InjectedBundle, which would fail to account of the time spent in
every WebContent process in the case of swapping.

Also, because of process caching, the timeout timer would sometime fire in a cached process and it
would lead to crashes when firing the timer.

* WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
(WTR::InjectedBundle::didReceiveMessageToPage):
(WTR::InjectedBundle::done):
* WebKitTestRunner/InjectedBundle/InjectedBundle.h:
(WTR::InjectedBundle::shouldDumpPixels const):
* WebKitTestRunner/InjectedBundle/TestRunner.cpp:
(WTR::TestRunner::TestRunner):
(WTR::TestRunner::waitUntilDone):
(WTR::TestRunner::setWaitUntilDone):
* WebKitTestRunner/InjectedBundle/TestRunner.h:
* WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:
* WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm:
* WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:
* WebKitTestRunner/InjectedBundle/wpe/TestRunnerWPE.cpp:
* WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::TestInvocation):
(WTR::TestInvocation::createTestSettingsDictionary):
(WTR::TestInvocation::didReceiveMessageFromInjectedBundle):
(WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):
(WTR::TestInvocation::initializeWaitToDumpWatchdogTimerIfNeeded):
(WTR::TestInvocation::invalidateWaitToDumpWatchdogTimer):
(WTR::TestInvocation::waitToDumpWatchdogTimerFired):
(WTR::TestInvocation::setWaitUntilDone):
(WTR::TestInvocation::done):
* WebKitTestRunner/TestInvocation.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@244723 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h
Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
Tools/WebKitTestRunner/InjectedBundle/TestRunner.h
Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp
Tools/WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm
Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp
Tools/WebKitTestRunner/InjectedBundle/wpe/TestRunnerWPE.cpp
Tools/WebKitTestRunner/TestInvocation.cpp
Tools/WebKitTestRunner/TestInvocation.h

index ee1596e..9c307d3 100644 (file)
@@ -1,3 +1,43 @@
+2019-04-27  Chris Dumez  <cdumez@apple.com>
+
+        [WKTR] Move test timeout handling to the UIProcess
+        https://bugs.webkit.org/show_bug.cgi?id=197333
+
+        Reviewed by Darin Adler.
+
+        Move test timeout handling in WebKitTestRunner to the UIProcess to play nicely with PSON. Previously,
+        we'd start the timeout timer in the InjectedBundle, which would fail to account of the time spent in
+        every WebContent process in the case of swapping.
+
+        Also, because of process caching, the timeout timer would sometime fire in a cached process and it
+        would lead to crashes when firing the timer.
+
+        * WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:
+        (WTR::InjectedBundle::didReceiveMessageToPage):
+        (WTR::InjectedBundle::done):
+        * WebKitTestRunner/InjectedBundle/InjectedBundle.h:
+        (WTR::InjectedBundle::shouldDumpPixels const):
+        * WebKitTestRunner/InjectedBundle/TestRunner.cpp:
+        (WTR::TestRunner::TestRunner):
+        (WTR::TestRunner::waitUntilDone):
+        (WTR::TestRunner::setWaitUntilDone):
+        * WebKitTestRunner/InjectedBundle/TestRunner.h:
+        * WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:
+        * WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm:
+        * WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:
+        * WebKitTestRunner/InjectedBundle/wpe/TestRunnerWPE.cpp:
+        * WebKitTestRunner/TestInvocation.cpp:
+        (WTR::TestInvocation::TestInvocation):
+        (WTR::TestInvocation::createTestSettingsDictionary):
+        (WTR::TestInvocation::didReceiveMessageFromInjectedBundle):
+        (WTR::TestInvocation::didReceiveSynchronousMessageFromInjectedBundle):
+        (WTR::TestInvocation::initializeWaitToDumpWatchdogTimerIfNeeded):
+        (WTR::TestInvocation::invalidateWaitToDumpWatchdogTimer):
+        (WTR::TestInvocation::waitToDumpWatchdogTimerFired):
+        (WTR::TestInvocation::setWaitUntilDone):
+        (WTR::TestInvocation::done):
+        * WebKitTestRunner/TestInvocation.h:
+
 2019-04-25  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] linkPolymorphicCall now does GC
index b8397cf..31d3b97 100644 (file)
@@ -208,9 +208,6 @@ void InjectedBundle::didReceiveMessageToPage(WKBundlePageRef page, WKStringRef m
         WKRetainPtr<WKStringRef> dumpPixelsKey = adoptWK(WKStringCreateWithUTF8CString("DumpPixels"));
         m_dumpPixels = WKBooleanGetValue(static_cast<WKBooleanRef>(WKDictionaryGetItemForKey(messageBodyDictionary, dumpPixelsKey.get())));
 
-        WKRetainPtr<WKStringRef> useWaitToDumpWatchdogTimerKey = adoptWK(WKStringCreateWithUTF8CString("UseWaitToDumpWatchdogTimer"));
-        m_useWaitToDumpWatchdogTimer = WKBooleanGetValue(static_cast<WKBooleanRef>(WKDictionaryGetItemForKey(messageBodyDictionary, useWaitToDumpWatchdogTimerKey.get())));
-
         WKRetainPtr<WKStringRef> timeoutKey = adoptWK(WKStringCreateWithUTF8CString("Timeout"));
         m_timeout = Seconds::fromMilliseconds(WKUInt64GetValue(static_cast<WKUInt64Ref>(WKDictionaryGetItemForKey(messageBodyDictionary, timeoutKey.get()))));
 
@@ -543,8 +540,6 @@ void InjectedBundle::done()
     page()->stopLoading();
     setTopLoadingFrame(0);
 
-    m_testRunner->invalidateWaitToDumpWatchdogTimer();
-
 #if HAVE(ACCESSIBILITY)
     m_accessibilityController->resetToConsistentState();
 #endif
index e387e3d..26927d2 100644 (file)
@@ -80,7 +80,6 @@ public:
     void setTopLoadingFrame(WKBundleFrameRef frame) { m_topLoadingFrame = frame; }
 
     bool shouldDumpPixels() const { return m_dumpPixels; }
-    bool useWaitToDumpWatchdogTimer() const { return m_useWaitToDumpWatchdogTimer; }
     bool dumpJSConsoleLogInStdErr() const { return m_dumpJSConsoleLogInStdErr; };
 
     void outputText(const String&);
@@ -194,7 +193,6 @@ private:
     State m_state { Idle };
 
     bool m_dumpPixels { false };
-    bool m_useWaitToDumpWatchdogTimer { true };
     bool m_useWorkQueue { false };
     bool m_pixelResultIsPending { false };
     bool m_dumpJSConsoleLogInStdErr { false };
index e9cc27c..04eb70c 100644 (file)
@@ -66,9 +66,6 @@ Ref<TestRunner> TestRunner::create()
 
 TestRunner::TestRunner()
     : m_userStyleSheetLocation(adoptWK(WKStringCreateWithUTF8CString("")))
-#if !PLATFORM(COCOA)
-    , m_waitToDumpWatchdogTimer(RunLoop::main(), this, &TestRunner::waitToDumpWatchdogTimerFired)
-#endif
 {
     platformInitialize();
 }
@@ -161,16 +158,13 @@ void TestRunner::waitUntilDone()
     RELEASE_ASSERT(injectedBundle.isTestRunning());
 
     setWaitUntilDone(true);
-    // FIXME: Watchdog timer should be moved to UI process in order to take the elapsed time in anotehr process into account.
-    if (injectedBundle.useWaitToDumpWatchdogTimer())
-        initializeWaitToDumpWatchdogTimerIfNeeded();
 }
 
 void TestRunner::setWaitUntilDone(bool value)
 {
-    WKRetainPtr<WKStringRef> messsageName = adoptWK(WKStringCreateWithUTF8CString("SetWaitUntilDone"));
+    WKRetainPtr<WKStringRef> messageName = adoptWK(WKStringCreateWithUTF8CString("SetWaitUntilDone"));
     WKRetainPtr<WKBooleanRef> messageBody = adoptWK(WKBooleanCreate(value));
-    WKBundlePostSynchronousMessage(InjectedBundle::singleton().bundle(), messsageName.get(), messageBody.get(), nullptr);
+    WKBundlePostSynchronousMessage(InjectedBundle::singleton().bundle(), messageName.get(), messageBody.get(), nullptr);
 }
 
 bool TestRunner::shouldWaitUntilDone() const
@@ -182,19 +176,6 @@ bool TestRunner::shouldWaitUntilDone() const
     return WKBooleanGetValue(adoptWK(static_cast<WKBooleanRef>(returnData)).get());
 }
 
-void TestRunner::waitToDumpWatchdogTimerFired()
-{
-    invalidateWaitToDumpWatchdogTimer();
-    auto& injectedBundle = InjectedBundle::singleton();
-#if PLATFORM(COCOA)
-    char buffer[1024];
-    snprintf(buffer, sizeof(buffer), "#PID UNRESPONSIVE - %s (pid %d)\n", getprogname(), getpid());
-    injectedBundle.outputText(buffer);
-#endif
-    injectedBundle.outputText("FAIL: Timed out waiting for notifyDone to be called\n\n");
-    injectedBundle.done();
-}
-
 void TestRunner::notifyDone()
 {
     auto& injectedBundle = InjectedBundle::singleton();
index c72be18..5ee6b09 100644 (file)
 #include <wtf/Seconds.h>
 #include <wtf/text/WTFString.h>
 
-#if PLATFORM(COCOA)
-#include <wtf/RetainPtr.h>
-#include <CoreFoundation/CFRunLoop.h>
-typedef RetainPtr<CFRunLoopTimerRef> PlatformTimerRef;
-#else
-#include <wtf/RunLoop.h>
-namespace WTR {
-class TestRunner;
-typedef RunLoop::Timer<TestRunner> PlatformTimerRef;
-}
-#endif
-
 namespace WTR {
 
 class TestRunner : public JSWrappable {
@@ -233,8 +221,6 @@ public:
     void clearDidReceiveServerRedirectForProvisionalNavigation();
 
     bool shouldWaitUntilDone() const;
-    void waitToDumpWatchdogTimerFired();
-    void invalidateWaitToDumpWatchdogTimer();
 
     // Downloads
     bool shouldFinishAfterDownload() const { return m_shouldFinishAfterDownload; }
@@ -512,7 +498,6 @@ private:
     TestRunner();
 
     void platformInitialize();
-    void initializeWaitToDumpWatchdogTimerIfNeeded();
 
     void setDumpPixels(bool);
     void setWaitUntilDone(bool);
@@ -527,8 +512,6 @@ private:
     WKRetainPtr<WKStringRef> m_userStyleSheetLocation;
     WKRetainPtr<WKArrayRef> m_allowedHosts;
 
-    PlatformTimerRef m_waitToDumpWatchdogTimer;
-
     double m_databaseDefaultQuota { -1 };
     double m_databaseMaxQuota { -1 };
 
index 10c999f..81b2158 100644 (file)
@@ -39,19 +39,6 @@ void TestRunner::platformInitialize()
 {
 }
 
-void TestRunner::invalidateWaitToDumpWatchdogTimer()
-{
-    m_waitToDumpWatchdogTimer.stop();
-}
-
-void TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded()
-{
-    if (m_waitToDumpWatchdogTimer.isActive())
-        return;
-
-    m_waitToDumpWatchdogTimer.startOneShot(m_timeout);
-}
-
 JSRetainPtr<JSStringRef> TestRunner::pathToLocalResource(JSStringRef url)
 {
     size_t urlSize = JSStringGetMaximumUTF8CStringSize(url);
index a3be678..0486dc8 100644 (file)
@@ -36,30 +36,6 @@ void TestRunner::platformInitialize()
 {
 }
 
-void TestRunner::invalidateWaitToDumpWatchdogTimer()
-{
-    if (!m_waitToDumpWatchdogTimer)
-        return;
-
-    CFRunLoopTimerInvalidate(m_waitToDumpWatchdogTimer.get());
-    m_waitToDumpWatchdogTimer = nullptr;
-}
-
-static void waitUntilDoneWatchdogTimerFired(CFRunLoopTimerRef timer, void* info)
-{
-    InjectedBundle::singleton().testRunner()->waitToDumpWatchdogTimerFired();
-}
-
-void TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded()
-{
-    if (m_waitToDumpWatchdogTimer)
-        return;
-
-    CFTimeInterval interval = m_timeout.seconds();
-    m_waitToDumpWatchdogTimer = adoptCF(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + interval, 0, 0, 0, WTR::waitUntilDoneWatchdogTimerFired, NULL));
-    CFRunLoopAddTimer(CFRunLoopGetCurrent(), m_waitToDumpWatchdogTimer.get(), kCFRunLoopCommonModes);
-}
-
 JSRetainPtr<JSStringRef> TestRunner::pathToLocalResource(JSStringRef url)
 {
     return JSStringRetain(url); // Do nothing on mac.
index 25fbff5..793b59b 100644 (file)
@@ -40,21 +40,10 @@ JSRetainPtr<JSStringRef> TestRunner::inspectorTestStubURL()
     return JSStringCreateWithUTF8CString("");
 }
 
-void TestRunner::invalidateWaitToDumpWatchdogTimer()
-{
-    m_waitToDumpWatchdogTimer.stop();
-}
-
 void TestRunner::platformInitialize()
 {
 }
 
-void TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded()
-{
-    if (!m_waitToDumpWatchdogTimer.isActive())
-        m_waitToDumpWatchdogTimer.startOneShot(m_timeout);
-}
-
 void TestRunner::installFakeHelvetica(JSStringRef configuration)
 {
     WTR::installFakeHelvetica(toWK(configuration).get());
index 25fbff5..793b59b 100644 (file)
@@ -40,21 +40,10 @@ JSRetainPtr<JSStringRef> TestRunner::inspectorTestStubURL()
     return JSStringCreateWithUTF8CString("");
 }
 
-void TestRunner::invalidateWaitToDumpWatchdogTimer()
-{
-    m_waitToDumpWatchdogTimer.stop();
-}
-
 void TestRunner::platformInitialize()
 {
 }
 
-void TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded()
-{
-    if (!m_waitToDumpWatchdogTimer.isActive())
-        m_waitToDumpWatchdogTimer.startOneShot(m_timeout);
-}
-
 void TestRunner::installFakeHelvetica(JSStringRef configuration)
 {
     WTR::installFakeHelvetica(toWK(configuration).get());
index 6ffab3a..5c96e83 100644 (file)
@@ -69,6 +69,7 @@ namespace WTR {
 TestInvocation::TestInvocation(WKURLRef url, const TestOptions& options)
     : m_options(options)
     , m_url(url)
+    , m_waitToDumpWatchdogTimer(RunLoop::main(), this, &TestInvocation::waitToDumpWatchdogTimerFired)
 {
     WKRetainPtr<WKStringRef> urlString = adoptWK(WKURLCopyString(m_url.get()));
 
@@ -138,10 +139,6 @@ WKRetainPtr<WKMutableDictionaryRef> TestInvocation::createTestSettingsDictionary
     WKRetainPtr<WKBooleanRef> dumpPixelsValue = adoptWK(WKBooleanCreate(m_dumpPixels));
     WKDictionarySetItem(beginTestMessageBody.get(), dumpPixelsKey.get(), dumpPixelsValue.get());
 
-    WKRetainPtr<WKStringRef> useWaitToDumpWatchdogTimerKey = adoptWK(WKStringCreateWithUTF8CString("UseWaitToDumpWatchdogTimer"));
-    WKRetainPtr<WKBooleanRef> useWaitToDumpWatchdogTimerValue = adoptWK(WKBooleanCreate(TestController::singleton().useWaitToDumpWatchdogTimer()));
-    WKDictionarySetItem(beginTestMessageBody.get(), useWaitToDumpWatchdogTimerKey.get(), useWaitToDumpWatchdogTimerValue.get());
-
     WKRetainPtr<WKStringRef> timeoutKey = adoptWK(WKStringCreateWithUTF8CString("Timeout"));
     WKRetainPtr<WKUInt64Ref> timeoutValue = adoptWK(WKUInt64Create(m_timeout.milliseconds()));
     WKDictionarySetItem(beginTestMessageBody.get(), timeoutKey.get(), timeoutValue.get());
@@ -357,8 +354,7 @@ void TestInvocation::didReceiveMessageFromInjectedBundle(WKStringRef messageName
         WKRetainPtr<WKStringRef> audioResultKey =  adoptWK(WKStringCreateWithUTF8CString("AudioResult"));
         m_audioResult = static_cast<WKDataRef>(WKDictionaryGetItemForKey(messageBodyDictionary, audioResultKey.get()));
 
-        m_gotFinalMessage = true;
-        TestController::singleton().notifyDone();
+        done();
         return;
     }
 
@@ -832,7 +828,7 @@ WKRetainPtr<WKTypeRef> TestInvocation::didReceiveSynchronousMessageFromInjectedB
 
     if (WKStringIsEqualToUTF8CString(messageName, "SetWaitUntilDone")) {
         ASSERT(WKGetTypeID(messageBody) == WKBooleanGetTypeID());
-        m_waitUntilDone = static_cast<unsigned char>(WKBooleanGetValue(static_cast<WKBooleanRef>(messageBody)));
+        setWaitUntilDone(static_cast<unsigned char>(WKBooleanGetValue(static_cast<WKBooleanRef>(messageBody))));
         return nullptr;
     }
     if (WKStringIsEqualToUTF8CString(messageName, "GetWaitUntilDone"))
@@ -1829,4 +1825,46 @@ void TestInvocation::performCustomMenuAction()
     WKPagePostMessageToInjectedBundle(TestController::singleton().mainWebView()->page(), messageName.get(), 0);
 }
 
+void TestInvocation::initializeWaitToDumpWatchdogTimerIfNeeded()
+{
+    if (m_waitToDumpWatchdogTimer.isActive())
+        return;
+
+    m_waitToDumpWatchdogTimer.startOneShot(m_timeout);
+}
+
+void TestInvocation::invalidateWaitToDumpWatchdogTimer()
+{
+    m_waitToDumpWatchdogTimer.stop();
+}
+
+void TestInvocation::waitToDumpWatchdogTimerFired()
+{
+    invalidateWaitToDumpWatchdogTimer();
+
+#if PLATFORM(COCOA)
+    char buffer[1024];
+    snprintf(buffer, sizeof(buffer), "#PID UNRESPONSIVE - %s (pid %d)\n", getprogname(), getpid());
+    outputText(buffer);
+#endif
+    outputText("FAIL: Timed out waiting for notifyDone to be called\n\n");
+    done();
+}
+
+void TestInvocation::setWaitUntilDone(bool waitUntilDone)
+{
+    m_waitUntilDone = waitUntilDone;
+    if (waitUntilDone && TestController::singleton().useWaitToDumpWatchdogTimer())
+        initializeWaitToDumpWatchdogTimerIfNeeded();
+}
+
+void TestInvocation::done()
+{
+    m_gotFinalMessage = true;
+    invalidateWaitToDumpWatchdogTimer();
+    RunLoop::main().dispatch([] {
+        TestController::singleton().notifyDone();
+    });
+}
+
 } // namespace WTR
index 9d3cf2f..c1a8b64 100644 (file)
@@ -33,6 +33,7 @@
 #include <WebKit/WKRetainPtr.h>
 #include <string>
 #include <wtf/Noncopyable.h>
+#include <wtf/RunLoop.h>
 #include <wtf/Seconds.h>
 #include <wtf/text/StringBuilder.h>
 
@@ -93,6 +94,13 @@ public:
 private:
     WKRetainPtr<WKMutableDictionaryRef> createTestSettingsDictionary();
 
+    void waitToDumpWatchdogTimerFired();
+    void initializeWaitToDumpWatchdogTimerIfNeeded();
+    void invalidateWaitToDumpWatchdogTimer();
+
+    void done();
+    void setWaitUntilDone(bool);
+
     void dumpResults();
     static void dump(const char* textToStdout, const char* textToStderr = 0, bool seenError = false);
     enum class SnapshotResultType { WebView, WebContents };
@@ -119,6 +127,7 @@ private:
     
     WKRetainPtr<WKURLRef> m_url;
     WTF::String m_urlString;
+    RunLoop::Timer<TestInvocation> m_waitToDumpWatchdogTimer;
 
     std::string m_expectedPixelHash;