Summary: | Storage Access API: Implement frame-specific access in the document.cookie layer | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||||||||||||||||||||||||||
Component: | WebKit2 | Assignee: | John Wilander <wilander> | ||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, berto, cdumez, cgarcia, commit-queue, dbates, esprehn+autocc, ews-watchlist, galpeter, gustavo, Hironori.Fujii, japhet, kangil.han, mcatanzaro, rniwa, webkit-bug-importer | ||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=184346 | ||||||||||||||||||||||||||||||||
Attachments: |
|
Description
John Wilander
2017-12-11 16:40:24 PST
Created attachment 329244 [details]
Patch
Uploading for early patch feedback and EWS results. Created attachment 329245 [details]
Patch
Comment on attachment 329245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329245&action=review Don't break the linux build. Otherwise r=me > Source/WebCore/platform/network/CacheValidation.cpp:351 > + return cookieRequestHeaderFieldValue(NetworkStorageSession::defaultStorageSession(), request.firstPartyForCookies(), request.url(), std::nullopt, std::nullopt, includeSecureCookies).first; We should look into passing in real values here. > Source/WebKit/Shared/mac/CookieStorageShim.mm:64 > + if (!WebProcess::singleton().ensureNetworkProcessConnection().connection().sendSync(Messages::NetworkConnectionToWebProcess::CookieRequestHeaderFieldValue(PAL::SessionID::defaultSessionID(), firstPartyForCookiesURL, inRequestURL, std::nullopt, std::nullopt, includeSecureCookies), Messages::NetworkConnectionToWebProcess::CookieRequestHeaderFieldValue::Reply(cookies, secureCookiesAccessed), 0)) This is no big deal. I filed https://bugs.webkit.org/show_bug.cgi?id=180766 to get rid of this code on all platforms where we have ITP. Comment on attachment 329245 [details] Patch Attachment 329245 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5649060 Number of test failures exceeded the failure limit. Created attachment 329253 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329245 [details] Patch Attachment 329245 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5649088 Number of test failures exceeded the failure limit. Created attachment 329256 [details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329245 [details] Patch Attachment 329245 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5649188 New failing tests: http/tests/websocket/tests/hybi/workers/worker-reload.html Created attachment 329258 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 329270 [details]
Patch
Created attachment 329278 [details]
Patch
WebSockets may not have a frame so I made CookieJar.cpp check for the existence of a frame and if there is none, call with std::nullopt for frameID and pageID. Comment on attachment 329278 [details] Patch Attachment 329278 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5651778 Number of test failures exceeded the failure limit. Created attachment 329290 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 329291 [details]
Patch
Comment on attachment 329291 [details] Patch Attachment 329291 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5652637 Number of test failures exceeded the failure limit. Created attachment 329299 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 329302 [details]
Patch
Introduced FrameLoaderClient::hasIDs() to be able to check whether the client has IDs available or not. Otherwise I hit RELEASE_ASSERT_NOT_REACHED() calls in WebKitLegacy. Comment on attachment 329302 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329302&action=review I think it's time to introduce a new structure that contains two std::optional<uint64_t>, and maybe some URLs, too. > Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:204 > +bool WebFrameLoaderClient::hasIDs() const I think returning std::nullopt from WebFrameLoaderClient::pageID is cleaner than this. I don't think we have a good reason to RELEASE_ASSERT_NOT_REACHED when they're called other than that we had no legitimate use of pageID in WebCore before this, and now we do. Returning std::nullopt is the correct WebKitLegacy behavior for this. I'd be open if someone feels differently. Created attachment 329379 [details]
Patch
(In reply to Alex Christensen from comment #22) > Comment on attachment 329302 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=329302&action=review > > I think it's time to introduce a new structure that contains two > std::optional<uint64_t>, and maybe some URLs, too. I will first see if I can get this working. Then either update this patch to do it in a follow-up patch. > > Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:204 > > +bool WebFrameLoaderClient::hasIDs() const > > I think returning std::nullopt from WebFrameLoaderClient::pageID is cleaner > than this. I don't think we have a good reason to > RELEASE_ASSERT_NOT_REACHED when they're called other than that we had no > legitimate use of pageID in WebCore before this, and now we do. Returning > std::nullopt is the correct WebKitLegacy behavior for this. I'd be open if > someone feels differently. OK. This is what Tim Horton suggested too. The new patch does this. Thanks! Created attachment 329391 [details]
Patch
Fix for the Windows ports. Comment on attachment 329391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=329391&action=review > Source/WebCore/loader/EmptyFrameLoaderClient.h:36 > + std::optional<uint64_t> frameID() const override { return 0; } > + std::optional<uint64_t> pageID() const override { return 0; } These should probably return nullopt (In reply to Alex Christensen from comment #27) > Comment on attachment 329391 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=329391&action=review > > > Source/WebCore/loader/EmptyFrameLoaderClient.h:36 > > + std::optional<uint64_t> frameID() const override { return 0; } > > + std::optional<uint64_t> pageID() const override { return 0; } > > These should probably return nullopt Thanks, Alex! Will fix. Created attachment 329402 [details]
Patch for landing
Comment on attachment 329402 [details] Patch for landing Clearing flags on attachment: 329402 Committed r225934: <https://trac.webkit.org/changeset/225934> All reviewed patches have been landed. Closing bug. A lot of crashed on AppleWin LayoutTests. I'm going to fix it in Bug 180926. |