Bug 200642 – Resource Load Statistics: Switch NSURLSession on top navigation to prevalent resource with user interaction

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

Since prevalent resources with user interaction get to keep their cookies and website data, we should use a different NSURLSessions for when they are first-party websites and have access to that data.

[John Wilander]

<rdar://problem/53962073>

[John Wilander]

Created attachment 376094 [details]
Patch

[John Wilander]

Created attachment 376097 [details]
Patch

[John Wilander]

Trying to address the build error. Strangely, I didn't get that error locally. Must be some flag I'm missing.

[Chris Dumez]

Comment on attachment 376094 [details]
Patch

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

Alex should probably take a look but here are some drive-by comments.

> Source/WebCore/page/RuntimeEnabledFeatures.h:150
> +    void setITPSessionSwitchingEnabled(bool isEnabled) { m_isITPSessionSwitchingEnabled = isEnabled; }

setIsITPSessionSwitchingEnabled()

> Source/WebCore/page/RuntimeEnabledFeatures.h:151
> +    bool itpSessionSwitchingEnabled() const { return m_isITPSessionSwitchingEnabled; }

isITPSessionSwitchingEnabled()

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:101
> +    void updateCookieBlockingForDomains(RegistrableDomainsToBlockCookiesFor&, CompletionHandler<void()>&&);

any reason the parameter is no long const?

> Source/WebKit/NetworkProcess/NetworkProcess.h:263
> +    void hasIsolatedSession(PAL::SessionID, const WebCore::RegistrableDomain, CompletionHandler<void(bool)>&&) const;

const WebCore::RegistrableDomain& (missing the &)

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:86
> +    RetainPtr<NSURLSession> session(WebCore::StoredCredentialsPolicy);

Why does this return a RetainPtr and not a NSURLSession* ? Are we transferring ownership?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:87
> +    RetainPtr<NSURLSession> isolatedSession(WebCore::StoredCredentialsPolicy, const WebCore::RegistrableDomain);

ditto.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1090
> +    auto entry = m_isolatedSessions.ensure(firstPartyDomain, [this, weakThis = makeWeakPtr(this)] {

No weakThis here, this is HashMap::ensure() is not asynchronous. Just capture this.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1093
> +            newEntry.sessionWithCredentialStorageDelegate = adoptNS([[WKNetworkSessionDelegate alloc] initWithNetworkSession:*this withCredentials:true]);

Can probably be written like so:
return IsolatedSession {
   a,
   b,
   c,
   d
};

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1107
> +        for (WebCore::RegistrableDomain key : m_isolatedSessions.keys()) {

auto& key (to copying refCounting churn).

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1155
> +    for (IsolatedSession session : m_isolatedSessions.values()) {

auto& session ?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1171
> +    for (SessionEntry entry : m_isolatedSessions.values())

auto& entry?

[Alex Christensen]

Comment on attachment 376094 [details]
Patch

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

> LayoutTests/http/tests/resourceLoadStatistics/switch-session-on-navigation-to-prevalent-with-interaction-expected.txt:7
> +PASS Should have and has the persistent cookie.

Are there cases we could test where the cookie should not be there?

[John Wilander]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 376094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376094&action=review
> 
> Alex should probably take a look but here are some drive-by comments.
> 
> > Source/WebCore/page/RuntimeEnabledFeatures.h:150
> > +    void setITPSessionSwitchingEnabled(bool isEnabled) { m_isITPSessionSwitchingEnabled = isEnabled; }
> 
> setIsITPSessionSwitchingEnabled()

For consistency or is it a style rule? I'll make the change.

> > Source/WebCore/page/RuntimeEnabledFeatures.h:151
> > +    bool itpSessionSwitchingEnabled() const { return m_isITPSessionSwitchingEnabled; }
> 
> isITPSessionSwitchingEnabled()

Ditto.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:101
> > +    void updateCookieBlockingForDomains(RegistrableDomainsToBlockCookiesFor&, CompletionHandler<void()>&&);
> 
> any reason the parameter is no long const?

First I was "How can it be a long?" but then I realized what you're saying. It should be const. Will fix.

> > Source/WebKit/NetworkProcess/NetworkProcess.h:263
> > +    void hasIsolatedSession(PAL::SessionID, const WebCore::RegistrableDomain, CompletionHandler<void(bool)>&&) const;
> 
> const WebCore::RegistrableDomain& (missing the &)

Will fix.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:86
> > +    RetainPtr<NSURLSession> session(WebCore::StoredCredentialsPolicy);
> 
> Why does this return a RetainPtr and not a NSURLSession* ? Are we
> transferring ownership?

You're probably right. This was to make the call site work as before. I wonder if I'll have to have two paths there if I make this a regular pointer.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:87
> > +    RetainPtr<NSURLSession> isolatedSession(WebCore::StoredCredentialsPolicy, const WebCore::RegistrableDomain);
> 
> ditto.
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1090
> > +    auto entry = m_isolatedSessions.ensure(firstPartyDomain, [this, weakThis = makeWeakPtr(this)] {
> 
> No weakThis here, this is HashMap::ensure() is not asynchronous. Just
> capture this.

Got it. Will fix.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1093
> > +            newEntry.sessionWithCredentialStorageDelegate = adoptNS([[WKNetworkSessionDelegate alloc] initWithNetworkSession:*this withCredentials:true]);
> 
> Can probably be written like so:
> return IsolatedSession {
>    a,
>    b,
>    c,
>    d
> };

I need at least the delegates as local variables since they are used to create the sessions.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1107
> > +        for (WebCore::RegistrableDomain key : m_isolatedSessions.keys()) {
> 
> auto& key (to copying refCounting churn).

Will fix.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1155
> > +    for (IsolatedSession session : m_isolatedSessions.values()) {
> 
> auto& session ?

Will fix.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1171
> > +    for (SessionEntry entry : m_isolatedSessions.values())
> 
> auto& entry?

Will fix.

Thanks, Chris!

[John Wilander]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 376094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376094&action=review
> 
> > LayoutTests/http/tests/resourceLoadStatistics/switch-session-on-navigation-to-prevalent-with-interaction-expected.txt:7
> > +PASS Should have and has the persistent cookie.
> 
> Are there cases we could test where the cookie should not be there?

I don't think so. I put these checks in here to a) make sure I knew what was affected by the change and b) to make sure this behavior is not changed underneath us at some later point. It would be a subtle regression.

[John Wilander]

Created attachment 376107 [details]
Patch

[John Wilander]

Hmm … I wonder if the pointers need to be ref counted? Because some task(s) might still be using the session when it’s dropped from the map.

[John Wilander]

(In reply to John Wilander from comment #10)
> Hmm … I wonder if the pointers need to be ref counted? Because some task(s)
> might still be using the session when it’s dropped from the map.

Discussed with Chris Dumez. We don't need to ref count it since that's handled underneath us.

[Alex Christensen]

Comment on attachment 376107 [details]
Patch

r=me

[John Wilander]

(In reply to Alex Christensen from comment #12)
> Comment on attachment 376107 [details]
> Patch
> 
> r=me

Thank you, Alex!

[WebKit Commit Bot]

Comment on attachment 376107 [details]
Patch

Clearing flags on attachment: 376107

Committed r248640: <https://trac.webkit.org/changeset/248640>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.