Bug 180682 – Storage Access API: Implement frame-specific access in the document.cookie layer

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

[John Wilander]

This is part 3 of making Storage Access API frame-specific.

[Radar WebKit Bug Importer]

<rdar://problem/35982257>

[John Wilander]

Created attachment 329244 [details]
Patch

[John Wilander]

Uploading for early patch feedback and EWS results.

[John Wilander]

Created attachment 329245 [details]
Patch

[Alex Christensen]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[John Wilander]

Created attachment 329270 [details]
Patch

[John Wilander]

Created attachment 329278 [details]
Patch

[John Wilander]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[John Wilander]

Created attachment 329291 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[John Wilander]

Created attachment 329302 [details]
Patch

[John Wilander]

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.

[Alex Christensen]

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.

[John Wilander]

Created attachment 329379 [details]
Patch

[John Wilander]

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

[John Wilander]

Created attachment 329391 [details]
Patch

[John Wilander]

Fix for the Windows ports.

[Alex Christensen]

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

[John Wilander]

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

[John Wilander]

Created attachment 329402 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 329402 [details]
Patch for landing

Clearing flags on attachment: 329402

Committed r225934: <https://trac.webkit.org/changeset/225934>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

A lot of crashed on AppleWin LayoutTests. I'm going to fix it in Bug 180926.