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
Brent Fulgham
2019-01-21 14:55:45 PST
Created attachment 359803 [details]
Patch
Created attachment 359805 [details]
Patch
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. Looks like the patch is still out-of-sync with ToT. Uploading a newly-rebased version. Created attachment 359816 [details]
Patch
Created attachment 359844 [details]
Patch
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? Created attachment 359906 [details]
Patch
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 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. Created attachment 359915 [details]
Patch
Created attachment 359916 [details]
Patch
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 (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. Committed r240360: <https://trac.webkit.org/changeset/240360> 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. 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 |