Bug 183216 – [iOS Debug] Multiple resourceLoadStatistics redirect tests are flaky timeouts

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryan Haddad]

The following layout test is flaky on iOS Debug WK2, timing out more often than it passes.

http/tests/resourceLoadStatistics/add-partitioning-to-redirect.html

Probable cause:

Unknown.

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fadd-partitioning-to-redirect.html

[Ryan Haddad]

Flakiness dashboard seems to blame https://trac.webkit.org/changeset/228983/webkit

[Ryan Haddad]

These two tests are also flaky timeouts on iOS Debug WK2:
http/tests/resourceLoadStatistics/add-blocking-to-redirect.html
http/tests/resourceLoadStatistics/remove-partitioning-in-redirect.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fadd-blocking-to-redirect.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fremove-partitioning-in-redirect.html

[Radar WebKit Bug Importer]

<rdar://problem/37992317>

[Ryan Haddad]

Disabled these three tests on Debug in https://trac.webkit.org/changeset/229098/webkit

[Brent Fulgham]

Created attachment 343265 [details]
Patch

[EWS Watchlist]

Attachment 343265 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:374:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:720:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:793:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:829:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:842:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:882:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:108:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:112:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:114:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 11 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 343265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343265&action=review

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1044
> +            grandfatherExistingWebsiteData([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)]() mutable {

Yes, I think this is a good way to fix this. grandfatherExistingWebsiteData() is called on the bg thread and calls its completion handler on the bg thread. scheduleClearInMemoryAndPersistent() is called on the main thread so it is the one doing the dispatch to the main thread before calling its completion handler. The fix below is not consistent however, see my comment.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1234
> +void WebResourceLoadStatisticsStore::updateCookiePartitioning(CompletionHandler<void()>&& completionHandler)

Given that this method is called on the bg thread, I think it would be better to call the completion handler on the same bg thread. If some call sites need to call a completion handler on the main thread, they can dispatch themselves.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1260
> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {

I do not think we should do this here. Let's do at the call site instead.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1302
> +        completionHandler();

ditto, I don't think we should call the completion handler on the main thread here.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1313
> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {

updateCookiePartitioningForDomains() is called on the bg thread, so I think it is awkward for it to call its completion handler on the different thread.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1348
> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {

ditto.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1364
> +    RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {

ditto.

[Brent Fulgham]

Comment on attachment 343265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343265&action=review

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1044
>> +            grandfatherExistingWebsiteData([protectedThis = WTFMove(protectedThis), completionHandler = WTFMove(completionHandler)]() mutable {
> 
> Yes, I think this is a good way to fix this. grandfatherExistingWebsiteData() is called on the bg thread and calls its completion handler on the bg thread. scheduleClearInMemoryAndPersistent() is called on the main thread so it is the one doing the dispatch to the main thread before calling its completion handler. The fix below is not consistent however, see my comment.

Ah, I see. Yes -- I'll do it that way.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1234
>> +void WebResourceLoadStatisticsStore::updateCookiePartitioning(CompletionHandler<void()>&& completionHandler)
> 
> Given that this method is called on the bg thread, I think it would be better to call the completion handler on the same bg thread. If some call sites need to call a completion handler on the main thread, they can dispatch themselves.

Fixed.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1260
>> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {
> 
> I do not think we should do this here. Let's do at the call site instead.

Fixed.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1302
>> +        completionHandler();
> 
> ditto, I don't think we should call the completion handler on the main thread here.

Done.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1313
>> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {
> 
> updateCookiePartitioningForDomains() is called on the bg thread, so I think it is awkward for it to call its completion handler on the different thread.

Fixed.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1348
>> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {
> 
> ditto.

Fixed.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1364
>> +    RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)] {
> 
> ditto.

Fixed.

[Brent Fulgham]

Created attachment 343280 [details]
Patch

[EWS Watchlist]

Attachment 343280 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:374:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:720:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:793:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:829:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:842:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:882:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:108:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:112:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:114:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 11 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 343280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343280&action=review

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1306
> +        completionHandler();

I do not think this should be called on the main thread. This method is called on the bg thread.

[Brent Fulgham]

Created attachment 343299 [details]
Patch

[EWS Watchlist]

Attachment 343299 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:374:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:720:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:793:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:829:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:842:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:882:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:108:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:112:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:114:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:115:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:118:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 11 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 343299 [details]
Patch

Attachment 343299 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8285350

New failing tests:
performance-api/performance-observer-no-document-leak.html

[EWS Watchlist]

Created attachment 343304 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Brent Fulgham]

performance-api/performance-observer-no-document-leak.html is a flaky timeout, and doesn't seem related to this patch.

[Chris Dumez]

Comment on attachment 343299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343299&action=review

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1306
> +        m_statisticsQueue->dispatch([completionHandler = WTFMove(completionHandler)] () {

Do we really need to redispatch here, cannot we move the call to completionHandler(); before the RunLoop::main().dispatch() ?

[Brent Fulgham]

Comment on attachment 343299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343299&action=review

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1306
>> +        m_statisticsQueue->dispatch([completionHandler = WTFMove(completionHandler)] () {
> 
> Do we really need to redispatch here, cannot we move the call to completionHandler(); before the RunLoop::main().dispatch() ?

Yes (sadly) -- we need the function m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler to run before we call the completion handler.

[Brent Fulgham]

Committed r233083: <https://trac.webkit.org/changeset/233083>

[Alexey Proskuryakov]

Are there any TestExpectations to remove after this change? Hopefully these aren't flaky any more:

> Disabled these three tests on Debug in https://trac.webkit.org/changeset/229098/webkit