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 / Tests | Assignee: | 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
Shawn Roberts
2019-04-25 09:43:28 PDT
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. |