Bug 182719 – Make WebResourceLoadStatisticsStore::processStatisticsAndDataRecords() call WebProcessProxy::notifyPageStatisticsAndDataRecordsProcessed() in a proper callback

• Component: New Bugs
• Status: RESOLVED FIXED

[Matt Lewis]

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

[Matt Lewis]

Marked as flaky in: https://trac.webkit.org/changeset/228403/webkit

[Radar WebKit Bug Importer]

<rdar://problem/37517370>

[John Wilander]

Created attachment 333988 [details]
Patch

[Brent Fulgham]

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.

[John Wilander]

(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.

[Brent Fulgham]

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.

[John Wilander]

Created attachment 334165 [details]
Patch

[John Wilander]

Sorry for the delay in updating the patch. I was out for a conference.

[Brent Fulgham]

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.

[John Wilander]

Comment on attachment 334165 [details]
Patch

Thanks, Brent!

[WebKit Commit Bot]

Comment on attachment 334165 [details]
Patch

Clearing flags on attachment: 334165

Committed r228828: <https://trac.webkit.org/changeset/228828>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Wilander]

Added a delay in the test case to see if that stabilizes it. https://bugs.webkit.org/show_bug.cgi?id=183024