Bug 139685 - [iOS] REGRESSION (r174642): DumpRenderTree.app test may dump result twice
Summary: [iOS] REGRESSION (r174642): DumpRenderTree.app test may dump result twice
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: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2014-12-16 11:07 PST by Daniel Bates
Modified: 2015-01-15 19:29 PST (History)
3 users (show)

See Also:

Attachments
Patch (1.93 KB, patch)
2014-12-16 11:12 PST, Daniel Bates 		no flags Details | Formatted Diff | Diff
Patch (6.39 KB, patch)
2015-01-15 17:26 PST, Daniel Bates 		no flags Details | Formatted Diff | Diff
Patch (6.42 KB, patch)
2015-01-15 17:27 PST, Daniel Bates 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2014-12-16 11:07:50 PST 
When running layout tests for iOS today I noticed that occasionally many tests in directory LayoutTests/fast/dom would fail after test running test LayoutTests/fast/dom/gc-10.html. From briefly debugging, DumpRenderTree.app is dumping the test result twice for test LayoutTests/fast/dom/gc-10.html because a new load may starts after dump() is called. This makes run-webkit-tests confused (since it expects exactly one test result per test run) and leads to run-webkit-tests using the wrong actual result in the comparison with the expected test result. Specifically, run-webkit-tests compares the actual result of test t_(i - 1) to the expected result of test t_i for i > N once it becomes confused where N := "the index of a test that starts a new load after DumpRenderTree.app calls dump()".
Comment 1 Daniel Bates 2014-12-16 11:12:06 PST 
Created attachment 243368 [details]
Patch
Comment 2 Alexey Proskuryakov 2014-12-16 11:30:21 PST 
Comment on attachment 243368 [details]
Patch

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

> Tools/ChangeLog:15
> +        Following <http://trac.webkit.org/changeset/174642>, when we receive a -didFinishLoadForFrame
> +        delegate callback DumpRenderTree.app allows the WebThread to run before dumping the output of
> +        a test. And the WebThread may load a new page during this time, which will ultimately lead to
> +        dumping the test result again. Therefore, we should only dump the test result if neither a new
> +        load began nor testRunner.waitUntilDone() was called after allowing the WebThread to run.

I'm concerned that this adds more unpredictability and flakiness. We first ask to dump results, then refuse to based on some out-of-band state, and without any guarantee that we will ultimately dump the results.

If we already decided that the test is complete, we should not make racy second guesses later.
Comment 3 Daniel Bates 2014-12-16 18:58:52 PST 
Comment on attachment 243368 [details]
Patch

This fix is incorrect.
Comment 4 Radar WebKit Bug Importer 2014-12-17 11:12:45 PST 
<rdar://problem/19281317>
Comment 5 Daniel Bates 2015-01-15 17:26:37 PST 
Created attachment 244736 [details]
Patch
Comment 6 Daniel Bates 2015-01-15 17:27:34 PST 
Created attachment 244737 [details]
Patch
Comment 7 Daniel Bates 2015-01-15 19:29:09 PST 
Comment on attachment 244737 [details]
Patch

Clearing flags on attachment: 244737

Committed r178570: <http://trac.webkit.org/changeset/178570>
Comment 8 Daniel Bates 2015-01-15 19:29:15 PST 
All reviewed patches have been landed.  Closing bug.