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

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

Summary: [iOS Debug] Multiple resourceLoadStatistics redirect tests are flaky timeouts

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	New Bugs (show other bugs)
Version:	Other
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Brent Fulgham

URL:
Keywords:	InRadar

Depends on:
Blocks:

Reported:	2018-02-28 09:53 PST by Ryan Haddad
Modified:	2018-06-23 01:26 PDT (History)
CC List:	7 users (show)

See Also:

Attachments
Patch (32.77 KB, patch) 2018-06-21 13:38 PDT, Brent Fulgham	no flags	Details \| Formatted Diff \| Diff
Patch (32.38 KB, patch) 2018-06-21 14:56 PDT, Brent Fulgham	no flags	Details \| Formatted Diff \| Diff
Patch (32.49 KB, patch) 2018-06-21 17:56 PDT, Brent Fulgham	cdumez: review+	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (8.53 MB, application/zip) 2018-06-21 20:01 PDT, EWS Watchlist	no flags	Details
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Ryan Haddad 2018-02-28 09:53:26 PST

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

Comment 1 Ryan Haddad 2018-02-28 09:54:18 PST

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

Comment 2 Ryan Haddad 2018-02-28 10:25:09 PST

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

Comment 3 Radar WebKit Bug Importer 2018-02-28 11:08:30 PST

<rdar://problem/37992317>

Comment 4 Ryan Haddad 2018-02-28 11:15:23 PST

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

Comment 5 Brent Fulgham 2018-06-21 13:38:03 PDT

Created attachment 343265 [details]
Patch

Comment 6 EWS Watchlist 2018-06-21 13:40:07 PDT

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.

Comment 7 Chris Dumez 2018-06-21 13:52:23 PDT

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.

Comment 8 Brent Fulgham 2018-06-21 14:56:17 PDT

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.

Comment 9 Brent Fulgham 2018-06-21 14:56:34 PDT

Created attachment 343280 [details]
Patch

Comment 10 EWS Watchlist 2018-06-21 14:58:51 PDT

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.

Comment 11 Chris Dumez 2018-06-21 15:15:48 PDT

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.

Comment 12 Brent Fulgham 2018-06-21 17:56:44 PDT

Created attachment 343299 [details]
Patch

Comment 13 EWS Watchlist 2018-06-21 17:59:47 PDT

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.

Comment 14 EWS Watchlist 2018-06-21 20:01:07 PDT

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

Comment 15 EWS Watchlist 2018-06-21 20:01:09 PDT

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

Comment 16 Brent Fulgham 2018-06-22 08:25:03 PDT

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

Comment 17 Chris Dumez 2018-06-22 08:39:15 PDT

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() ?

Comment 18 Brent Fulgham 2018-06-22 09:51:46 PDT

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.

Comment 19 Brent Fulgham 2018-06-22 09:55:03 PDT

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

Comment 20 Alexey Proskuryakov 2018-06-23 01:26:23 PDT

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