Bug 200642 - Resource Load Statistics: Switch NSURLSession on top navigation to prevalent resource with user interaction
Summary: Resource Load Statistics: Switch NSURLSession on top navigation to prevalent ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-12 14:16 PDT by John Wilander
Modified: 2019-08-13 15:34 PDT (History)
5 users (show)

See Also:


Attachments
Patch (82.11 KB, patch)
2019-08-12 14:35 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (82.42 KB, patch)
2019-08-12 15:02 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (82.56 KB, patch)
2019-08-12 16:53 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-08-12 14:16:19 PDT
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.
Comment 1 John Wilander 2019-08-12 14:16:41 PDT
<rdar://problem/53962073>
Comment 2 John Wilander 2019-08-12 14:35:24 PDT
Created attachment 376094 [details]
Patch
Comment 3 John Wilander 2019-08-12 15:02:10 PDT
Created attachment 376097 [details]
Patch
Comment 4 John Wilander 2019-08-12 15:03:31 PDT
Trying to address the build error. Strangely, I didn't get that error locally. Must be some flag I'm missing.
Comment 5 Chris Dumez 2019-08-12 15:06:25 PDT
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?
Comment 6 Alex Christensen 2019-08-12 15:38:00 PDT
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?
Comment 7 John Wilander 2019-08-12 15:43:08 PDT
(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!
Comment 8 John Wilander 2019-08-12 15:44:20 PDT
(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.
Comment 9 John Wilander 2019-08-12 16:53:26 PDT
Created attachment 376107 [details]
Patch
Comment 10 John Wilander 2019-08-13 08:39:17 PDT
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.
Comment 11 John Wilander 2019-08-13 09:19:00 PDT
(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.
Comment 12 Alex Christensen 2019-08-13 13:25:22 PDT
Comment on attachment 376107 [details]
Patch

r=me
Comment 13 John Wilander 2019-08-13 14:14:07 PDT
(In reply to Alex Christensen from comment #12)
> Comment on attachment 376107 [details]
> Patch
> 
> r=me

Thank you, Alex!
Comment 14 WebKit Commit Bot 2019-08-13 15:34:36 PDT
Comment on attachment 376107 [details]
Patch

Clearing flags on attachment: 376107

Committed r248640: <https://trac.webkit.org/changeset/248640>
Comment 15 WebKit Commit Bot 2019-08-13 15:34:38 PDT
All reviewed patches have been landed.  Closing bug.