Bug 161106 – Stop using CookiesStrategy

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Stop using CookiesStrategy

[Alex Christensen]

Created attachment 286781 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Alex Christensen]

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.

[Alex Christensen]

This needs to be split into two functions, one for the web process and one for the network process

[Alex Christensen]

Created attachment 286899 [details]
Patch

[Alex Christensen]

Created attachment 286905 [details]
Patch

[Alex Christensen]

Created attachment 286910 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Alex Christensen]

Created attachment 359037 [details]
Patch

[Alex Christensen]

Created attachment 359039 [details]
Patch

[Don Olmstead]

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?

[Don Olmstead]

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

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 359042 [details]
Patch

[Alex Christensen]

Created attachment 359051 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alex Christensen]

(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 :/

[Antti Koivisto]

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.

[Alex Christensen]

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

[Alex Christensen]

Created attachment 359186 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 359203 [details]
Patch

[Alex Christensen]

Created attachment 359204 [details]
patch

[Alex Christensen]

Created attachment 359206 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r240014