Bug 174306 – Reset cookie partitioning state after network process crashes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brent Fulgham]

When the network process crashes, we need to notify the respawned process of the current knowledge of prevalent resources and other load statistics.

[Brent Fulgham]

<rdar://problem/33171605>

[Brent Fulgham]

Created attachment 314999 [details]
Patch

[Brent Fulgham]

Created attachment 315011 [details]
Patch

[Brent Fulgham]

Created attachment 315013 [details]
Patch

[Brent Fulgham]

Created attachment 315064 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

Created attachment 315074 [details]
Patch (Rebaselined)

[Chris Dumez]

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.

[Brent Fulgham]

Created attachment 315123 [details]
Patch

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 315129 [details]
Patch

[Chris Dumez]

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?

[Brent Fulgham]

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.

[Brent Fulgham]

Committed r219352: <http://trac.webkit.org/changeset/219352>