Bug 143382 - http/tests/misc/DOMContentLoaded-event.html is flaky
Summary: http/tests/misc/DOMContentLoaded-event.html is flaky
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-03 12:11 PDT by Alexey Proskuryakov
Modified: 2015-04-16 23:07 PDT (History)
5 users (show)

See Also:


Attachments
proposed fix (3.30 KB, patch)
2015-04-15 14:10 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed fix (3.27 KB, patch)
2015-04-16 15:58 PDT, Alexey Proskuryakov
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (622.58 KB, application/zip)
2015-04-16 16:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (668.04 KB, application/zip)
2015-04-16 17:08 PDT, Build Bot
no flags Details
proposed patch (3.78 KB, patch)
2015-04-16 20:51 PDT, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2015-04-03 12:11:37 PDT
http/tests/misc/DOMContentLoaded-event.html is flaky on Yosemite WK2. Looks like its 10 ms timer takes a very long time to fire, so loading of a resource with 250 ms timeout finishes first.
Comment 1 Alexey Proskuryakov 2015-04-15 14:10:35 PDT
Created attachment 250860 [details]
proposed fix
Comment 2 Chris Dumez 2015-04-16 14:32:36 PDT
Comment on attachment 250860 [details]
proposed fix

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

> LayoutTests/http/tests/misc/DOMContentLoaded-event.html:29
> +                    log('FAILED: ' + (newTimestamp - timetamp) + " ms between DOMContentLoaded and load events");

Based on the if check, newTimestamp == timetamp here. So it will always be 0ms unless I am misreading.
Comment 3 Chris Dumez 2015-04-16 14:34:54 PDT
Comment on attachment 250860 [details]
proposed fix

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

>> LayoutTests/http/tests/misc/DOMContentLoaded-event.html:29
>> +                    log('FAILED: ' + (newTimestamp - timetamp) + " ms between DOMContentLoaded and load events");
> 
> Based on the if check, newTimestamp == timetamp here. So it will always be 0ms unless I am misreading.

Also typo in the variable name: timetamp -> timestamp
Comment 4 Alexey Proskuryakov 2015-04-16 15:58:23 PDT
Created attachment 250963 [details]
proposed fix
Comment 5 Chris Dumez 2015-04-16 16:05:04 PDT
Comment on attachment 250963 [details]
proposed fix

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

> LayoutTests/http/tests/misc/DOMContentLoaded-event.html:26
> +                if (timestamp != newTimestamp)

I believe we want to check that the DOMContentLoaded fires *before* the window load event. I don't believe the current check is sufficient for this purpose. newTimeStamp > timeStamp would be more correct but the issue is that timeStamp may be uninitialized if DOMContentLoaded hasn't fired yet.
How about simply have a global 'hasDOMContentFired' boolean (initially false), setting it to true in the DOMContentLoaded handler, and then having a check in the window load handler to make sure hasCOMContentFired is true?

> LayoutTests/http/tests/misc/DOMContentLoaded-event.html:36
>          <p>You should see a note about the DOMContentLoaded event being fired and bubbled, a timer being called, and then the load event being fired.</p>

This still mentions a Timer being called.
Comment 6 Build Bot 2015-04-16 16:30:03 PDT
Comment on attachment 250963 [details]
proposed fix

Attachment 250963 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6399118071037952

New failing tests:
http/tests/misc/DOMContentLoaded-event.html
Comment 7 Build Bot 2015-04-16 16:30:08 PDT
Created attachment 250976 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 8 Build Bot 2015-04-16 17:07:59 PDT
Comment on attachment 250963 [details]
proposed fix

Attachment 250963 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5594552584896512

New failing tests:
http/tests/misc/DOMContentLoaded-event.html
Comment 9 Build Bot 2015-04-16 17:08:03 PDT
Created attachment 250981 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 10 Alexey Proskuryakov 2015-04-16 20:51:38 PDT
Created attachment 250999 [details]
proposed patch

> I believe we want to check that the DOMContentLoaded fires *before* the window load event. I don't believe the current check is sufficient for this purpose.

Well, the test in the previous patch would fail (have different output) if DOMContentLoaded didn't fire, but I made it more explicit.
Comment 11 Chris Dumez 2015-04-16 21:15:49 PDT
Comment on attachment 250999 [details]
proposed patch

r=me
Comment 12 WebKit Commit Bot 2015-04-16 23:07:53 PDT
Comment on attachment 250999 [details]
proposed patch

Clearing flags on attachment: 250999

Committed r182940: <http://trac.webkit.org/changeset/182940>
Comment 13 WebKit Commit Bot 2015-04-16 23:07:59 PDT
All reviewed patches have been landed.  Closing bug.