Bug 197285

Summary: REGRESSION (~244100) [Mac WK2 Debug] Layout Test http/tests/resourceLoadStatistics/prune-statistics.html is a flaky failure
Product: WebKit Reporter: Shawn Roberts <sroberts>
Component: Tools / TestsAssignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, jlewis3, katherine_cheney, lforschler, ryanhaddad, tsavell, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Shawn Roberts
Reported 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
Attachments
Patch (68.77 KB, patch)
2019-10-15 09:43 PDT, Kate Cheney 		no flags
DetailsFormatted DiffDiff
Patch (14.08 KB, patch)
2019-10-15 11:55 PDT, Kate Cheney 		no flags
DetailsFormatted DiffDiff
Patch (3.95 KB, patch)
2019-10-15 17:31 PDT, Kate Cheney 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-04-25 09:44:08 PDT
<rdar://problem/50208370>
Shawn Roberts
Comment 2 2019-04-25 09:49:01 PDT
Marked flaky in https://trac.webkit.org/changeset/244647/webkit
Shawn Roberts
Comment 3 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.
Shawn Roberts
Comment 4 2019-05-23 15:08:44 PDT
Updated expectations for Release builds as well. https://trac.webkit.org/changeset/245720/webkit
Kate Cheney
Comment 5 2019-10-15 09:43:34 PDT
Created attachment 380995 [details] Patch
Chris Dumez
Comment 6 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.
Chris Dumez
Comment 7 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.
Chris Dumez
Comment 8 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.
Kate Cheney
Comment 9 2019-10-15 11:55:29 PDT
Created attachment 381008 [details] Patch
Chris Dumez
Comment 10 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"?
Kate Cheney
Comment 11 2019-10-15 17:31:21 PDT
Created attachment 381041 [details] Patch
Kate Cheney
Comment 12 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.
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2019-10-15 19:08:40 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.