Bug 193659

Summary: Switch NetworkStorageSession portions of ResourceLoadStatistics to Async message passing style
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, bfulgham, tsavell, webkit-bug-importer, wilander
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 193556    
Bug Blocks: 193199, 193303    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch achristensen: review+

Description Brent Fulgham 2019-01-21 14:55:45 PST
Change the implementations of ResourceLoadStatistics code in NetworkStorageSession to use Async message passing, so that more of the code is autogenerated. This should make test runs more consistent, and should reduce the possibility of bookkeeping errors in the message handling implementations.
Comment 1 Radar WebKit Bug Importer 2019-01-21 14:56:27 PST
<rdar://problem/47433290>
Comment 2 Brent Fulgham 2019-01-22 16:21:31 PST
Created attachment 359803 [details]
Patch
Comment 3 Brent Fulgham 2019-01-22 16:30:13 PST
Created attachment 359805 [details]
Patch
Comment 4 Alex Christensen 2019-01-22 17:04:24 PST
Comment on attachment 359805 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ShouldGrandfatherStatistics.h:40
> +template<> struct EnumTraits<WebKit::ShouldGrandfatherStatistics> {

Fun fact: enum classes with bool as the underlying type don't need an EnumTraits because the decoder knows it can only have two values.
Comment 5 Brent Fulgham 2019-01-22 17:06:32 PST
Looks like the patch is still out-of-sync with ToT. Uploading a newly-rebased version.
Comment 6 Brent Fulgham 2019-01-22 17:06:48 PST
Created attachment 359816 [details]
Patch
Comment 7 Brent Fulgham 2019-01-22 20:25:49 PST
Created attachment 359844 [details]
Patch
Comment 8 Alex Christensen 2019-01-23 10:53:32 PST
Comment on attachment 359844 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ShouldGrandfatherStatistics.h:40
> +template<> struct EnumTraits<WebKit::ShouldGrandfatherStatistics> {

Still not needed.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:604
> +            resourceLoadStatistics->dumpResourceLoadStatistics(WTFMove(completionHandler));

It seems like we should call the completion handler in the other cases, too.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:723
>          ASSERT_NOT_REACHED();

ditto

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:869
>          ASSERT_NOT_REACHED();

ditto

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:887
>          ASSERT_NOT_REACHED();

ditto

> Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:409
> +#if ENABLE(RESOURCE_LOAD_STATISTICS)

Should we just call the callback with the context if the feature is not enabled?
Comment 9 Brent Fulgham 2019-01-23 11:13:58 PST
Created attachment 359906 [details]
Patch
Comment 10 Alex Christensen 2019-01-23 11:14:52 PST
Comment on attachment 359906 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:931
>          ASSERT_NOT_REACHED();

more completion handler calling
Comment 11 Brent Fulgham 2019-01-23 11:31:22 PST
Comment on attachment 359906 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:931
>>          ASSERT_NOT_REACHED();
> 
> more completion handler calling

Even with ASSERT_NOT_REACHED? I guess that's debug-only.
Comment 12 Brent Fulgham 2019-01-23 11:40:28 PST
Created attachment 359915 [details]
Patch
Comment 13 Brent Fulgham 2019-01-23 11:47:38 PST
Created attachment 359916 [details]
Patch
Comment 14 Alex Christensen 2019-01-23 13:52:45 PST
Comment on attachment 359916 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ShouldGrandfatherStatistics.h:34
> +    

extra space
Comment 15 Brent Fulgham 2019-01-23 13:54:16 PST
(In reply to Alex Christensen from comment #14)
> Comment on attachment 359916 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359916&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ShouldGrandfatherStatistics.h:34
> > +    
> 
> extra space

Doh! Fixed.
Comment 16 Brent Fulgham 2019-01-23 13:55:55 PST
Committed r240360: <https://trac.webkit.org/changeset/240360>
Comment 17 Truitt Savell 2019-01-25 13:14:46 PST
I found that https://trac.webkit.org/changeset/240360/webkit

caused this test 
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fcapped-lifetime-for-cookie-set-in-js.html

to become a flakey failure on wk2.

History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fcapped-lifetime-for-cookie-set-in-js.html

reproduced with:
run-webkit-tests --root testbuild-240360 http/tests/resourceLoadStatistics/capped-lifetime-for-cookie-set-in-js.html --iterations 500 -f 

r240360 reproduces the failures while r240359 has no failures occur.
Comment 18 Truitt Savell 2019-01-25 13:56:08 PST
I am also very certain that http/tests/resourceLoadStatistics/cap-cache-max-age-for-prevalent-resource.html has become a flaky failure due to https://trac.webkit.org/changeset/240360/webkit

Tracking my investigation in https://bugs.webkit.org/show_bug.cgi?id=193752