Bug 213553 - [ iOS macOS ] http/tests/resourceLoadStatistics/grandfathering-database.html is a rare flaky timeout
Summary: [ iOS macOS ] http/tests/resourceLoadStatistics/grandfathering-database.html ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
: 213552 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-06-24 05:02 PDT by Jacob Uphoff
Modified: 2020-07-20 12:47 PDT (History)
7 users (show)

See Also:


Attachments
Patch (45.86 KB, patch)
2020-07-14 15:35 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch for landing (45.96 KB, patch)
2020-07-20 11:03 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jacob Uphoff 2020-06-24 05:02:52 PDT
http/tests/resourceLoadStatistics/grandfathering-database.html

This test has been a rare flaky timeout since it started running on iOS and then shortly after for macOS. The timeouts appear on Release WK2. It times out more often on iOS and internally for macOS.

History:

https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2FresourceLoadStatistics%2Fgrandfathering-database.html&platform=ios&platform=mac&limit=50000

Diff:

@@ -1,5 +1,3 @@
-PASS successfullyParsed is true
+#PID UNRESPONSIVE - WebKitTestRunner (pid 14028)
+FAIL: Timed out waiting for notifyDone to be called
 
-TEST COMPLETE
-PASS Grandfathered cookie was not purged.
-
Comment 1 Radar WebKit Bug Importer 2020-06-24 05:03:26 PDT
<rdar://problem/64696432>
Comment 2 Jacob Uphoff 2020-06-24 05:10:52 PDT
Set expectations here: https://trac.webkit.org/changeset/263452/webkit
Comment 3 Alexey Proskuryakov 2020-06-25 18:11:27 PDT
*** Bug 213552 has been marked as a duplicate of this bug. ***
Comment 4 Kate Cheney 2020-07-14 15:35:16 PDT
Created attachment 404294 [details]
Patch
Comment 5 Darin Adler 2020-07-15 10:33:00 PDT
Comment on attachment 404294 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2088
> +    bool notifyPagesWhenDataRecordsWereModified = !!callback;
> +    // Setting a callback implies we expect to receive callbacks. So register for them.
> +    setStatisticsNotifyPagesWhenDataRecordsWereScanned(notifyPagesWhenDataRecordsWereModified);

I understand turning this on if we were passed a callback.

I don’t fully understand turning it off if we were passed null. Are we the one and only client? Does this need to be reset between tests?
Comment 6 Kate Cheney 2020-07-20 10:28:35 PDT
(In reply to Darin Adler from comment #5)
> Comment on attachment 404294 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404294&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2088
> > +    bool notifyPagesWhenDataRecordsWereModified = !!callback;
> > +    // Setting a callback implies we expect to receive callbacks. So register for them.
> > +    setStatisticsNotifyPagesWhenDataRecordsWereScanned(notifyPagesWhenDataRecordsWereModified);
> 
> I understand turning this on if we were passed a callback.
> 
> I don’t fully understand turning it off if we were passed null. Are we the
> one and only client? Does this need to be reset between tests?

Yes, this is reset between tests in WKWebsiteDataStoreStatisticsResetToConsistentState. 

This function doesn't make sense without a callback, but thinking more about it, I agree turning it off when passed null does not make sense. I think the better option is only calling TestRunner::setStatisticsNotifyPagesWhenDataRecordsWereScanned() if the callback isn't null, and doing nothing if it is null. I'll change that before landing. Thanks for the comments!
Comment 7 Kate Cheney 2020-07-20 11:03:58 PDT
Created attachment 404731 [details]
Patch for landing
Comment 8 EWS 2020-07-20 12:47:54 PDT
Committed r264609: <https://trac.webkit.org/changeset/264609>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404731 [details].