Created attachment 396031 [details]
Patch

<rdar://problem/59854450>

Created attachment 396791 [details]
Patch

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.

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

Are the EWS failures real?

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

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

Yes, it will definitely need such perf analysis.

Created attachment 397148 [details]
Patch

Comment on attachment 397148 [details]
Patch

closer but still not there.

Created attachment 397235 [details]
Patch

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