Bug 158019 – Fix ResourceTiming XHR flakiness

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yoav Weiss]

Fix ResourceTiming XHR tests flakiness

[Yoav Weiss]

Created attachment 279638 [details]
Patch

[Yoav Weiss]

This should fix the test flakiness observed in https://bugs.webkit.org/show_bug.cgi?id=157816

It also removes the XHR-specific treatment that I added in https://bugs.webkit.org/show_bug.cgi?id=157669 as it is not required.

It seems like the ResourceTiming entries are added in CachedResourceLoader::loadDone only after the onload event for the XHR is running. I've opened a related spec issue to see what the correct behavior here should be: https://github.com/w3c/resource-timing/issues/55

[Build Bot]

Comment on attachment 279638 [details]
Patch

Attachment 279638 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1373989

New failing tests:
http/tests/performance/performance-resource-timing-xhr-single-entry.html

[Build Bot]

Created attachment 279640 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279638 [details]
Patch

Attachment 279638 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1374049

New failing tests:
http/tests/performance/performance-resource-timing-xhr-single-entry.html

[Build Bot]

Created attachment 279641 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279638 [details]
Patch

Attachment 279638 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1374057

New failing tests:
http/tests/performance/performance-resource-timing-xhr-single-entry.html

[Build Bot]

Created attachment 279642 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 279638 [details]
Patch

Attachment 279638 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1374054

New failing tests:
http/tests/performance/performance-resource-timing-xhr-single-entry.html

[Build Bot]

Created attachment 279643 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Yoav Weiss]

Created attachment 279645 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 279645 [details]
Patch

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

> LayoutTests/http/tests/performance/performance-resource-timing-cached-entries.html:35
> +            setTimeout(runTest, 10);

A timeout of 10 ms is never right in regression tests. A process can be easily suspended for much longer. It's especially expected on GuardMalloc and ASan tests, but even release build tests have a lot of competition for resources.

[Yoav Weiss]

(In reply to comment #12)
> Comment on attachment 279645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279645&action=review
> 
> > LayoutTests/http/tests/performance/performance-resource-timing-cached-entries.html:35
> > +            setTimeout(runTest, 10);
> 
> A timeout of 10 ms is never right in regression tests. A process can be
> easily suspended for much longer. It's especially expected on GuardMalloc
> and ASan tests, but even release build tests have a lot of competition for
> resources.

Any timeout would do here, it's just that I need for the test to run after the XHR's load event is over. So if the process will get suspended for a longer period, that's not an issue as long as the test's timeout is not exceeded. Let me know if a different timeout value (or a different method to run the test after the onload is over) is preferred.

[Alex Christensen]

Comment on attachment 279645 [details]
Patch

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

Would it work with a timeout of 0?  I think a better approach would be to create the xhr after you're done with the first runTest call to make sure it will never start before the first one.

> LayoutTests/http/tests/performance/performance-resource-timing-xhr-single-entry.html:35
> +            setTimeout(runTest, 10);

Why do you call runTest twice but only expect it to be run once?

[Yoav Weiss]

(In reply to comment #14)
> Comment on attachment 279645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279645&action=review
> 
> Would it work with a timeout of 0?

I believe it would.

> I think a better approach would be to
> create the xhr after you're done with the first runTest call to make sure it
> will never start before the first one.
> 
> > LayoutTests/http/tests/performance/performance-resource-timing-xhr-single-entry.html:35
> > +            setTimeout(runTest, 10);
> 
> Why do you call runTest twice but only expect it to be run once?

I copied this mechanism from another test where the XHR call is racing with window's onload event (as XHR is not blocking onload). So, I wanted the test to run once at the later one of those events, but not inside XHR's onload event, as the entries are not yet added at this point. It's true that for the xhr-single-entry I could just eliminate the window.onload part altogether.

[Yoav Weiss]

Created attachment 279666 [details]
Patch

[Yoav Weiss]

Created attachment 279707 [details]
Patch

[Yoav Weiss]

Latest patch adds back the tests that got rolled out in http://trac.webkit.org/changeset/201343, as this fixes their flakiness.

Let me know if it's preferred that I'll split this patch into two separate ones (as it treats two different flakiness issues).

[Alex Christensen]

Could you describe the difference in your latest patch?

[Yoav Weiss]

Created attachment 279818 [details]
Patch

[Yoav Weiss]

I removed the parts related to initiator tests flakiness, and only left in the parts related to XHR flakiness, in order to make the patch clearer. I'll submit the initiator parts as a separate patch.

[Yoav Weiss]

Initiator changes are at https://bugs.webkit.org/show_bug.cgi?id=158094

[WebKit Commit Bot]

Comment on attachment 279818 [details]
Patch

Clearing flags on attachment: 279818

Committed r201414: <http://trac.webkit.org/changeset/201414>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.