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
: 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)
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, Build Bot
no flags Details

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 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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