Correctly keep track of NetworkDataTasks with and without credentials when using...
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Mar 2016 22:13:00 +0000 (22:13 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Mar 2016 22:13:00 +0000 (22:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=154876

Reviewed by Brady Eidson.

I was seeing an assertion failure from ASSERT(!m_session.m_dataTaskMap.contains(taskIdentifier()))
in the NetworkDataTask constructor sometimes.  This is because a task identifier is not enough information
to uniquely find a NetworkDataTask in a NetworkSession since r196034 because there are two NSURLSessions
in a NetworkSession, one with credentials and one without.  The assertion would fire in a case like if we
made the first NetworkDataTask with credentials (taskIdentifier is 1) and the first NetworkDataTask
without credentials before the first NetworkDataTask with credentials was finished.  In that case, the
taskIdentifier would also be 1, which would conflict with the other taskIdentifier.  That taskIdentifier
would uniquely identify the task in the correct NSURLSession, though, so the solution is to keep a map
for each NSURLSession in the NetworkSession.

* NetworkProcess/NetworkDataTask.h:
* NetworkProcess/NetworkSession.h:
* NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:
(WebKit::NetworkDataTask::NetworkDataTask):
(WebKit::NetworkDataTask::~NetworkDataTask):
(WebKit::NetworkDataTask::suspend):
(WebKit::serverTrustCredential):
* NetworkProcess/cocoa/NetworkSessionCocoa.mm:
(-[WKNetworkSessionDelegate URLSession:task:didSendBodyData:totalBytesSent:totalBytesExpectedToSend:]):
(-[WKNetworkSessionDelegate URLSession:task:willPerformHTTPRedirection:newRequest:completionHandler:]):
(-[WKNetworkSessionDelegate URLSession:task:didReceiveChallenge:completionHandler:]):
(-[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:]):
(-[WKNetworkSessionDelegate URLSession:dataTask:didReceiveResponse:completionHandler:]):
(-[WKNetworkSessionDelegate URLSession:dataTask:didReceiveData:]):
(-[WKNetworkSessionDelegate URLSession:downloadTask:didWriteData:totalBytesWritten:totalBytesExpectedToWrite:]):
(-[WKNetworkSessionDelegate URLSession:dataTask:didBecomeDownloadTask:]):
(WebKit::NetworkSession::clearCredentials):
(WebKit::NetworkSession::dataTaskForIdentifier):
(WebKit::NetworkSession::addDownloadID):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@197413 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit2/ChangeLog
Source/WebKit2/NetworkProcess/NetworkDataTask.h
Source/WebKit2/NetworkProcess/NetworkSession.h
Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm
Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm

index 110763a..986911d 100644 (file)
@@ -1,3 +1,40 @@
+2016-03-01  Alex Christensen  <achristensen@webkit.org>
+
+        Correctly keep track of NetworkDataTasks with and without credentials when using NetworkSession
+        https://bugs.webkit.org/show_bug.cgi?id=154876
+
+        Reviewed by Brady Eidson.
+
+        I was seeing an assertion failure from ASSERT(!m_session.m_dataTaskMap.contains(taskIdentifier()))
+        in the NetworkDataTask constructor sometimes.  This is because a task identifier is not enough information
+        to uniquely find a NetworkDataTask in a NetworkSession since r196034 because there are two NSURLSessions
+        in a NetworkSession, one with credentials and one without.  The assertion would fire in a case like if we
+        made the first NetworkDataTask with credentials (taskIdentifier is 1) and the first NetworkDataTask 
+        without credentials before the first NetworkDataTask with credentials was finished.  In that case, the 
+        taskIdentifier would also be 1, which would conflict with the other taskIdentifier.  That taskIdentifier
+        would uniquely identify the task in the correct NSURLSession, though, so the solution is to keep a map
+        for each NSURLSession in the NetworkSession.
+
+        * NetworkProcess/NetworkDataTask.h:
+        * NetworkProcess/NetworkSession.h:
+        * NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:
+        (WebKit::NetworkDataTask::NetworkDataTask):
+        (WebKit::NetworkDataTask::~NetworkDataTask):
+        (WebKit::NetworkDataTask::suspend):
+        (WebKit::serverTrustCredential):
+        * NetworkProcess/cocoa/NetworkSessionCocoa.mm:
+        (-[WKNetworkSessionDelegate URLSession:task:didSendBodyData:totalBytesSent:totalBytesExpectedToSend:]):
+        (-[WKNetworkSessionDelegate URLSession:task:willPerformHTTPRedirection:newRequest:completionHandler:]):
+        (-[WKNetworkSessionDelegate URLSession:task:didReceiveChallenge:completionHandler:]):
+        (-[WKNetworkSessionDelegate URLSession:task:didCompleteWithError:]):
+        (-[WKNetworkSessionDelegate URLSession:dataTask:didReceiveResponse:completionHandler:]):
+        (-[WKNetworkSessionDelegate URLSession:dataTask:didReceiveData:]):
+        (-[WKNetworkSessionDelegate URLSession:downloadTask:didWriteData:totalBytesWritten:totalBytesExpectedToWrite:]):
+        (-[WKNetworkSessionDelegate URLSession:dataTask:didBecomeDownloadTask:]):
+        (WebKit::NetworkSession::clearCredentials):
+        (WebKit::NetworkSession::dataTaskForIdentifier):
+        (WebKit::NetworkSession::addDownloadID):
+
 2016-03-01  Andreas Kling  <akling@apple.com>
 
         REGRESSION (r154616): Accelerated drawing is off during the initial load
index 1432664..6797e91 100644 (file)
@@ -93,7 +93,6 @@ public:
     void resume();
     
     typedef uint64_t TaskIdentifier;
-    TaskIdentifier taskIdentifier();
     
     ~NetworkDataTask();
     
index 0a8b213..454b2f2 100644 (file)
@@ -57,14 +57,15 @@ public:
     static NetworkSession& defaultSession();
     void clearCredentials();
 
-    NetworkDataTask* dataTaskForIdentifier(NetworkDataTask::TaskIdentifier);
+    NetworkDataTask* dataTaskForIdentifier(NetworkDataTask::TaskIdentifier, WebCore::StoredCredentials);
 
     void addDownloadID(NetworkDataTask::TaskIdentifier, DownloadID);
     DownloadID downloadID(NetworkDataTask::TaskIdentifier);
     DownloadID takeDownloadID(NetworkDataTask::TaskIdentifier);
     
 private:
-    HashMap<NetworkDataTask::TaskIdentifier, NetworkDataTask*> m_dataTaskMap;
+    HashMap<NetworkDataTask::TaskIdentifier, NetworkDataTask*> m_dataTaskMapWithCredentials;
+    HashMap<NetworkDataTask::TaskIdentifier, NetworkDataTask*> m_dataTaskMapWithoutCredentials;
     HashMap<NetworkDataTask::TaskIdentifier, DownloadID> m_downloadMap;
 #if PLATFORM(COCOA)
     RetainPtr<NSURLSession> m_sessionWithCredentialStorage;
index 3407169..be4dcbe 100644 (file)
@@ -76,22 +76,28 @@ NetworkDataTask::NetworkDataTask(NetworkSession& session, NetworkDataTaskClient&
         nsRequest = mutableRequest;
     }
 
-    if (storedCredentials == WebCore::AllowStoredCredentials)
+    if (storedCredentials == WebCore::AllowStoredCredentials) {
         m_task = [m_session.m_sessionWithCredentialStorage dataTaskWithRequest:nsRequest];
-    else
+        ASSERT(!m_session.m_dataTaskMapWithCredentials.contains([m_task taskIdentifier]));
+        m_session.m_dataTaskMapWithCredentials.add([m_task taskIdentifier], this);
+    } else {
         m_task = [m_session.m_sessionWithoutCredentialStorage dataTaskWithRequest:nsRequest];
-    
-    ASSERT(!m_session.m_dataTaskMap.contains(taskIdentifier()));
-    m_session.m_dataTaskMap.add(taskIdentifier(), this);
+        ASSERT(!m_session.m_dataTaskMapWithoutCredentials.contains([m_task taskIdentifier]));
+        m_session.m_dataTaskMapWithoutCredentials.add([m_task taskIdentifier], this);
+    }
 }
 
 NetworkDataTask::~NetworkDataTask()
 {
+    ASSERT(isMainThread());
     if (m_task) {
-        ASSERT(m_session.m_dataTaskMap.contains(taskIdentifier()));
-        ASSERT(m_session.m_dataTaskMap.get(taskIdentifier()) == this);
-        ASSERT(isMainThread());
-        m_session.m_dataTaskMap.remove(taskIdentifier());
+        if (m_storedCredentials == WebCore::StoredCredentials::AllowStoredCredentials) {
+            ASSERT(m_session.m_dataTaskMapWithCredentials.get([m_task taskIdentifier]) == this);
+            m_session.m_dataTaskMapWithCredentials.remove([m_task taskIdentifier]);
+        } else {
+            ASSERT(m_session.m_dataTaskMapWithoutCredentials.get([m_task taskIdentifier]) == this);
+            m_session.m_dataTaskMapWithoutCredentials.remove([m_task taskIdentifier]);
+        }
     }
 }
 
@@ -260,11 +266,6 @@ void NetworkDataTask::suspend()
     [m_task suspend];
 }
 
-auto NetworkDataTask::taskIdentifier() -> TaskIdentifier
-{
-    return [m_task taskIdentifier];
-}
-
 WebCore::Credential serverTrustCredential(const WebCore::AuthenticationChallenge& challenge)
 {
     return WebCore::Credential([NSURLCredential credentialForTrust:challenge.nsURLAuthenticationChallenge().protectionSpace.serverTrust]);
index 40b789f..2f7e318 100644 (file)
@@ -98,13 +98,15 @@ static NSURLSessionAuthChallengeDisposition toNSURLSessionAuthChallengeDispositi
 
 - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didSendBodyData:(int64_t)bytesSent totalBytesSent:(int64_t)totalBytesSent totalBytesExpectedToSend:(int64_t)totalBytesExpectedToSend
 {
-    if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier))
+    auto storedCredentials = session.configuration.URLCredentialStorage ? WebCore::StoredCredentials::AllowStoredCredentials : WebCore::StoredCredentials::DoNotAllowStoredCredentials;
+    if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier, storedCredentials))
         networkDataTask->didSendData(totalBytesSent, totalBytesExpectedToSend);
 }
 
 - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task willPerformHTTPRedirection:(NSHTTPURLResponse *)response newRequest:(NSURLRequest *)request completionHandler:(void (^)(NSURLRequest *))completionHandler
 {
-    if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier)) {
+    auto storedCredentials = session.configuration.URLCredentialStorage ? WebCore::StoredCredentials::AllowStoredCredentials : WebCore::StoredCredentials::DoNotAllowStoredCredentials;
+    if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier, storedCredentials)) {
         auto completionHandlerCopy = Block_copy(completionHandler);
         networkDataTask->willPerformHTTPRedirection(response, request, [completionHandlerCopy](const WebCore::ResourceRequest& request) {
             completionHandlerCopy(request.nsURLRequest(WebCore::UpdateHTTPBody));
@@ -115,7 +117,8 @@ static NSURLSessionAuthChallengeDisposition toNSURLSessionAuthChallengeDispositi
 
 - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didReceiveChallenge:(NSURLAuthenticationChallenge *)challenge completionHandler:(void (^)(NSURLSessionAuthChallengeDisposition disposition, NSURLCredential *credential))completionHandler
 {
-    if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier)) {
+    auto storedCredentials = session.configuration.URLCredentialStorage ? WebCore::StoredCredentials::AllowStoredCredentials : WebCore::StoredCredentials::DoNotAllowStoredCredentials;
+    if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier, storedCredentials)) {
         auto completionHandlerCopy = Block_copy(completionHandler);
         auto challengeCompletionHandler = [completionHandlerCopy](WebKit::AuthenticationChallengeDisposition disposition, const WebCore::Credential& credential)
         {
@@ -132,7 +135,8 @@ static NSURLSessionAuthChallengeDisposition toNSURLSessionAuthChallengeDispositi
 
 - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didCompleteWithError:(NSError *)error
 {
-    if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier))
+    auto storedCredentials = session.configuration.URLCredentialStorage ? WebCore::StoredCredentials::AllowStoredCredentials : WebCore::StoredCredentials::DoNotAllowStoredCredentials;
+    if (auto* networkDataTask = _session->dataTaskForIdentifier(task.taskIdentifier, storedCredentials))
         networkDataTask->didCompleteWithError(error);
     else if (error) {
         auto downloadID = _session->takeDownloadID(task.taskIdentifier);
@@ -145,7 +149,8 @@ static NSURLSessionAuthChallengeDisposition toNSURLSessionAuthChallengeDispositi
 
 - (void)URLSession:(NSURLSession *)session dataTask:(NSURLSessionDataTask *)dataTask didReceiveResponse:(NSURLResponse *)response completionHandler:(void (^)(NSURLSessionResponseDisposition disposition))completionHandler
 {
-    if (auto* networkDataTask = _session->dataTaskForIdentifier(dataTask.taskIdentifier)) {
+    auto storedCredentials = session.configuration.URLCredentialStorage ? WebCore::StoredCredentials::AllowStoredCredentials : WebCore::StoredCredentials::DoNotAllowStoredCredentials;
+    if (auto* networkDataTask = _session->dataTaskForIdentifier(dataTask.taskIdentifier, storedCredentials)) {
         ASSERT(isMainThread());
         WebCore::ResourceResponse resourceResponse(response);
         copyTimingData([dataTask _timingData], resourceResponse.resourceLoadTiming());
@@ -159,7 +164,8 @@ static NSURLSessionAuthChallengeDisposition toNSURLSessionAuthChallengeDispositi
 
 - (void)URLSession:(NSURLSession *)session dataTask:(NSURLSessionDataTask *)dataTask didReceiveData:(NSData *)data
 {
-    if (auto* networkDataTask = _session->dataTaskForIdentifier(dataTask.taskIdentifier))
+    auto storedCredentials = session.configuration.URLCredentialStorage ? WebCore::StoredCredentials::AllowStoredCredentials : WebCore::StoredCredentials::DoNotAllowStoredCredentials;
+    if (auto* networkDataTask = _session->dataTaskForIdentifier(dataTask.taskIdentifier, storedCredentials))
         networkDataTask->didReceiveData(WebCore::SharedBuffer::wrapNSData(data));
 }
 
@@ -172,7 +178,8 @@ static NSURLSessionAuthChallengeDisposition toNSURLSessionAuthChallengeDispositi
 
 - (void)URLSession:(NSURLSession *)session downloadTask:(NSURLSessionDownloadTask *)downloadTask didWriteData:(int64_t)bytesWritten totalBytesWritten:(int64_t)totalBytesWritten totalBytesExpectedToWrite:(int64_t)totalBytesExpectedToWrite
 {
-    ASSERT_WITH_MESSAGE(!_session->dataTaskForIdentifier([downloadTask taskIdentifier]), "The NetworkDataTask should be destroyed immediately after didBecomeDownloadTask returns");
+    auto storedCredentials = session.configuration.URLCredentialStorage ? WebCore::StoredCredentials::AllowStoredCredentials : WebCore::StoredCredentials::DoNotAllowStoredCredentials;
+    ASSERT_WITH_MESSAGE(!_session->dataTaskForIdentifier([downloadTask taskIdentifier], storedCredentials), "The NetworkDataTask should be destroyed immediately after didBecomeDownloadTask returns");
 
     auto downloadID = _session->downloadID([downloadTask taskIdentifier]);
     if (auto* download = WebKit::NetworkProcess::singleton().downloadManager().download(downloadID))
@@ -186,7 +193,8 @@ static NSURLSessionAuthChallengeDisposition toNSURLSessionAuthChallengeDispositi
 
 - (void)URLSession:(NSURLSession *)session dataTask:(NSURLSessionDataTask *)dataTask didBecomeDownloadTask:(NSURLSessionDownloadTask *)downloadTask
 {
-    if (auto* networkDataTask = _session->dataTaskForIdentifier([dataTask taskIdentifier])) {
+    auto storedCredentials = session.configuration.URLCredentialStorage ? WebCore::StoredCredentials::AllowStoredCredentials : WebCore::StoredCredentials::DoNotAllowStoredCredentials;
+    if (auto* networkDataTask = _session->dataTaskForIdentifier([dataTask taskIdentifier], storedCredentials)) {
         auto downloadID = networkDataTask->pendingDownloadID();
         auto& downloadManager = WebKit::NetworkProcess::singleton().downloadManager();
         auto download = std::make_unique<WebKit::Download>(downloadManager, downloadID);
@@ -266,15 +274,18 @@ NetworkSession::~NetworkSession()
 
 void NetworkSession::clearCredentials()
 {
-    ASSERT(m_dataTaskMap.isEmpty());
+    ASSERT(m_dataTaskMapWithCredentials.isEmpty());
+    ASSERT(m_dataTaskMapWithoutCredentials.isEmpty());
     ASSERT(m_downloadMap.isEmpty());
     m_sessionWithCredentialStorage = [NSURLSession sessionWithConfiguration:m_sessionWithCredentialStorage.get().configuration delegate:static_cast<id>(m_sessionDelegate.get()) delegateQueue:[NSOperationQueue mainQueue]];
 }
 
-NetworkDataTask* NetworkSession::dataTaskForIdentifier(NetworkDataTask::TaskIdentifier taskIdentifier)
+NetworkDataTask* NetworkSession::dataTaskForIdentifier(NetworkDataTask::TaskIdentifier taskIdentifier, WebCore::StoredCredentials storedCredentials)
 {
     ASSERT(isMainThread());
-    return m_dataTaskMap.get(taskIdentifier);
+    if (storedCredentials == WebCore::StoredCredentials::AllowStoredCredentials)
+        return m_dataTaskMapWithCredentials.get(taskIdentifier);
+    return m_dataTaskMapWithoutCredentials.get(taskIdentifier);
 }
 
 void NetworkSession::addDownloadID(NetworkDataTask::TaskIdentifier taskIdentifier, DownloadID downloadID)