Changeset 137783 in webkit

Timestamp:

Dec 14, 2012, 3:33:45 PM (14 years ago)

Author:

ap@apple.com

Message:

<rdar://problem/12874760> NetworkProcess loads may get stuck when WebProcess quits
​https://bugs.webkit.org/show_bug.cgi?id=105056

Reviewed by Anders Carlsson.

Make response maps per-connection.

• NetworkProcess/NetworkConnectionToWebProcess.cpp: (WebKit::NetworkConnectionToWebProcess::didClose): Cancel waiting for responses from WebProcess, they will never arrive.

• NetworkProcess/NetworkConnectionToWebProcess.h: (WebKit::NetworkConnectionToWebProcess::willSendRequestResponseMap): (WebKit::NetworkConnectionToWebProcess::canAuthenticateAgainstProtectionSpaceResponseMap): Maps now live here.

• NetworkProcess/NetworkResourceLoader.cpp: (WebKit::NetworkResourceLoader::connectionToWebProcessDidClose): Added a FIXME.

(WebKit::NetworkResourceLoader::willSendRequest):
(WebKit::NetworkResourceLoader::willSendRequestHandled):
(WebKit::NetworkResourceLoader::canAuthenticateAgainstProtectionSpace):
(WebKit::NetworkResourceLoader::canAuthenticateAgainstProtectionSpaceHandled):
Handle the cases where we can't send a request, or can't expect a response any more.

• Shared/BlockingResponseMap.h: (BlockingResponseMap): (BlockingResponseMap::BlockingResponseMap): (BlockingResponseMap::~BlockingResponseMap): (BlockingResponseMap::waitForResponse): (BlockingResponseMap::didReceiveResponse): (BlockingResponseMap::cancel): (BlockingBoolResponseMap): (BlockingBoolResponseMap::BlockingBoolResponseMap): (BlockingBoolResponseMap::~BlockingBoolResponseMap): (BlockingBoolResponseMap::waitForResponse): (BlockingBoolResponseMap::didReceiveResponse): (BlockingBoolResponseMap::cancel): Added an ability to cancel, and slightly beefed up overall.

Location:

trunk/Source/WebKit2

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/NetworkConnectionToWebProcess.cpp (modified) (1 diff)
• NetworkProcess/NetworkConnectionToWebProcess.h (modified) (3 diffs)
• NetworkProcess/NetworkResourceLoader.cpp (modified) (7 diffs)
• Shared/BlockingResponseMap.h (modified) (5 diffs)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2012-12-14  Alexey Proskuryakov  <ap@apple.com>
+
+        <rdar://problem/12874760> NetworkProcess loads may get stuck when WebProcess quits
+        https://bugs.webkit.org/show_bug.cgi?id=105056
+
+        Reviewed by Anders Carlsson.
+
+        Make response maps per-connection.
+
+        * NetworkProcess/NetworkConnectionToWebProcess.cpp:
+        (WebKit::NetworkConnectionToWebProcess::didClose): Cancel waiting for responses
+        from WebProcess, they will never arrive.
+
+        * NetworkProcess/NetworkConnectionToWebProcess.h:
+        (WebKit::NetworkConnectionToWebProcess::willSendRequestResponseMap):
+        (WebKit::NetworkConnectionToWebProcess::canAuthenticateAgainstProtectionSpaceResponseMap):
+        Maps now live here.
+
+        * NetworkProcess/NetworkResourceLoader.cpp:
+        (WebKit::NetworkResourceLoader::connectionToWebProcessDidClose): Added a FIXME.
+
+        (WebKit::NetworkResourceLoader::willSendRequest):
+        (WebKit::NetworkResourceLoader::willSendRequestHandled):
+        (WebKit::NetworkResourceLoader::canAuthenticateAgainstProtectionSpace):
+        (WebKit::NetworkResourceLoader::canAuthenticateAgainstProtectionSpaceHandled):
+        Handle the cases where we can't send a request, or can't expect a response any more.
+
+        * Shared/BlockingResponseMap.h:
+        (BlockingResponseMap):
+        (BlockingResponseMap::BlockingResponseMap):
+        (BlockingResponseMap::~BlockingResponseMap):
+        (BlockingResponseMap::waitForResponse):
+        (BlockingResponseMap::didReceiveResponse):
+        (BlockingResponseMap::cancel):
+        (BlockingBoolResponseMap):
+        (BlockingBoolResponseMap::BlockingBoolResponseMap):
+        (BlockingBoolResponseMap::~BlockingBoolResponseMap):
+        (BlockingBoolResponseMap::waitForResponse):
+        (BlockingBoolResponseMap::didReceiveResponse):
+        (BlockingBoolResponseMap::cancel):
+        Added an ability to cancel, and slightly beefed up overall.
+
 2012-12-14  Anders Carlsson  <andersca@apple.com>
 

trunk/Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp

@@ -103 +103 @@
     
     NetworkProcess::shared().removeNetworkConnectionToWebProcess(this);
-    
-    // FIXME (NetworkProcess): We might consider actively clearing out all requests for this connection.
-    // But that might not be necessary as the observer mechanism used above is much more direct.
+
+    // Unblock waiting threads.
+    m_willSendRequestResponseMap.cancel();
+    m_canAuthenticateAgainstProtectionSpaceResponseMap.cancel();
 
     Vector<NetworkConnectionToWebProcessObserver*> observers;

trunk/Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h

@@ -29 +29 @@
 #if ENABLE(NETWORK_PROCESS)
 
+#include "BlockingResponseMap.h"
 #include "Connection.h"
 #include "NetworkConnectionToWebProcessMessages.h"
@@ -61 +62 @@
 
     bool isSerialLoadingEnabled() const { return m_serialLoadingEnabled; }
+
+    BlockingResponseMap<WebCore::ResourceRequest*>& willSendRequestResponseMap() { return m_willSendRequestResponseMap; }
+    BlockingBoolResponseMap& canAuthenticateAgainstProtectionSpaceResponseMap() { return m_canAuthenticateAgainstProtectionSpaceResponseMap; }
 
 private:
@@ -96 +100 @@
     
     HashSet<NetworkConnectionToWebProcessObserver*> m_observers;
-    
+
+    BlockingResponseMap<WebCore::ResourceRequest*> m_willSendRequestResponseMap;
+    BlockingBoolResponseMap m_canAuthenticateAgainstProtectionSpaceResponseMap;
+
     bool m_serialLoadingEnabled;
 };

trunk/Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp

@@ -29 +29 @@
 #if ENABLE(NETWORK_PROCESS)
 
-#include "BlockingResponseMap.h"
 #include "DataReference.h"
 #include "Logging.h"
@@ -145 +144 @@
 {
     ASSERT_ARG(connection, connection == m_connection.get());
+    // FIXME (NetworkProcess): Cancel the load. The request may be long-living, so we don't want it to linger around after all clients are gone.
 }
 
@@ -178 +178 @@
 }
 
-static BlockingResponseMap<ResourceRequest*>& willSendRequestResponseMap()
-{
-    AtomicallyInitializedStatic(BlockingResponseMap<ResourceRequest*>&, responseMap = *new BlockingResponseMap<ResourceRequest*>);
-    return responseMap;
-}
-
 static uint64_t generateWillSendRequestID()
 {
@@ -192 +186 @@
 void NetworkResourceLoader::willSendRequest(ResourceHandle*, ResourceRequest& request, const ResourceResponse& redirectResponse)
 {
-    // We only expect to get the willSendRequest callback from ResourceHandle as the result of a redirect
+    // We only expect to get the willSendRequest callback from ResourceHandle as the result of a redirect.
     ASSERT(!redirectResponse.isNull());
 
     uint64_t requestID = generateWillSendRequestID();
 
-    send(Messages::WebResourceLoader::WillSendRequest(requestID, request, redirectResponse));
-    
-    OwnPtr<ResourceRequest> newRequest = willSendRequestResponseMap().waitForResponse(requestID);
-    request = *newRequest;
+    if (!send(Messages::WebResourceLoader::WillSendRequest(requestID, request, redirectResponse))) {
+        request = ResourceRequest();
+        return;
+    }
+
+    OwnPtr<ResourceRequest> newRequest = m_connection->willSendRequestResponseMap().waitForResponse(requestID);
+    request = newRequest ? *newRequest : ResourceRequest();
 
     RunLoop::main()->dispatch(WTF::bind(&NetworkResourceLoadScheduler::receivedRedirect, &NetworkProcess::shared().networkResourceLoadScheduler(), m_identifier, request.url()));
@@ -207 +204 @@
 void NetworkResourceLoader::willSendRequestHandled(uint64_t requestID, const WebCore::ResourceRequest& newRequest)
 {
-    willSendRequestResponseMap().didReceiveResponse(requestID, adoptPtr(new ResourceRequest(newRequest)));
+    m_connection->willSendRequestResponseMap().didReceiveResponse(requestID, adoptPtr(new ResourceRequest(newRequest)));
 }
 
@@ -301 +298 @@
 
 #if USE(PROTECTION_SPACE_AUTH_CALLBACK)
-static BlockingBoolResponseMap& canAuthenticateAgainstProtectionSpaceResponseMap()
-{
-    AtomicallyInitializedStatic(BlockingBoolResponseMap&, responseMap = *new BlockingBoolResponseMap);
-    return responseMap;
-}
-
 static uint64_t generateCanAuthenticateAgainstProtectionSpaceID()
 {
@@ -317 +308 @@
     uint64_t requestID = generateCanAuthenticateAgainstProtectionSpaceID();
 
-    send(Messages::WebResourceLoader::CanAuthenticateAgainstProtectionSpace(requestID, protectionSpace));
-
-    return canAuthenticateAgainstProtectionSpaceResponseMap().waitForResponse(requestID);
+    if (!send(Messages::WebResourceLoader::CanAuthenticateAgainstProtectionSpace(requestID, protectionSpace)))
+        return false;
+
+    return m_connection->canAuthenticateAgainstProtectionSpaceResponseMap().waitForResponse(requestID);
 }
 
 void NetworkResourceLoader::canAuthenticateAgainstProtectionSpaceHandled(uint64_t requestID, bool canAuthenticate)
 {
-    canAuthenticateAgainstProtectionSpaceResponseMap().didReceiveResponse(requestID, canAuthenticate);
+    m_connection->canAuthenticateAgainstProtectionSpaceResponseMap().didReceiveResponse(requestID, canAuthenticate);
 }
 #endif

trunk/Source/WebKit2/Shared/BlockingResponseMap.h

@@ -34 +34 @@
 template<typename T>
 class BlockingResponseMap {
+WTF_MAKE_NONCOPYABLE(BlockingResponseMap);
 public:
+    BlockingResponseMap() : m_canceled(false) { }
+    ~BlockingResponseMap() { ASSERT(m_responses.isEmpty()); }
+
     PassOwnPtr<T> waitForResponse(uint64_t requestID)
     {
         while (true) {
             MutexLocker locker(m_mutex);
+
+            if (m_canceled)
+                return nullptr;
 
             if (OwnPtr<T> response = m_responses.take(requestID))
@@ -55 +62 @@
 
         m_responses.set(requestID, response);
-        // FIXME (NetworkProcess): Waking up all threads is quite inefficient.
+        // FIXME (NetworkProcess): <rdar://problem/12886430>: Waking up all threads is quite inefficient.
+        m_condition.broadcast();
+    }
+
+    void cancel()
+    {
+        m_canceled = true;
+
+        // FIXME (NetworkProcess): <rdar://problem/12886430>: Waking up all threads is quite inefficient.
         m_condition.broadcast();
     }
@@ -64 +79 @@
 
     HashMap<uint64_t, OwnPtr<T> > m_responses;
+    bool m_canceled;
 };
 
 class BlockingBoolResponseMap {
+WTF_MAKE_NONCOPYABLE(BlockingBoolResponseMap);
 public:
+    BlockingBoolResponseMap() : m_canceled(false) { }
+    ~BlockingBoolResponseMap() { ASSERT(m_responses.isEmpty()); }
+
     bool waitForResponse(uint64_t requestID)
     {
         while (true) {
             MutexLocker locker(m_mutex);
+
+            // FIXME: Differentiate between canceled wait and a negative response.
+            if (m_canceled)
+                return false;
 
             HashMap<uint64_t, bool>::iterator iter = m_responses.find(requestID);
@@ -92 +116 @@
 
         m_responses.set(requestID, response);
-        // FIXME (NetworkProcess): Waking up all threads is quite inefficient.
+        // FIXME (NetworkProcess): <rdar://problem/12886430>: Waking up all threads is quite inefficient.
+        m_condition.broadcast();
+    }
+
+    void cancel()
+    {
+        m_canceled = true;
+
+        // FIXME (NetworkProcess): <rdar://problem/12886430>: Waking up all threads is quite inefficient.
         m_condition.broadcast();
     }
@@ -101 +133 @@
 
     HashMap<uint64_t, bool> m_responses;
+    bool m_canceled;
 };