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
<rdar://problem/50208370>
Marked flaky in https://trac.webkit.org/changeset/244647/webkit
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.
Updated expectations for Release builds as well. https://trac.webkit.org/changeset/245720/webkit
Created attachment 380995 [details] Patch
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 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 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.
Created attachment 381008 [details] Patch
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"?
Created attachment 381041 [details] Patch
(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 on attachment 381041 [details] Patch Clearing flags on attachment: 381041 Committed r251176: <https://trac.webkit.org/changeset/251176>
All reviewed patches have been landed. Closing bug.