Bug 197285 - REGRESSION (~244100) [Mac WK2 Debug] Layout Test http/tests/resourceLoadStatistics/prune-statistics.html is a flaky failure
Summary: REGRESSION (~244100) [Mac WK2 Debug] Layout Test http/tests/resourceLoadStati...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: katherine_cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-25 09:43 PDT by Shawn Roberts
Modified: 2019-10-15 19:08 PDT (History)
9 users (show)

See Also:


Attachments
Patch (68.77 KB, patch)
2019-10-15 09:43 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (14.08 KB, patch)
2019-10-15 11:55 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2019-10-15 17:31 PDT, katherine_cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Roberts 2019-04-25 09:43:28 PDT
The following layout test is flaky on Mac WK2 Debug

http/tests/resourceLoadStatistics/prune-statistics.html

Probable cause:

Unable to reproduce it locally running with 1 child process or even in full parallel. Test had similar flaky behavior before and was fixed in https://trac.webkit.org/changeset/233888/webkit

Around r244100 it started to fail flakily.

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fprune-statistics.html

Diff:

--- /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/prune-statistics-expected.txt
+++ /Volumes/Data/slave/mojave-debug-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/prune-statistics-actual.txt
@@ -1,3 +1,7 @@
+FAIL: Tried to install a second TestRunner callback for the same event (id 11)
+
+FAIL: Tried to install a second TestRunner callback for the same event (id 19)
+
 Tests that statistics are pruned in the right order.
 
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
@@ -5,13 +9,13 @@
 
 PASS Test iteration 1 passed.
 PASS Test iteration 2 passed.
+FAIL checkIfPrevalent: Test iteration 3 failed. http://127.0.0.3:8000/temp was prevalent
+FAIL checkIfPrevalentAccordingToInitialExpectation: Test iteration 3 failed. http://127.0.0.4:8000/temp wasn't prevalent
+FAIL checkIfPrevalentAccordingToInitialExpectation: Test iteration 3 failed. http://127.0.0.7:8000/temp wasn't prevalent
+FAIL checkIfPrevalentAccordingToInitialExpectation: Test iteration 3 failed. http://127.0.0.8:8000/temp wasn't prevalent
 PASS Test iteration 3 passed.
-PASS Test iteration 4 passed.
-PASS Test iteration 5 passed.
-PASS Test iteration 6 passed.
-PASS Test iteration 7 passed.
-PASS Test iteration 8 passed.
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE
Comment 1 Radar WebKit Bug Importer 2019-04-25 09:44:08 PDT
<rdar://problem/50208370>
Comment 2 Shawn Roberts 2019-04-25 09:49:01 PDT
Marked flaky in https://trac.webkit.org/changeset/244647/webkit
Comment 3 Shawn Roberts 2019-05-23 15:02:35 PDT
Issue is occurring on Mojave Release now.

Still cannot reproduce locally. Even tried with test lists from the worker on the runs and cannot reproduce it locally.
Comment 4 Shawn Roberts 2019-05-23 15:08:44 PDT
Updated expectations for Release builds as well.

https://trac.webkit.org/changeset/245720/webkit
Comment 5 katherine_cheney 2019-10-15 09:43:34 PDT
Created attachment 380995 [details]
Patch
Comment 6 Chris Dumez 2019-10-15 09:46:55 PDT
Comment on attachment 380995 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1323
> +        if (!(parameters().isRunningTest) || testingShouldScheduleStatistics())

It is not good practice to check in code if we are running tests. We used to do this and stopped years ago.
Comment 7 Chris Dumez 2019-10-15 09:49:22 PDT
Comment on attachment 380995 [details]
Patch

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

> Source/WebKit/ChangeLog:11
> +        were called during execution of prune-statistics.html. 

Note that we're supposed to be clearing state between tests to avoid flakiness. Based on your change log, it looks to me that WKTR is not properly clearing ITP state between tests (It leaves scheduled processing checks around). Maybe the fix should be to avoid clear those between tests, with the rest of the ITP data.
Comment 8 Chris Dumez 2019-10-15 09:50:51 PDT
Comment on attachment 380995 [details]
Patch

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

>> Source/WebKit/ChangeLog:11
>> +        were called during execution of prune-statistics.html. 
> 
> Note that we're supposed to be clearing state between tests to avoid flakiness. Based on your change log, it looks to me that WKTR is not properly clearing ITP state between tests (It leaves scheduled processing checks around). Maybe the fix should be to avoid clear those between tests, with the rest of the ITP data.

WKWebsiteDataStoreStatisticsResetToConsistentState() is supposed to be what prevents ITP state to stay around from one test to another. It is getting called by WKTR between each test already.
Comment 9 katherine_cheney 2019-10-15 11:55:29 PDT
Created attachment 381008 [details]
Patch
Comment 10 Chris Dumez 2019-10-15 16:58:24 PDT
Comment on attachment 381008 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:775
> +void NetworkProcess::cancelPendingRequests(PAL::SessionID sessionID, CompletionHandler<void()>&& completionHandler)

Having a name as generic as "cancelPendingRequests" on the NetworkProcess, and yet be so specific to ITP is wrong.

Same comment applies to resetParametersToDefaultValues() below.

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:551
> +    store.cancelPendingRequests([callbackAggregator = callbackAggregator.copyRef()] { });

Could scheduleClearInMemoryAndPersistent() take care of this? Aren't pending requests "in memory data"?
Comment 11 katherine_cheney 2019-10-15 17:31:21 PDT
Created attachment 381041 [details]
Patch
Comment 12 katherine_cheney 2019-10-15 17:32:10 PDT
(In reply to Chris Dumez from comment #10)
> Comment on attachment 381008 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381008&action=review

> > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:551
> > +    store.cancelPendingRequests([callbackAggregator = callbackAggregator.copyRef()] { });
> 
> Could scheduleClearInMemoryAndPersistent() take care of this? Aren't pending
> requests "in memory data"?

Yes, good idea. This makes it a much smaller patch.
Comment 13 WebKit Commit Bot 2019-10-15 19:08:39 PDT
Comment on attachment 381041 [details]
Patch

Clearing flags on attachment: 381041

Committed r251176: <https://trac.webkit.org/changeset/251176>
Comment 14 WebKit Commit Bot 2019-10-15 19:08:40 PDT
All reviewed patches have been landed.  Closing bug.