Bug 139525 - REGRESSION (r174642): Watchdog timer expiration reported as DumpRenderTree.app timeout instead of test failure
Summary: REGRESSION (r174642): Watchdog timer expiration reported as DumpRenderTree.ap...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad iOS 8.1
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2014-12-10 19:43 PST by Daniel Bates
Modified: 2014-12-16 10:04 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.43 KB, patch)
2014-12-10 19:46 PST, Daniel Bates
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2014-12-10 19:43:07 PST
A test that calls testRunner.waitUntilDone() and doesn't call testRunner.notifyDone() before the DumpRenderTree watchdog timer expires may be incorrectly reported as a DumpRenderTree.app timeout (i.e. the process was consider unresponsive by run-webkit-tests) instead of a test failure.
Comment 1 Daniel Bates 2014-12-10 19:46:30 PST
Created attachment 243090 [details]
Patch
Comment 2 Daniel Bates 2014-12-10 19:53:53 PST
Committed r177129: <http://trac.webkit.org/changeset/177129>
Comment 3 Alexey Proskuryakov 2014-12-15 13:47:26 PST
Comment on attachment 243090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243090&action=review

> Tools/Scripts/webkitpy/port/ios.py:89
> +            # 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

I don't think that this does what is says it does. This added code changes default timeout from 35 seconds to 80 seconds, and it changes slow test timeout from 175 seconds to 400 seconds.

It is not clear to me what the actual problem was. DumpRenderTree's timeout is 30 seconds, are you saying that 5 seconds is not enough for the result to propagate? That would be surprising.
Comment 4 Daniel Bates 2014-12-16 08:26:42 PST
Comment on attachment 243090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243090&action=review

>> Tools/Scripts/webkitpy/port/ios.py:89
>> +            return 80 * 1000
> 
> I don't think that this does what is says it does. This added code changes default timeout from 35 seconds to 80 seconds, and it changes slow test timeout from 175 seconds to 400 seconds.
> 
> It is not clear to me what the actual problem was. DumpRenderTree's timeout is 30 seconds, are you saying that 5 seconds is not enough for the result to propagate? That would be surprising.

You're right! I meant to write seconds instead of milliseconds. Yes, 5 seconds wasn't enough time for the results to propagate when running 16 to 24 instances of DumpRenderTree (why?).
Comment 5 Alexey Proskuryakov 2014-12-16 10:04:34 PST
I think that it's probably not about propagation, but about returning to event loop - DumpRenderTree detects the timeout with a watchdog timer, so the timer needs to fire in order for this to work.

I don't think that enabling a huge run-webkit-tests timeout is the right answer, as that makes tests that freeze permanently take an inordinate amount of time. Instead, I'm fixing this by having both types of timeout be reported uniformly.