document.cookie should not do a sync IPC to the network process for iframes that do not have storage access

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

document.cookie should not do a sync IPC to the network process for iframes that do not have storage access.

[Chris Dumez]

Created attachment 387397 [details]
WIP Patch

[Chris Dumez]

Created attachment 387535 [details]
WIP Patch

[Chris Dumez]

Created attachment 387567 [details]
WIP Patch

[Chris Dumez]

Created attachment 387593 [details]
WIP Patch

Almost ready.

[Chris Dumez]

Created attachment 387681 [details]
Patch

[Chris Dumez]

Created attachment 387684 [details]
Patch

[Geoffrey Garen]

Comment on attachment 387684 [details]
Patch

r=me

[John Wilander]

Comment on attachment 387684 [details]
Patch

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

Thanks for fixing the preferences read and the improvements to when blocking is done and to test async behavior. See inline comments.

> Source/WebKit/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

I thought we always described the change in both the WebCore and the WebKit change logs, at least when there are significant changes in both.

> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:65
> +        return false;

Does this match some existing behavior?

> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:69
> +        return false;

What does this imply? Can there be cookies for empty resource domains?

> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:108
> +    if (!WebProcess::singleton().ensureNetworkProcessConnection().connection().sendSync(Messages::NetworkConnectionToWebProcess::CookiesForDOM(document.firstPartyForCookies(), sameSiteInfo(document), url, frameID, pageID, shouldIncludeSecureCookies(document, url), delegatesCookieBlockingToNetworkProcess ? ShouldApplyITPCookieBlockingPolicy::Yes : ShouldApplyITPCookieBlockingPolicy::No), Messages::NetworkConnectionToWebProcess::CookiesForDOM::Reply(cookieString, secureCookiesAccessed), 0))

Now that I see the use of ShouldApplyITPCookieBlockingPolicy, I think a more detailed enum is called for, such as { IsFirstParty, IsThirdPartyWithStorageAccess, IsThirdPartyWithoutStorageAccess, IsMissingPartyContext }, the last value being my attempt to encode the two cases with empty domains as commented on above. Then ITP in the network process can make an informed decision instead of being served a decision Apply policy Yes or No.

Such an enum would also make sense in both places instead of this delegatesCookieBlockingToNetworkProcess boolean plus the Yes/No enum.

[Chris Dumez]

Comment on attachment 387684 [details]
Patch

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

>> Source/WebKit/ChangeLog:6
>> +        Reviewed by NOBODY (OOPS!).
> 
> I thought we always described the change in both the WebCore and the WebKit change logs, at least when there are significant changes in both.

Not sure what the policy is. I can add the changelog to both WebKit and WebCore.

>> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:65
>> +        return false;
> 
> Does this match some existing behavior?

This logic is identical to what is used in NetworkStorageSession::shouldBlockCookies() on network process side.

>> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:69
>> +        return false;
> 
> What does this imply? Can there be cookies for empty resource domains?

This logic is identical to what is used in NetworkStorageSession::shouldBlockCookies() on network process side.

>> Source/WebKit/WebProcess/WebPage/WebCookieJar.cpp:108
>> +    if (!WebProcess::singleton().ensureNetworkProcessConnection().connection().sendSync(Messages::NetworkConnectionToWebProcess::CookiesForDOM(document.firstPartyForCookies(), sameSiteInfo(document), url, frameID, pageID, shouldIncludeSecureCookies(document, url), delegatesCookieBlockingToNetworkProcess ? ShouldApplyITPCookieBlockingPolicy::Yes : ShouldApplyITPCookieBlockingPolicy::No), Messages::NetworkConnectionToWebProcess::CookiesForDOM::Reply(cookieString, secureCookiesAccessed), 0))
> 
> Now that I see the use of ShouldApplyITPCookieBlockingPolicy, I think a more detailed enum is called for, such as { IsFirstParty, IsThirdPartyWithStorageAccess, IsThirdPartyWithoutStorageAccess, IsMissingPartyContext }, the last value being my attempt to encode the two cases with empty domains as commented on above. Then ITP in the network process can make an informed decision instead of being served a decision Apply policy Yes or No.
> 
> Such an enum would also make sense in both places instead of this delegatesCookieBlockingToNetworkProcess boolean plus the Yes/No enum.

The logic for checking is the same on the network process and WebProcess side. A yes/no bit is thus enough to communicate to the network process that we already did the check.

[Chris Dumez]

Created attachment 387730 [details]
Patch

[John Wilander]

Comment on attachment 387730 [details]
Patch

The shouldAskITPInNetworkProcess boolean and the ShouldAskITP enum are both good improvements. I assume you are waiting for EWS before you add cq+. Regardless, I'm OK for it to go on the queue.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 387730 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 387730 [details]
Patch

Clearing flags on attachment: 387730

Committed r254556: <https://trac.webkit.org/changeset/254556>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58592851>