Bug 193659 – Switch NetworkStorageSession portions of ResourceLoadStatistics to Async message passing style

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/47433290>

[Brent Fulgham]

Created attachment 359803 [details]
Patch

[Brent Fulgham]

Created attachment 359805 [details]
Patch

[Alex Christensen]

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.

[Brent Fulgham]

Looks like the patch is still out-of-sync with ToT. Uploading a newly-rebased version.

[Brent Fulgham]

Created attachment 359816 [details]
Patch

[Brent Fulgham]

Created attachment 359844 [details]
Patch

[Alex Christensen]

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?

[Brent Fulgham]

Created attachment 359906 [details]
Patch

[Alex Christensen]

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

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 359915 [details]
Patch

[Brent Fulgham]

Created attachment 359916 [details]
Patch

[Alex Christensen]

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

[Brent Fulgham]

(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.

[Brent Fulgham]

Committed r240360: <https://trac.webkit.org/changeset/240360>

[Truitt Savell]

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.

[Truitt Savell]

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