Bug 210313 – Use NetworkSessionCocoa::isolatedSession for all loads

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Alex Christensen]

Use NetworkSessionCocoa::isolatedSession for all loads

[Alex Christensen]

Created attachment 396031 [details]
Patch

[Alex Christensen]

<rdar://problem/59854450>

[Alex Christensen]

Created attachment 396791 [details]
Patch

[John Wilander]

Comment on attachment 396791 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2663
> +    completionHandler(true);

Is this something we can get rid of all together?

> Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:79
> +    // ALSO: We should double check that removing an isolated session in NetworkSessionCocoa::isolatedSession with an active download doesn't stop that download.

Could isolatedSessions be told that they're being used for a download? If so, we need to think about abuse cases:
1. A website deliberately starts an infinite download only to keep its isolated session.
2. A website makes a set of redirects to start infinite downloads for isolated sessions in order to consume resources, potentially exhausting the available isolated sessions.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1198
> +    auto firstParty = WebCore::RegistrableDomain(request.firstPartyForCookies());

We really should make request.firstPartyForCookies() return a RegistrableDomain. I believe that's the only way it's used. If not, we should have a lazy function to get the RegistrableDomain. This conversion is done too often.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1204
> +    return isolatedSession(WebCore::StoredCredentialsPolicy::Use, WebCore::RegistrableDomain(url), NavigatingToAppBoundDomain::No);

Could this be the place to pass on info about being used for a download? I assume we need to consider scenarios where more than one download is ongoing and all of them must be completed before we discard the isolated session.

[Alex Christensen]

(In reply to John Wilander from comment #4)
> Comment on attachment 396791 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396791&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2663
> > +    completionHandler(true);
> 
> Is this something we can get rid of all together?
Yes.  I was just seeing what tests I need to work on with this last patch.

> > Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:79
> > +    // ALSO: We should double check that removing an isolated session in NetworkSessionCocoa::isolatedSession with an active download doesn't stop that download.
> 
> Could isolatedSessions be told that they're being used for a download? If
> so, we need to think about abuse cases:
> 1. A website deliberately starts an infinite download only to keep its
> isolated session.
> 2. A website makes a set of redirects to start infinite downloads for
> isolated sessions in order to consume resources, potentially exhausting the
> available isolated sessions.
A download would only keep the NSURLSession alive, not the isolatedSession.  I don't think this is an issue.

[Geoffrey Garen]

Are the EWS failures real?

[Alex Christensen]

yes, I'm working on fixing them and finishing this right now.

[Geoffrey Garen]

Also: Can you submit a PLT A/B test with this change?

[Alex Christensen]

Yes, it will definitely need such perf analysis.

[Alex Christensen]

Created attachment 397148 [details]
Patch

[Alex Christensen]

Comment on attachment 397148 [details]
Patch

closer but still not there.

[Alex Christensen]

Created attachment 397235 [details]
Patch

[Alex Christensen]

*** This bug has been marked as a duplicate of bug 230750 ***