The following layout test is flaky on High Sierra WK2 http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-deletion.html Probable cause: The failures started after https://trac.webkit.org/changeset/228294 Flakiness Dashboard: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fpartitioned-and-unpartitioned-cookie-deletion.html https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r228318%20(2007)/results.html Diff: --- /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-deletion-expected.txt +++ /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/partitioned-and-unpartitioned-cookie-deletion-actual.txt @@ -27,13 +27,13 @@ -------- After removal, should receive no cookies. Did not receive cookie named 'firstPartyCookie'. -Did not receive cookie named 'thirdPartyCookie'. -Client-side document.cookie: +Received cookie named 'thirdPartyCookie'. +Client-side document.cookie: thirdPartyCookie=value -------- Frame: '<!--framePath //<!--frame4-->-->' -------- After user interaction, should receive no cookies. -Did not receive cookie named 'firstPartyCookie'. +Received cookie named 'firstPartyCookie'. Did not receive cookie named 'thirdPartyCookie'. -Client-side document.cookie: +Client-side document.cookie: firstPartyCookie=value
Marked as flaky in: https://trac.webkit.org/changeset/228403/webkit
<rdar://problem/37517370>
Created attachment 333988 [details] Patch
Comment on attachment 333988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333988&action=review I think this is close, but not quite right. r- to move the last few steps of the method inside the completion handlers. > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:282 > + pruneStatisticsIfNeeded(); Shouldn't "pruneStatisticsIfNeeded()" and "m_persistentStorage.scheduleOrWriteMemoryStore" be part of the completion handler? I think part of the flakiness is that 'removeDataRecords' can take time to complete (hopping from work queue, to main thread, to work queue, to main thread). I think after this patch, removeDataRecords will be called at the expected time now, but that 'pruneStatisticsIfNeeded' might run before removeDataRecords completes (since removeDataRecords hops to the main thread before doing the delete, which might allow the work queue to return and call 'pruneStatisticsIfNeeded' before the deletes happen. Likewise, I don't think we want to schedule the write of the in-memory store until these bits of work are finished, so they should be inside the completion handler, too.
(In reply to Brent Fulgham from comment #4) > Comment on attachment 333988 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=333988&action=review > > I think this is close, but not quite right. r- to move the last few steps of > the method inside the completion handlers. > > > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:282 > > + pruneStatisticsIfNeeded(); > > Shouldn't "pruneStatisticsIfNeeded()" and > "m_persistentStorage.scheduleOrWriteMemoryStore" be part of the completion > handler? > > I think part of the flakiness is that 'removeDataRecords' can take time to > complete (hopping from work queue, to main thread, to work queue, to main > thread). I think after this patch, removeDataRecords will be called at the > expected time now, but that 'pruneStatisticsIfNeeded' might run before > removeDataRecords completes (since removeDataRecords hops to the main thread > before doing the delete, which might allow the work queue to return and call > 'pruneStatisticsIfNeeded' before the deletes happen. > > Likewise, I don't think we want to schedule the write of the in-memory store > until these bits of work are finished, so they should be inside the > completion handler, too. Thanks, Brent! I was thinking along those lines too but would you be fine landing this as a first step? The benefit would be to address the flakiness that’s hitting the bots.
Comment on attachment 333988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333988&action=review >>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:282 >>> + pruneStatisticsIfNeeded(); >> >> Shouldn't "pruneStatisticsIfNeeded()" and "m_persistentStorage.scheduleOrWriteMemoryStore" be part of the completion handler? >> >> I think part of the flakiness is that 'removeDataRecords' can take time to complete (hopping from work queue, to main thread, to work queue, to main thread). I think after this patch, removeDataRecords will be called at the expected time now, but that 'pruneStatisticsIfNeeded' might run before removeDataRecords completes (since removeDataRecords hops to the main thread before doing the delete, which might allow the work queue to return and call 'pruneStatisticsIfNeeded' before the deletes happen. >> >> Likewise, I don't think we want to schedule the write of the in-memory store until these bits of work are finished, so they should be inside the completion handler, too. > > Thanks, Brent! I was thinking along those lines too but would you be fine landing this as a first step? The benefit would be to address the flakiness that’s hitting the bots. No -- the change I'm asking for would only be copying two lines and adding them to the two completion handlers here. I guess you'll need to capture 'this' and so forth, but it's not a big change. I think that should be done as part of this patch.
Created attachment 334165 [details] Patch
Sorry for the delay in updating the patch. I was out for a conference.
Comment on attachment 334165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334165&action=review r=me. Thanks for being patient with these changes! > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:291 > + } Great! I think that's what we needed.
Comment on attachment 334165 [details] Patch Thanks, Brent!
Comment on attachment 334165 [details] Patch Clearing flags on attachment: 334165 Committed r228828: <https://trac.webkit.org/changeset/228828>
All reviewed patches have been landed. Closing bug.
Added a delay in the test case to see if that stabilizes it. https://bugs.webkit.org/show_bug.cgi?id=183024