Summary: | Stop using CookiesStrategy | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | 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
Alex Christensen
2016-08-23 14:46:40 PDT
Created attachment 286781 [details]
Patch
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 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 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. This needs to be split into two functions, one for the web process and one for the network process Created attachment 286899 [details]
Patch
Created attachment 286905 [details]
Patch
Created attachment 286910 [details]
Patch
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 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
Created attachment 359037 [details]
Patch
Created attachment 359039 [details]
Patch
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? (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 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. Created attachment 359042 [details]
Patch
Created attachment 359051 [details]
Patch
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 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 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 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
(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 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. (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. Created attachment 359186 [details]
Patch
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 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 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 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 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 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
Created attachment 359203 [details]
Patch
Created attachment 359204 [details]
patch
Created attachment 359206 [details]
Patch
|