Summary: | Reset cookie partitioning state after network process crashes | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | bfulgham, buildbot, cdumez, wilander | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Brent Fulgham
2017-07-09 20:04:42 PDT
Created attachment 314999 [details]
Patch
Created attachment 315011 [details]
Patch
Created attachment 315013 [details]
Patch
Created attachment 315064 [details]
Patch
Comment on attachment 315064 [details] Patch Attachment 315064 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4097651 New failing tests: storage/indexeddb/modern/new-database-after-user-delete.html Created attachment 315071 [details]
Archive of layout-test-results from ews112 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 315074 [details]
Patch (Rebaselined)
Comment on attachment 315074 [details] Patch (Rebaselined) View in context: https://bugs.webkit.org/attachment.cgi?id=315074&action=review Bots are red. Will do a proper review tomorrow. > Source/WebKit2/UIProcess/WebProcessPool.cpp:485 > +#if HAVE(CFNETWORK_STORAGE_PARTITIONING) The method we're calling is is not specific to partitioning (at least based on its name) and could be extended in the future to do other things. Therefore, I would move this ifdef to the implementation, where we actually call a partitioning-related method. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:634 > +void WebResourceLoadStatisticsStore::networkProcessDidCrash() I would call this scheduleCookiePartitioningStateReset. The "schedule" prefix is a pattern I have used for other methods in this class that dispatch to a background queue before doing something. > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:154 > + void resetPartitionCookiesState(); Where is the implementation? Also, I would call this resetCookiePartitioningState. > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1108 > +void WebsiteDataStore::notifyResourceLoadStatisticsNetworkProcessDidCrash() I would just call this one networkProcessDidCrash(). You could imagine it being useful for other things than load statistics. Created attachment 315123 [details]
Patch
Comment on attachment 315074 [details] Patch (Rebaselined) View in context: https://bugs.webkit.org/attachment.cgi?id=315074&action=review >> Source/WebKit2/UIProcess/WebProcessPool.cpp:485 >> +#if HAVE(CFNETWORK_STORAGE_PARTITIONING) > > The method we're calling is is not specific to partitioning (at least based on its name) and could be extended in the future to do other things. Therefore, I would move this ifdef to the implementation, where we actually call a partitioning-related method. Will do. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:634 >> +void WebResourceLoadStatisticsStore::networkProcessDidCrash() > > I would call this scheduleCookiePartitioningStateReset. The "schedule" prefix is a pattern I have used for other methods in this class that dispatch to a background queue before doing something. Makes sense. >> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:154 >> + void resetPartitionCookiesState(); > > Where is the implementation? Also, I would call this resetCookiePartitioningState. yes -- that's why the bots are red! Too many merges yesterday! :-) >> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1108 >> +void WebsiteDataStore::notifyResourceLoadStatisticsNetworkProcessDidCrash() > > I would just call this one networkProcessDidCrash(). You could imagine it being useful for other things than load statistics. Will do. Created attachment 315129 [details]
Patch
Comment on attachment 315129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315129&action=review r=me if the bots are happy. > Source/WebKit2/UIProcess/WebProcessPool.cpp:485 > + m_websiteDataStore->websiteDataStore().networkProcessDidCrash(); Using websiteDataStore() instead of m_websiteDataStore would look safer as it returns a reference. > LayoutTests/http/tests/loading/resourceLoadStatistics/resources/get-cookies.php:13 > +if(!isset($_COOKIE[$_GET["name3"]])) { Could we also check that isset($_GET["name3"]) to avoid having to rebaseline other tests that do not have a name3? Comment on attachment 315129 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315129&action=review >> Source/WebKit2/UIProcess/WebProcessPool.cpp:485 >> + m_websiteDataStore->websiteDataStore().networkProcessDidCrash(); > > Using websiteDataStore() instead of m_websiteDataStore would look safer as it returns a reference. Will do. >> LayoutTests/http/tests/loading/resourceLoadStatistics/resources/get-cookies.php:13 >> +if(!isset($_COOKIE[$_GET["name3"]])) { > > Could we also check that isset($_GET["name3"]) to avoid having to rebaseline other tests that do not have a name3? Yep. I just tried this and it works properly. It also reduces the size of this change. Committed r219352: <http://trac.webkit.org/changeset/219352> |