Bug 203934 – Re-enable NSURLSession isolation after r252116

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Re-enable NSURLSession isolation after r252116

[Alex Christensen]

Created attachment 382991 [details]
Patch

[Alex Christensen]

<rdar://problem/56921584>

[John Wilander]

Comment on attachment 382991 [details]
Patch

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

Nice! See my comments. I'd like someone else to have a look to, who's more familiar with task identifiers.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:122
> +    SessionWrapper m_ephemeralStatelessSession;

To make sure I understand, you're changing it so that stateless is now WithoutCredentialStorage and ephemeralStatelessCookieless is now ephemeralStateless?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:544
> +            ASSERT_NOT_REACHED();

This we need to test manually after you land since layout tests don't support HSTS.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1140
> +    m_ephemeralStatelessSession.initialize(configuration, *this, WebCore::StoredCredentialsPolicy::EphemeralStatelessCookieless);

Maybe we should rename StoredCredentialsPolicy::EphemeralStatelessCookieless to StoredCredentialsPolicy::EphemeralStateless to match the member naming?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1144
>  {

#if ENABLE(RESOURCE_LOAD_STATISTICS)

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1150
> +        ASSERT_NOT_REACHED();

#endif

[Chris Dumez]

I refactored this code quite a bit after the huge regression so your patch definitely will not apply.

[Alex Christensen]

Created attachment 382999 [details]
Patch

[Alex Christensen]

(In reply to John Wilander from comment #3)
> Comment on attachment 382991 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382991&action=review
> 
> Nice! See my comments. I'd like someone else to have a look to, who's more
> familiar with task identifiers.
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:122
> > +    SessionWrapper m_ephemeralStatelessSession;
> 
> To make sure I understand, you're changing it so that stateless is now
> WithoutCredentialStorage and ephemeralStatelessCookieless is now
> ephemeralStateless?
Correct.  Stateless implies cookieless, and what we called "stateless" before was leftover from when we tried to make it actually stateless and learned that it needs to be just without credentials.

[Alex Christensen]

Created attachment 383002 [details]
Patch

[Alex Christensen]

Created attachment 383004 [details]
Patch

[Chris Dumez]

Comment on attachment 383004 [details]
Patch

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

> Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:76
> +    cocoaSession.m_sessionWithCredentialStorage.downloadMap.add(taskIdentifier, m_downloadID);

I think we should avoid interacting with network session data members directly, this is bad practice.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:66
> +    friend class Download;

Let's avoid this. I think we should strive to remove friends, not add more.

[Chris Dumez]

Comment on attachment 383004 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:54
> +    WeakPtr<NetworkSessionCocoa> sessionCocoa;

Including this here seems wasteful:
- NetworkDataTaskCocoa already has access to this via m_session.
- This IsolatedSession struct has 2 SessionWrapper members, so ends up with 2 identical pointers to the NetworkSessionCocoa.

I think we should extra this member out.

[Chris Dumez]

Comment on attachment 383004 [details]
Patch

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

>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:54
>> +    WeakPtr<NetworkSessionCocoa> sessionCocoa;
> 
> Including this here seems wasteful:
> - NetworkDataTaskCocoa already has access to this via m_session.
> - This IsolatedSession struct has 2 SessionWrapper members, so ends up with 2 identical pointers to the NetworkSessionCocoa.
> 
> I think we should extra this member out.

No to mention that the NetworkSessionCocoa has these data members:
SessionWrapper m_sessionWithCredentialStorage;
SessionWrapper m_sessionWithoutCredentialStorage;
SessionWrapper m_ephemeralStatelessSession;

So ends up with 3 unnecessary WeakPtrs to itself.

[Chris Dumez]

Comment on attachment 383004 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:537
> +        if (auto sessionCocoa = _sessionWrapper ? _sessionWrapper->sessionCocoa.get() : nullptr) {

Could use [self sessionCocoa].

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:573
> +        if (auto sessionCocoa = _sessionWrapper ? _sessionWrapper->sessionCocoa.get() : nullptr) {

Could use [self sessionCocoa].

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:897
> +    ASSERT(!_sessionWrapper->downloadMap.contains(downloadTask.taskIdentifier));

RELEASE_ASSERT ?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1370
> +    ASSERT(!m_sessionWithCredentialStorage.webSocketDataTaskMap.contains(task.identifier()));

RELEASE_ASSERT?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1376
> +    ASSERT(m_sessionWithCredentialStorage.webSocketDataTaskMap.contains(task.identifier()));

RELEASE_ASSERT?

[Alex Christensen]

Comment on attachment 383004 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:54
>>> +    WeakPtr<NetworkSessionCocoa> sessionCocoa;
>> 
>> Including this here seems wasteful:
>> - NetworkDataTaskCocoa already has access to this via m_session.
>> - This IsolatedSession struct has 2 SessionWrapper members, so ends up with 2 identical pointers to the NetworkSessionCocoa.
>> 
>> I think we should extra this member out.
> 
> No to mention that the NetworkSessionCocoa has these data members:
> SessionWrapper m_sessionWithCredentialStorage;
> SessionWrapper m_sessionWithoutCredentialStorage;
> SessionWrapper m_ephemeralStatelessSession;
> 
> So ends up with 3 unnecessary WeakPtrs to itself.

These WeakPtrs are actually necessary right now to get from the download delegate callbacks to the NetworkSession because Downloads are owned by the DownloadManager and only have an identifier, no reference to their NetworkSession.  We could fix this in a further refactoring, but I think that should not be in the same patch as the rest of this.  I'll put a fixme for now.

[Alex Christensen]

Created attachment 383043 [details]
Patch

[Chris Dumez]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 383004 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383004&action=review
> 
> >>> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:54
> >>> +    WeakPtr<NetworkSessionCocoa> sessionCocoa;
> >> 
> >> Including this here seems wasteful:
> >> - NetworkDataTaskCocoa already has access to this via m_session.
> >> - This IsolatedSession struct has 2 SessionWrapper members, so ends up with 2 identical pointers to the NetworkSessionCocoa.
> >> 
> >> I think we should extra this member out.
> > 
> > No to mention that the NetworkSessionCocoa has these data members:
> > SessionWrapper m_sessionWithCredentialStorage;
> > SessionWrapper m_sessionWithoutCredentialStorage;
> > SessionWrapper m_ephemeralStatelessSession;
> > 
> > So ends up with 3 unnecessary WeakPtrs to itself.
> 
> These WeakPtrs are actually necessary right now to get from the download
> delegate callbacks to the NetworkSession because Downloads are owned by the
> DownloadManager and only have an identifier, no reference to their
> NetworkSession.  We could fix this in a further refactoring, but I think
> that should not be in the same patch as the rest of this.  I'll put a fixme
> for now.

Whoever needs it, can store it separately. It does not need and should not be in this structure.

[Chris Dumez]

Comment on attachment 383043 [details]
Patch

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

> Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:73
> +    m_downloadTask = [cocoaSession.sessionWrapperForDownloads().session downloadTaskWithResumeData:updatedData];

Download seems to have access to its session here, so what do you mean?

I don’t think there is a rush to re-land me so I do not understand the need to land a FIXME.

[Alex Christensen]

Created attachment 383046 [details]
Patch

[Alex Christensen]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 383043 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383043&action=review
> 
> > Source/WebKit/NetworkProcess/Downloads/cocoa/DownloadCocoa.mm:73
> > +    m_downloadTask = [cocoaSession.sessionWrapperForDownloads().session downloadTaskWithResumeData:updatedData];
> 
> Download seems to have access to its session here, so what do you mean?
Once we have a Download that's easy.  It's getting from the WKNetworkSessionDelegate to the Download that's hard because we only have a DownloadID, and the Downloads must be gotten through the DownloadManager right now.  I fixed it by adding a WeakPtr<NetworkProcess> in DownloadData that is made only when we make a Download and used only for Downloads.  I think that was a lot of work just to remove the 3-WeakPtr initialization cost addition to creating a NetworkSession (which is a heavy object).  I don't think it was worth that work right now, but it's done and ready for review again.

[Chris Dumez]

Comment on attachment 383046 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:399
> +    WeakPtr<WebKit::SessionWrapper> _sessionWrapper;

I think this should have a WeakPtr to the NetworkSessionCocoa and another WeakPtr to the sessionWrapper.
Then you can get the network process from the NetworkSessionCocoa, like we used to. And you don't need to have a WeakPtr to the NetworkProcess in the DownloadData.

[Alex Christensen]

Created attachment 383053 [details]
Patch

[Chris Dumez]

Comment on attachment 383053 [details]
Patch

r=me

[Alex Christensen]

Created attachment 383054 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r252185