Skip to content
Permalink
Browse files
Sort out timeout implementations in DRT and WKTR
https://bugs.webkit.org/show_bug.cgi?id=139671

Reviewed by Simon Fraser.

Test timeout implementation had many deficiencies, please see the bug for details.
Most notably, we shouldn't have the tool confused about timeouts vs. failures, and
[ Slow ] modifiers should work a lot better.

* DumpRenderTree/TestRunner.cpp: (TestRunner::TestRunner):
* DumpRenderTree/TestRunner.h: (TestRunner::setCustomTimeout):
* DumpRenderTree/mac/DumpRenderTree.mm: (runTest):
* DumpRenderTree/mac/TestRunnerMac.mm: (TestRunner::setWaitToDump):
DumpRenderTree already read the --timeout option from command line, and webkitpy
was already configured to pass it on Mac and iOS. Let's actually use it.
TestCommand already had the same 30 second default, so this doesn't change behavior
when DRT is ran manually without the option.
Windows DumpRenderTree will need to be fixed separately (that's easy).

* DumpRenderTree/TestRunner.cpp: (TestRunner::waitToDumpWatchdogTimerFired()):
Don't print the timeout message to stdout to match WebKitTestRunner. It would be
slightly better to use stderr in both, as this is an out of band message, but
that's a larger refactoring, and the difference is minimal in practice.

* Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
(SingleTestRunner.__init__): Ensure that script and tool timeouts are substantially
different. We want the tool to reliably detect timeouts that can be detected, and
not race with the script for that.

* Scripts/webkitpy/port/base.py: (Port.default_timeout_ms): Don't make WebKit2
timeout longer than WebKit1 one, I doubt that this is necessary. Now that the value
is honored inmore cases, that could make tests run slower.
* Scripts/webkitpy/port/driver.py:
(Driver.__init__):
(Driver.run_test):
(Driver.cmd_line):
(Driver._check_for_driver_timeout):
Detect tests that have the timeout output, and make these have the proper Timeout result.

* Scripts/webkitpy/port/ios.py: (IOSSimulatorPort.default_timeout_ms): Remove an
incorrect recent change - 80 * 1000 is 80 seconds, not 80 milliseconds.

* WebKitTestRunner/InjectedBundle/TestRunner.cpp:
(WTR::TestRunner::setCustomTimeout): Deleted.
* WebKitTestRunner/InjectedBundle/TestRunner.h:
(WTR::TestRunner::setCustomTimeout):
* WebKitTestRunner/InjectedBundle/efl/TestRunnerEfl.cpp:
(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
* WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm:
(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
* WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:
(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
* WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:
(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
Updated to use a timeout passed from UI process, which used to be ignored.

* WebKitTestRunner/TestController.cpp:
(WTR::TestController::TestController):
(WTR::TestController::runUntil):
(WTR::TestController::getCustomTimeout): Deleted.
* WebKitTestRunner/TestController.h:
Delete unused m_timeout. First, it was always 0, and second, we don't need it at all.
Changed default message timeouts to match new run-webkit-tests timeout. These don't
affect ports where timeout is passed per test (shouldn't they all be like that?).

* WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::invoke):
(WTR::TestInvocation::setCustomTimeout): Deleted.
* WebKitTestRunner/TestInvocation.h:
(WTR::TestInvocation::setCustomTimeout):
(WTR::TestInvocation::customTimeout):
Ditto.



Canonical link: https://commits.webkit.org/157571@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@177363 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
aproskuryakov committed Dec 16, 2014
1 parent 38ebe85 commit 304f45d2df7e5ba68c79ddb28dd7b386f3711163
@@ -1,3 +1,78 @@
2014-12-15 Alexey Proskuryakov <ap@apple.com>

Sort out timeout implementations in DRT and WKTR
https://bugs.webkit.org/show_bug.cgi?id=139671

Reviewed by Simon Fraser.

Test timeout implementation had many deficiencies, please see the bug for details.
Most notably, we shouldn't have the tool confused about timeouts vs. failures, and
[ Slow ] modifiers should work a lot better.

* DumpRenderTree/TestRunner.cpp: (TestRunner::TestRunner):
* DumpRenderTree/TestRunner.h: (TestRunner::setCustomTimeout):
* DumpRenderTree/mac/DumpRenderTree.mm: (runTest):
* DumpRenderTree/mac/TestRunnerMac.mm: (TestRunner::setWaitToDump):
DumpRenderTree already read the --timeout option from command line, and webkitpy
was already configured to pass it on Mac and iOS. Let's actually use it.
TestCommand already had the same 30 second default, so this doesn't change behavior
when DRT is ran manually without the option.
Windows DumpRenderTree will need to be fixed separately (that's easy).

* DumpRenderTree/TestRunner.cpp: (TestRunner::waitToDumpWatchdogTimerFired()):
Don't print the timeout message to stdout to match WebKitTestRunner. It would be
slightly better to use stderr in both, as this is an out of band message, but
that's a larger refactoring, and the difference is minimal in practice.

* Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
(SingleTestRunner.__init__): Ensure that script and tool timeouts are substantially
different. We want the tool to reliably detect timeouts that can be detected, and
not race with the script for that.

* Scripts/webkitpy/port/base.py: (Port.default_timeout_ms): Don't make WebKit2
timeout longer than WebKit1 one, I doubt that this is necessary. Now that the value
is honored inmore cases, that could make tests run slower.
* Scripts/webkitpy/port/driver.py:
(Driver.__init__):
(Driver.run_test):
(Driver.cmd_line):
(Driver._check_for_driver_timeout):
Detect tests that have the timeout output, and make these have the proper Timeout result.

* Scripts/webkitpy/port/ios.py: (IOSSimulatorPort.default_timeout_ms): Remove an
incorrect recent change - 80 * 1000 is 80 seconds, not 80 milliseconds.

* WebKitTestRunner/InjectedBundle/TestRunner.cpp:
(WTR::TestRunner::setCustomTimeout): Deleted.
* WebKitTestRunner/InjectedBundle/TestRunner.h:
(WTR::TestRunner::setCustomTimeout):
* WebKitTestRunner/InjectedBundle/efl/TestRunnerEfl.cpp:
(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
* WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm:
(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
* WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:
(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
* WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:
(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
Updated to use a timeout passed from UI process, which used to be ignored.

* WebKitTestRunner/TestController.cpp:
(WTR::TestController::TestController):
(WTR::TestController::runUntil):
(WTR::TestController::getCustomTimeout): Deleted.
* WebKitTestRunner/TestController.h:
Delete unused m_timeout. First, it was always 0, and second, we don't need it at all.
Changed default message timeouts to match new run-webkit-tests timeout. These don't
affect ports where timeout is passed per test (shouldn't they all be like that?).

* WebKitTestRunner/TestInvocation.cpp:
(WTR::TestInvocation::invoke):
(WTR::TestInvocation::setCustomTimeout): Deleted.
* WebKitTestRunner/TestInvocation.h:
(WTR::TestInvocation::setCustomTimeout):
(WTR::TestInvocation::customTimeout):
Ditto.

2014-12-16 Grzegorz Czajkowski <g.czajkowski@samsung.com>

[EFL] Add logging domain for MiniBrowser
@@ -115,6 +115,7 @@ TestRunner::TestRunner(const std::string& testURL, const std::string& expectedPi
, m_testURL(testURL)
, m_expectedPixelHash(expectedPixelHash)
, m_titleTextDirection("ltr")
, m_timeout(0)
{
}

@@ -2257,7 +2258,6 @@ void TestRunner::ignoreLegacyWebNotificationPermissionRequests()
void TestRunner::waitToDumpWatchdogTimerFired()
{
const char* message = "FAIL: Timed out waiting for notifyDone to be called\n";
fprintf(stderr, "%s", message);
fprintf(stdout, "%s", message);
notifyDone();
}
@@ -356,6 +356,8 @@ class TestRunner : public RefCounted<TestRunner> {

bool hasPendingWebNotificationClick() const { return m_hasPendingWebNotificationClick; }

void setCustomTimeout(int duration) { m_timeout = duration; }

private:
TestRunner(const std::string& testURL, const std::string& expectedPixelHash);

@@ -431,6 +433,8 @@ class TestRunner : public RefCounted<TestRunner> {
static JSClassRef getJSClass();
static JSStaticValue* staticValues();
static JSStaticFunction* staticFunctions();

int m_timeout;
};

#endif // TestRunner_h
@@ -1863,6 +1863,7 @@ static void runTest(const string& inputLine)
#endif

gTestRunner = TestRunner::create(testURL, command.expectedPixelHash);
gTestRunner->setCustomTimeout(command.timeout);
topLoadingFrame = nil;
#if !PLATFORM(IOS)
ASSERT(!draggingInfo); // the previous test should have called eventSender.mouseUp to drop!
@@ -646,8 +646,6 @@ static inline size_t indexOfSeparatorAfterDirectoryName(const std::string& direc
[[mainFrame webView] _updateActiveState];
}

static const CFTimeInterval waitToDumpWatchdogInterval = 30.0;

static void waitUntilDoneWatchdogFired(CFRunLoopTimerRef timer, void* info)
{
gTestRunner->waitToDumpWatchdogTimerFired();
@@ -656,8 +654,8 @@ static void waitUntilDoneWatchdogFired(CFRunLoopTimerRef timer, void* info)
void TestRunner::setWaitToDump(bool waitUntilDone)
{
m_waitToDump = waitUntilDone;
if (m_waitToDump && shouldSetWaitToDumpWatchdog())
setWaitToDumpWatchdog(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + waitToDumpWatchdogInterval, 0, 0, 0, waitUntilDoneWatchdogFired, NULL));
if (m_waitToDump && m_timeout && shouldSetWaitToDumpWatchdog())
setWaitToDumpWatchdog(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + m_timeout / 1000.0, 0, 0, 0, waitUntilDoneWatchdogFired, NULL));
}

int TestRunner::windowCount()
@@ -55,13 +55,19 @@ def __init__(self, port, options, results_directory, worker_name, driver, test_i
self._options = options
self._results_directory = results_directory
self._driver = driver
self._timeout = test_input.timeout
self._worker_name = worker_name
self._test_name = test_input.test_name
self._should_run_pixel_test = test_input.should_run_pixel_test
self._reference_files = test_input.reference_files
self._stop_when_done = stop_when_done

self._timeout = test_input.timeout
if self._timeout > 5000:
# Timeouts are detected by both script and tool; tool detected timeouts are
# better, because they contain partial output. Give the tool some time to
# report the timeout instead of being killed.
self._timeout = int(self._timeout) - 5000

if self._reference_files:
# Detect and report a test which has a wrong combination of expectation files.
# For example, if 'foo.html' has two expectation files, 'foo-expected.html' and
@@ -138,10 +138,6 @@ def default_pixel_tests(self):
return False

def default_timeout_ms(self):
if self.get_option('webkit_test_runner'):
# Add some more time to WebKitTestRunner because it needs to syncronise the state
# with the web process and we want to detect if there is a problem with that in the driver.
return 80 * 1000
return 35 * 1000

def driver_stop_timeout(self):
@@ -137,6 +137,8 @@ def __init__(self, port, worker_number, pixel_tests, no_timeout=False):
self._crashed_process_name = None
self._crashed_pid = None

self._driver_timed_out = False

# WebKitTestRunner can report back subprocesses that became unresponsive
# This could mean they crashed.
self._subprocess_was_unresponsive = False
@@ -172,6 +174,7 @@ def run_test(self, driver_input, stop_when_done):
start_time = time.time()
self.start(driver_input.should_run_pixel_test, driver_input.args)
test_begin_time = time.time()
self._driver_timed_out = False
self.error_from_test = str()
self.err_seen_eof = False

@@ -184,6 +187,7 @@ def run_test(self, driver_input, stop_when_done):

crashed = self.has_crashed()
timed_out = self._server_process.timed_out
driver_timed_out = self._driver_timed_out
pid = self._server_process.pid()

if stop_when_done or crashed or timed_out:
@@ -213,7 +217,7 @@ def run_test(self, driver_input, stop_when_done):

return DriverOutput(text, image, actual_image_hash, audio,
crash=crashed, test_time=time.time() - test_begin_time, measurements=self._measurements,
timeout=timed_out, error=self.error_from_test,
timeout=timed_out or driver_timed_out, error=self.error_from_test,
crashed_process_name=self._crashed_process_name,
crashed_pid=self._crashed_pid, crash_log=crash_log, pid=pid)

@@ -349,7 +353,6 @@ def cmd_line(self, pixel_tests, per_test_args):
cmd.append('--threaded')
if self._no_timeout:
cmd.append('--no-timeout')
# FIXME: We need to pass --timeout=SECONDS to WebKitTestRunner for WebKit2.

cmd.extend(self._port.get_option('additional_drt_flag', []))
cmd.extend(self._port.additional_drt_flag())
@@ -359,6 +362,10 @@ def cmd_line(self, pixel_tests, per_test_args):
cmd.append('-')
return cmd

def _check_for_driver_timeout(self, out_line):
if out_line == "FAIL: Timed out waiting for notifyDone to be called":
self._driver_timed_out = True

def _check_for_driver_crash(self, error_line):
if error_line == "#CRASHED\n":
# This is used on Windows and iOS to report that the process has crashed
@@ -82,11 +82,6 @@ def relay_path(self):
def default_timeout_ms(self):
if self.get_option('guard_malloc'):
return 350 * 1000
if not self.get_option('webkit_test_runner'):
# DumpRenderTree.app waits for the WebThread to run before dumping its output. In practice
# it seems sufficient to wait up to 80ms to ensure that the WebThread ran and hence output
# for the test is dumped.
return 80 * 1000
return super(IOSSimulatorPort, self).default_timeout_ms()

def supports_per_test_timeout(self):
@@ -55,8 +55,6 @@

namespace WTR {

const double TestRunner::waitToDumpWatchdogTimerInterval = 30;

PassRefPtr<TestRunner> TestRunner::create()
{
return adoptRef(new TestRunner);
@@ -93,6 +91,7 @@ TestRunner::TestRunner()
, m_policyDelegatePermissive(false)
, m_globalFlag(false)
, m_customFullScreenBehavior(false)
, m_timeout(30000)
, m_databaseDefaultQuota(-1)
, m_databaseMaxQuota(-1)
, m_userStyleSheetEnabled(false)
@@ -164,11 +163,6 @@ void TestRunner::notifyDone()
m_waitToDump = false;
}

void TestRunner::setCustomTimeout(int timeout)
{
m_timeout = timeout;
}

void TestRunner::addUserScript(JSStringRef source, bool runAtStart, bool allFrames)
{
WKRetainPtr<WKStringRef> sourceWK = toWK(source);
@@ -263,7 +263,7 @@ class TestRunner : public JSWrappable {

bool callShouldCloseOnWebView();

void setCustomTimeout(int duration);
void setCustomTimeout(int duration) { m_timeout = duration; }

// Work queue.
void queueBackNavigation(unsigned howFarBackward);
@@ -280,8 +280,6 @@ class TestRunner : public JSWrappable {
JSValueRef neverInlineFunction(JSValueRef theFunction);

private:
static const double waitToDumpWatchdogTimerInterval;

TestRunner();

void platformInitialize();
@@ -53,8 +53,7 @@ void TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded()
if (m_waitToDumpWatchdogTimer)
return;

m_waitToDumpWatchdogTimer = ecore_timer_loop_add(waitToDumpWatchdogTimerInterval,
waitToDumpWatchdogTimerCallback, 0);
m_waitToDumpWatchdogTimer = ecore_timer_loop_add(m_timeout / 1000.0, waitToDumpWatchdogTimerCallback, 0);
}

JSRetainPtr<JSStringRef> TestRunner::pathToLocalResource(JSStringRef url)
@@ -49,7 +49,7 @@ void TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded()
return;

m_waitToDumpWatchdogTimer.scheduleAfterDelay("[WTR] waitToDumpWatchdogTimerCallback", [this] { waitToDumpWatchdogTimerFired(); },
std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(waitToDumpWatchdogTimerInterval)));
std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(m_timeout / 1000.0)));
}

JSRetainPtr<JSStringRef> TestRunner::pathToLocalResource(JSStringRef url)
@@ -52,7 +52,8 @@ static void waitUntilDoneWatchdogTimerFired(CFRunLoopTimerRef timer, void* info)
if (m_waitToDumpWatchdogTimer)
return;

m_waitToDumpWatchdogTimer = adoptCF(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + waitToDumpWatchdogTimerInterval, 0, 0, 0, WTR::waitUntilDoneWatchdogTimerFired, NULL));
CFTimeInterval interval = m_timeout / 1000.0;
m_waitToDumpWatchdogTimer = adoptCF(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + interval, 0, 0, 0, WTR::waitUntilDoneWatchdogTimerFired, NULL));
CFRunLoopAddTimer(CFRunLoopGetCurrent(), m_waitToDumpWatchdogTimer.get(), kCFRunLoopCommonModes);
}

@@ -56,7 +56,7 @@ void TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded()
if (m_waitToDumpWatchdogTimer)
return;

m_waitToDumpWatchdogTimer = ::SetTimer(0, waitToDumpWatchdogTimerIdentifier, waitToDumpWatchdogTimerInterval * 1000, WTR::waitToDumpWatchdogTimerFired);
m_waitToDumpWatchdogTimer = ::SetTimer(0, waitToDumpWatchdogTimerIdentifier, m_timeout, WTR::waitToDumpWatchdogTimerFired);
}

JSRetainPtr<JSStringRef> TestRunner::pathToLocalResource(JSStringRef url)
@@ -70,11 +70,12 @@ const unsigned TestController::viewHeight = 600;
const unsigned TestController::w3cSVGViewWidth = 480;
const unsigned TestController::w3cSVGViewHeight = 360;

// defaultLongTimeout + defaultShortTimeout should be less than 80,
// defaultLongTimeout + defaultShortTimeout should be less than 35,
// the default timeout value of the test harness so we can detect an
// unresponsive web process.
static const double defaultLongTimeout = 60;
static const double defaultShortTimeout = 15;
// These values are only used by ports that don't have --timeout option passed to WebKitTestRunner.
static const double defaultLongTimeout = 25;
static const double defaultShortTimeout = 5;
static const double defaultNoTimeout = -1;

static WKURLRef blankURL()
@@ -110,7 +111,6 @@ TestController::TestController(int argc, const char* argv[])
, m_noTimeout(defaultNoTimeout)
, m_useWaitToDumpWatchdogTimer(true)
, m_forceNoTimeout(false)
, m_timeout(0)
, m_didPrintWebProcessCrashedMessage(false)
, m_shouldExitWhenWebProcessCrashes(true)
, m_beforeUnloadReturnValue(true)
@@ -194,11 +194,6 @@ static void decidePolicyForUserMediaPermissionRequest(WKPageRef, WKFrameRef, WKS
TestController::shared().handleUserMediaPermissionRequest(permissionRequest);
}

int TestController::getCustomTimeout()
{
return m_timeout;
}

WKPageRef TestController::createOtherPage(WKPageRef oldPage, WKURLRequestRef, WKDictionaryRef, WKEventModifiers, WKEventMouseButton, const void* clientInfo)
{
PlatformWebView* parentView = static_cast<PlatformWebView*>(const_cast<void*>(clientInfo));
@@ -979,9 +974,6 @@ void TestController::runUntil(bool& done, TimeoutDuration timeoutDuration)
case LongTimeout:
timeout = m_longTimeout;
break;
case CustomTimeout:
timeout = m_timeout;
break;
case NoTimeout:
default:
timeout = m_noTimeout;
@@ -68,14 +68,12 @@ class TestController {
bool shouldUseRemoteLayerTree() const { return m_shouldUseRemoteLayerTree; }

// Runs the run loop until `done` is true or the timeout elapses.
enum TimeoutDuration { ShortTimeout, LongTimeout, NoTimeout, CustomTimeout };
enum TimeoutDuration { ShortTimeout, LongTimeout, NoTimeout };
bool useWaitToDumpWatchdogTimer() { return m_useWaitToDumpWatchdogTimer; }
void runUntil(bool& done, TimeoutDuration);
void notifyDone();

void configureViewForTest(const TestInvocation&);

int getCustomTimeout();

bool beforeUnloadReturnValue() const { return m_beforeUnloadReturnValue; }
void setBeforeUnloadReturnValue(bool value) { m_beforeUnloadReturnValue = value; }
@@ -231,8 +229,6 @@ class TestController {
bool m_useWaitToDumpWatchdogTimer;
bool m_forceNoTimeout;

int m_timeout;

bool m_didPrintWebProcessCrashedMessage;
bool m_shouldExitWhenWebProcessCrashes;

0 comments on commit 304f45d

Please sign in to comment.