Bug 161106

Summary: Stop using CookiesStrategy
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, buildbot, don.olmstead, ews-watchlist, koivisto, rniwa, webkit-bug-importer
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews105 for mac-yosemite-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews117 for mac-highsierra
none
Patch
none
patch
none
Patch none

Description Alex Christensen 2016-08-23 14:46:40 PDT
Stop using CookiesStrategy
Comment 1 Alex Christensen 2016-08-23 14:51:20 PDT
Created attachment 286781 [details]
Patch
Comment 2 Build Bot 2016-08-23 15:46:35 PDT
Comment on attachment 286781 [details]
Patch

Attachment 286781 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1930368

New failing tests:
http/tests/cache/disk-cache/disk-cache-vary-cookie.html
Comment 3 Build Bot 2016-08-23 15:46:38 PDT
Created attachment 286794 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 4 Alex Christensen 2016-08-23 17:21:39 PDT
Comment on attachment 286781 [details]
Patch

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

> Source/WebCore/platform/network/CacheValidation.cpp:343
> -        auto* cookieStrategy = platformStrategies() ? platformStrategies()->cookiesStrategy() : nullptr;
> -        if (!cookieStrategy) {
> -            ASSERT(sessionID == SessionID::defaultSessionID());
> -            return cookieRequestHeaderFieldValue(NetworkStorageSession::defaultStorageSession(), request.firstPartyForCookies(), request.url());
> -        }
> -        return cookieStrategy->cookieRequestHeaderFieldValue(sessionID, request.firstPartyForCookies(), request.url());
> +        if (auto session = NetworkStorageSession::storageSession(sessionID))
> +            return cookieRequestHeaderFieldValue(*session, request.firstPartyForCookies(), request.url());
> +        return String();

This code is being called from the WebProcess for the Memory Cache and from the NetworkProcess for the Disk Cache.  There is no Page in the NetworkProcess to get a CookieJar from.  It is unclear to me what to do in this case.  Maybe this is an expected change in behavior.
Comment 5 Alex Christensen 2016-08-23 17:36:21 PDT
This needs to be split into two functions, one for the web process and one for the network process
Comment 6 Alex Christensen 2016-08-24 15:41:12 PDT
Created attachment 286899 [details]
Patch
Comment 7 Alex Christensen 2016-08-24 16:30:23 PDT
Created attachment 286905 [details]
Patch
Comment 8 Alex Christensen 2016-08-24 16:45:45 PDT
Created attachment 286910 [details]
Patch
Comment 9 Build Bot 2016-08-24 18:28:20 PDT
Comment on attachment 286910 [details]
Patch

Attachment 286910 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1936999

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/anonymous-mode-unsupported.htm
http/tests/cookies/third-party-cookie-relaxing.html
http/tests/cookies/http-get-cookie-set-in-js.html
http/tests/websocket/tests/hybi/httponly-cookie.pl
http/tests/navigation/statistics.html
http/tests/cookies/private-cookie-storage.html
http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html
http/tests/contentextensions/block-cookies-send.html
http/tests/cache/disk-cache/disk-cache-vary-cookie.html
Comment 10 Build Bot 2016-08-24 18:28:23 PDT
Created attachment 286927 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 11 Alex Christensen 2019-01-14 08:13:46 PST
Created attachment 359037 [details]
Patch
Comment 12 Alex Christensen 2019-01-14 09:04:57 PST
Created attachment 359039 [details]
Patch
Comment 13 Don Olmstead 2019-01-14 09:12:11 PST
Comment on attachment 359039 [details]
Patch

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

r=me assuming compiles on platforms.

Any additional work for other ports?

> Source/WebKitLegacy/win/Plugins/PluginView.cpp:1293
> +            }

What happened here?
Comment 14 Don Olmstead 2019-01-14 09:13:15 PST
(In reply to Don Olmstead from comment #13)
> Comment on attachment 359039 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359039&action=review
> 
> r=me assuming compiles on platforms.
> 
> Any additional work for other ports?
> 
> > Source/WebKitLegacy/win/Plugins/PluginView.cpp:1293
> > +            }
> 
> What happened here?

I'm seeing boxes in the diff through Review Patch after the }
Comment 15 Alex Christensen 2019-01-14 09:23:12 PST
Comment on attachment 359039 [details]
Patch

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

>>> Source/WebKitLegacy/win/Plugins/PluginView.cpp:1293
>>> +            }
>> 
>> What happened here?
> 
> I'm seeing boxes in the diff through Review Patch after the }

I don't know.  This was me blindly editing Windows code in Xcode.  If you could get this actually compiling on Windows that'd be great.
Comment 16 Alex Christensen 2019-01-14 09:26:46 PST
Created attachment 359042 [details]
Patch
Comment 17 Alex Christensen 2019-01-14 10:34:03 PST
Created attachment 359051 [details]
Patch
Comment 18 EWS Watchlist 2019-01-14 11:48:13 PST
Comment on attachment 359051 [details]
Patch

Attachment 359051 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10746850

New failing tests:
http/tests/cache/disk-cache/disk-cache-vary-cookie.html
Comment 19 EWS Watchlist 2019-01-14 11:48:14 PST
Created attachment 359062 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 20 EWS Watchlist 2019-01-14 12:40:10 PST
Comment on attachment 359051 [details]
Patch

Attachment 359051 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10747056

New failing tests:
http/tests/cache/disk-cache/disk-cache-vary-cookie.html
Comment 21 EWS Watchlist 2019-01-14 12:40:11 PST
Created attachment 359069 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6
Comment 22 Alex Christensen 2019-01-14 21:17:31 PST
(In reply to Don Olmstead from comment #13)
> What happened here?
I did this again in another patch and realized what I did.  This was the result of me using emacs commands in Xcode :/
Comment 23 Antti Koivisto 2019-01-15 00:55:36 PST
Comment on attachment 359051 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Stop using CookiesStrategy

A word of motivation would be nice.

> Source/WebCore/loader/CookieJar.cpp:74
> +    if (auto* session = NetworkStorageSession::storageSession(document.sessionID()))
> +        result = session->cookiesForDOM(document.firstPartyForCookies(), sameSiteInfo(document), url, frameID, pageID, includeSecureCookies);
>      else
> -        result = platformStrategies()->cookiesStrategy()->cookiesForDOM(document.sessionID(), document.firstPartyForCookies(), sameSiteInfo(document), url, WTF::nullopt, WTF::nullopt, includeSecureCookies);
> +        ASSERT_NOT_REACHED();

Is there a particular reason for this defensiveness? Why bother null testing and asserting if the session really can't be null?

> Source/WebCore/loader/CookieJar.cpp:111
> +    if (auto* session = NetworkStorageSession::storageSession(document.sessionID()))
> +        session->setCookiesFromDOM(document.firstPartyForCookies(), sameSiteInfo(document), url, frameID, pageID, cookieString);
>      else
> -        platformStrategies()->cookiesStrategy()->setCookiesFromDOM(document.sessionID(), document.firstPartyForCookies(), sameSiteInfo(document), url, WTF::nullopt, WTF::nullopt, cookieString);
> +        ASSERT_NOT_REACHED();

Also here and in many other places.
Comment 24 Alex Christensen 2019-01-15 10:48:37 PST
(In reply to Antti Koivisto from comment #23)
> Is there a particular reason for this defensiveness? Why bother null testing
> and asserting if the session really can't be null?

It shouldn't be null.  Some of these could be hit in rare occasions when something is starting to load right as a document/frame/page is being torn down, and there's no reason to crash if that happens.  I've fixed many such null-dereferencing crashes in the loader and I'm preemptively fixing them now.
Comment 25 Alex Christensen 2019-01-15 11:52:48 PST
Created attachment 359186 [details]
Patch
Comment 26 EWS Watchlist 2019-01-15 13:05:22 PST
Comment on attachment 359186 [details]
Patch

Attachment 359186 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10762656

New failing tests:
imported/w3c/web-platform-tests/xhr/open-url-multi-window-3.htm
fast/xsl/transform-xhr-doc.xhtml
Comment 27 EWS Watchlist 2019-01-15 13:05:24 PST
Created attachment 359195 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 28 EWS Watchlist 2019-01-15 13:20:50 PST
Comment on attachment 359186 [details]
Patch

Attachment 359186 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10762698

New failing tests:
imported/w3c/web-platform-tests/xhr/open-url-multi-window-3.htm
media/track/track-remove-crash.html
fast/xsl/transform-xhr-doc.xhtml
Comment 29 EWS Watchlist 2019-01-15 13:20:52 PST
Created attachment 359197 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 30 EWS Watchlist 2019-01-15 13:51:24 PST
Comment on attachment 359186 [details]
Patch

Attachment 359186 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10762748

New failing tests:
media/track/regions-webvtt/vtt-region-parser.html
imported/w3c/web-platform-tests/xhr/open-url-multi-window-3.htm
fast/xsl/transform-xhr-doc.xhtml
Comment 31 EWS Watchlist 2019-01-15 13:51:26 PST
Created attachment 359201 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 32 Alex Christensen 2019-01-15 14:11:31 PST
Created attachment 359203 [details]
Patch
Comment 33 Alex Christensen 2019-01-15 14:23:04 PST
Created attachment 359204 [details]
patch
Comment 34 Alex Christensen 2019-01-15 14:39:04 PST
Created attachment 359206 [details]
Patch
Comment 35 Alex Christensen 2019-01-15 15:31:49 PST
http://trac.webkit.org/r240014