Attachment 368366 Details for Bug 197333 – Patch

[patch] Patch

bug-197333-20190426165100.patch (text/plain), 17.40 KB, created by Chris Dumez on 2019-04-26 16:51:01 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-04-26 16:51:01 PDT

Size: 17.40 KB

patch

obsolete

>Subversion Revision: 244685
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 0de37da93985c630f606c2b53eb6427bd831fd93..eeab3ec3fa422e7881eaec0316d05390de925fd1 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,43 @@
>+2019-04-26  Chris Dumez  <cdumez@apple.com>
>+
>+        [WKTR] Move test timeout handling to the UIProcess
>+        https://bugs.webkit.org/show_bug.cgi?id=197333
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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  Simon Fraser  <simon.fraser@apple.com>
> 
>         REGRESSION (r234330): 3 legacy-animation-engine/compositing tests are flaky failures
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
>index b8397cf6e35553b6123e5e53a062012c0d253223..31d3b970eace6e2850293e4e2e69740bf91d73ae 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
>+++ b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp
>@@ -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
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h
>index e387e3d2cc2f3c7505f40c6b968b91a50fcf74ef..26927d21a47073f2b880ed4cd168d7ac3148d990 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h
>+++ b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.h
>@@ -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 };
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp b/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
>index e9cc27c236491bbf2d34ef2a250db66dd2b11576..04eb70cf97414997d13abc5f6f5b07b591151d7f 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
>+++ b/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp
>@@ -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();
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/TestRunner.h b/Tools/WebKitTestRunner/InjectedBundle/TestRunner.h
>index c72be1804a6cfa53d9460b35fee149c349729228..9473cbecb110a135f8adf0594f5634a0e7e95b8a 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/TestRunner.h
>+++ b/Tools/WebKitTestRunner/InjectedBundle/TestRunner.h
>@@ -233,8 +233,6 @@ public:
>     void clearDidReceiveServerRedirectForProvisionalNavigation();
> 
>     bool shouldWaitUntilDone() const;
>-    void waitToDumpWatchdogTimerFired();
>-    void invalidateWaitToDumpWatchdogTimer();
> 
>     // Downloads
>     bool shouldFinishAfterDownload() const { return m_shouldFinishAfterDownload; }
>@@ -512,7 +510,6 @@ private:
>     TestRunner();
> 
>     void platformInitialize();
>-    void initializeWaitToDumpWatchdogTimerIfNeeded();
> 
>     void setDumpPixels(bool);
>     void setWaitUntilDone(bool);
>@@ -527,8 +524,6 @@ private:
>     WKRetainPtr<WKStringRef> m_userStyleSheetLocation;
>     WKRetainPtr<WKArrayRef> m_allowedHosts;
> 
>-    PlatformTimerRef m_waitToDumpWatchdogTimer;
>-
>     double m_databaseDefaultQuota { -1 };
>     double m_databaseMaxQuota { -1 };
> 
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp b/Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp
>index 10c999f45858c5b7e8911f29ec9ad83f93c826ef..81b21588b85ccd0f0693b536865449bbf493af46 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp
>+++ b/Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp
>@@ -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);
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm b/Tools/WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm
>index a3be678a520df32a22b59ecba70209556e25d4c2..0486dc866879880e101f5a9dd5c60c8095880e9e 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm
>+++ b/Tools/WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm
>@@ -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.
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp b/Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp
>index 25fbff5afab110ed7d7498097b4557ebe2cec164..793b59b8250abf9aa5531dc6cb3879f88332258d 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp
>+++ b/Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp
>@@ -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());
>diff --git a/Tools/WebKitTestRunner/InjectedBundle/wpe/TestRunnerWPE.cpp b/Tools/WebKitTestRunner/InjectedBundle/wpe/TestRunnerWPE.cpp
>index 25fbff5afab110ed7d7498097b4557ebe2cec164..793b59b8250abf9aa5531dc6cb3879f88332258d 100644
>--- a/Tools/WebKitTestRunner/InjectedBundle/wpe/TestRunnerWPE.cpp
>+++ b/Tools/WebKitTestRunner/InjectedBundle/wpe/TestRunnerWPE.cpp
>@@ -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());
>diff --git a/Tools/WebKitTestRunner/TestInvocation.cpp b/Tools/WebKitTestRunner/TestInvocation.cpp
>index 6ffab3aaa1d5c9a978d376b018d58c1ddc6bf09b..5c96e832a6d5e93f4175a6482ca9891ab3fe58ce 100644
>--- a/Tools/WebKitTestRunner/TestInvocation.cpp
>+++ b/Tools/WebKitTestRunner/TestInvocation.cpp
>@@ -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
>diff --git a/Tools/WebKitTestRunner/TestInvocation.h b/Tools/WebKitTestRunner/TestInvocation.h
>index 9d3cf2fcffffa23edd44c0800717f695bd694497..c1a8b643bb627203419d0eceaacf00af8dba5e57 100644
>--- a/Tools/WebKitTestRunner/TestInvocation.h
>+++ b/Tools/WebKitTestRunner/TestInvocation.h
>@@ -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;
>

Actions: View | Formatted Diff | Diff

Attachments on bug 197333: 368366 | 368367