REGRESSION (r200887): LayoutTest http/tests/performance/performance-resource-timing-cached-entries.html is flaky

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

LayoutTest http/tests/performance/performance-resource-timing-cached-entries.html is flaky on all platforms

Most recent failure:
<https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r201023%20(5296)/results.html>

Flakiness dashboard:
<https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fperformance%2Fperformance-resource-timing-cached-entries.html>

--- /Volumes/Data/slave/elcapitan-debug-tests-wk1/build/layout-test-results/http/tests/performance/performance-resource-timing-cached-entries-expected.txt
+++ /Volumes/Data/slave/elcapitan-debug-tests-wk1/build/layout-test-results/http/tests/performance/performance-resource-timing-cached-entries-actual.txt
@@ -1,2 +1,2 @@
-PASS foundResource is 2
+FAIL foundResource should be 2. Was 3.

[Alexey Proskuryakov]

This test is super flaky, even causing EWS false positives. It needs to be dealt with very soon.

Should we just roll out the patch, given that its test doesn't work reliably?

[Yoav Weiss]

I'm looking into it now.

[Yoav Weiss]

Created attachment 279216 [details]
Patch

[Yoav Weiss]

I wasn't able to recreate the issue locally, but since the test showed more entries than expected, clearing the entries at the beginning of the test might do the trick.
I'll open a separate issue to investigate why this is required in some cases. (as it should happen automatically between navigations)

[WebKit Commit Bot]

Attachment 279216 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yoav Weiss]

Created attachment 279221 [details]
Patch

[Yoav Weiss]

(In reply to comment #6)
> Created attachment 279221 [details]
> Patch

While removing my own tabs from the ChangeLog (oops), I found several others, so removed them as well.

[Alexey Proskuryakov]

Comment on attachment 279221 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        This patch attempts to unflake the test.

What is your plan for investigating the root cause? With the test deflaked on the bots, and with the issue not reproducible for you locally, I don't see how it can be actionable.

[Yoav Weiss]

I'm not sure, but I thought unflaking the test would help avoid EWS false positives. We could always later on introduce an ignored test that just checks for this flakiness on the bots.

Regarding the issue itself, the entries are stored inside m_performance which is referenced by DOMWindow. Is it possible that DOMWindow survives navigations/test runs?

If so, the solution is most probably to clear m_performance when a new navigation is initiated. Does that make sense?

[Alexey Proskuryakov]

> Is it possible that DOMWindow survives navigations/test runs?

That seems unlikely.

Is there any logging you can add to the test to see what happened (maybe print out all the resources if the count is wrong)? You can mark the test as flaky and add the extra logging - that way you can collect information and still unbreak EWS.

[Yoav Weiss]

(In reply to comment #10)
> > Is it possible that DOMWindow survives navigations/test runs?
> 
> That seems unlikely.
> 
> Is there any logging you can add to the test to see what happened (maybe
> print out all the resources if the count is wrong)? You can mark the test as
> flaky and add the extra logging - that way you can collect information and
> still unbreak EWS.

That makes sense. I'll do that shortly.

[Yoav Weiss]

Created attachment 279286 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 279286 [details]
Patch

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

> LayoutTests/http/tests/performance/performance-resource-timing-cached-entries.html:11
> +    if (window.performance)
> +        console.log("current entries: " + performance.getEntriesByType('resource').length);

I was thinking of something like (right after a shouldBe()):

if (foundResource !== 2) {
    for (var i = 0; i < resources.length; ++i) {
        if (resources[i].name.indexOf("square") !== -1)
            debug(resources.name);
    }
}

That way, there will be more information logged, and you won't need to change the expectation.

[Yoav Weiss]

Created attachment 279355 [details]
Patch

[Yoav Weiss]

Created attachment 279357 [details]
Patch

[Yoav Weiss]

Comment on attachment 279357 [details]
Patch

Added more logs. Landing

[Build Bot]

Comment on attachment 279357 [details]
Patch

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

New failing tests:
storage/websql/database-lock-after-reload.html

[Build Bot]

Created attachment 279360 [details]
Archive of layout-test-results from ews103 for mac-yosemite

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

[Yoav Weiss]

Comment on attachment 279357 [details]
Patch

The failing tests seem unrelated. Trying again to cq+

[WebKit Commit Bot]

Comment on attachment 279357 [details]
Patch

Clearing flags on attachment: 279357

Committed r201130: <http://trac.webkit.org/changeset/201130>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.