Re-enable NSURLSession isolation after r252116
Created attachment 382991 [details] Patch
<rdar://problem/56921584>
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
I refactored this code quite a bit after the huge regression so your patch definitely will not apply.
Created attachment 382999 [details] Patch
(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.
Created attachment 383002 [details] Patch
Created attachment 383004 [details] Patch
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.
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.
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.
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?
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.
Created attachment 383043 [details] Patch
(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.
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.
Created attachment 383046 [details] Patch
(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.
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.
Created attachment 383053 [details] Patch
Comment on attachment 383053 [details] Patch r=me
Created attachment 383054 [details] Patch
http://trac.webkit.org/r252185