Changeset 32871 in webkit

Timestamp:

May 5, 2008 9:09:00 AM (16 years ago)

Author:

Antti Koivisto

Message:

2008-05-05 Antti Koivisto <Antti Koivisto>

Reviewed by Darin.

Speculative fix for <rdar://problem/5906790>
Crash in Loader::servePendingRequests() due to hash table being modified during iteration

I don't know how to reproduce this. It would require the load to fail (or succeed)
synchronously, something that should not usually happen.

• loader/loader.cpp: (WebCore::Loader::Loader): (WebCore::Loader::load): (WebCore::Loader::servePendingRequests): (WebCore::Loader::cancelRequests): (WebCore::Loader::Host::Host):
• loader/loader.h: (WebCore::Loader::Host::name):

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/loader.cpp (modified) (5 diffs)
• loader/loader.h (modified) (3 diffs)

trunk/WebCore/ChangeLog

@@ +1 @@
+2008-05-05  Antti Koivisto  <antti@apple.com>
+
+        Reviewed by Darin.
+
+        Speculative fix for <rdar://problem/5906790> 
+        Crash in Loader::servePendingRequests() due to hash table being modified during iteration
+        
+        I don't know how to reproduce this. It would require the load to fail (or succeed)
+        synchronously, something that should not usually happen.
+
+        * loader/loader.cpp:
+        (WebCore::Loader::Loader):
+        (WebCore::Loader::load):
+        (WebCore::Loader::servePendingRequests):
+        (WebCore::Loader::cancelRequests):
+        (WebCore::Loader::Host::Host):
+        * loader/loader.h:
+        (WebCore::Loader::Host::name):
+
 2008-05-05  Ariya Hidayat  <ariya.hidayat@trolltech.com>
 

trunk/WebCore/loader/loader.cpp

@@ -59 +59 @@
     
 Loader::Loader()
-    : m_nonHTTPProtocolHost(maxRequestsInFlightForNonHTTPProtocols)
+    : m_nonHTTPProtocolHost("non-http-protocol-host", maxRequestsInFlightForNonHTTPProtocols)
     , m_requestTimer(this, &Loader::requestTimerFired)
 {
@@ -103 +103 @@
     bool isHTTP = url.protocolIs("http") || url.protocolIs("https");
     if (isHTTP) {
-        String hostName = url.host();
-        host = m_hosts.get(hostName);
+        AtomicString hostName = url.host();
+        host = m_hosts.get(hostName.impl());
         if (!host) {
-            host = new Host(maxRequestsInFlightPerHost);
-            m_hosts.add(hostName, host);
+            host = new Host(hostName, maxRequestsInFlightPerHost);
+            m_hosts.add(hostName.impl(), host);
         }
     } else 
@@ -142 +142 @@
     m_nonHTTPProtocolHost.servePendingRequests(minimumPriority);
 
-    HostMap::iterator end = m_hosts.end();
-    Vector<String> toRemove;
-    for (HostMap::iterator it = m_hosts.begin(); it != end; ++it) {
-        Host* host = it->second;
+    Vector<Host*> hostsToServe;
+    copyValuesToVector(m_hosts, hostsToServe);
+    for (unsigned n = 0; n < hostsToServe.size(); ++n) {
+        Host* host = hostsToServe[n];
         if (host->hasRequests())
             host->servePendingRequests(minimumPriority);
-        else
-            toRemove.append(it->first);
-    }
-    for (unsigned n = 0; n < toRemove.size(); ++n) {
-        HostMap::iterator it = m_hosts.find(toRemove[n]);
-        delete it->second;
-        m_hosts.remove(it);
+        else {
+            AtomicString name = host->name();
+            delete host;
+            m_hosts.remove(name.impl());
+        }
     }
 }
@@ -163 +161 @@
         m_nonHTTPProtocolHost.cancelRequests(docLoader);
     
-    HostMap::iterator end = m_hosts.end();
-    for (HostMap::iterator it = m_hosts.begin(); it != end; ++it) {
-        Host* host = it->second;
+    Vector<Host*> hostsToCancel;
+    copyValuesToVector(m_hosts, hostsToCancel);
+    for (unsigned n = 0; n < hostsToCancel.size(); ++n) {
+        Host* host = hostsToCancel[n];
         if (host->hasRequests())
             host->cancelRequests(docLoader);
@@ -178 +177 @@
 }
     
-Loader::Host::Host(unsigned maxRequestsInFlight)
-    : m_maxRequestsInFlight(maxRequestsInFlight)
+Loader::Host::Host(const AtomicString& name, unsigned maxRequestsInFlight)
+    : m_name(name)
+    , m_maxRequestsInFlight(maxRequestsInFlight)
 {
 }

trunk/WebCore/loader/loader.h

@@ -23 +23 @@
 #define loader_h
 
+#include "AtomicString.h"
+#include "AtomicStringImpl.h"
 #include "PlatformString.h"
-#include "StringHash.h"
 #include "SubresourceLoaderClient.h"
 #include "Timer.h"
@@ -57 +58 @@
         class Host : private SubresourceLoaderClient {
         public:
-            Host(unsigned maxRequestsInFlight);
+            Host(const AtomicString& name, unsigned maxRequestsInFlight);
             ~Host();
             
+            const AtomicString& name() const { return m_name; }
             void addRequest(Request*, Priority);
             void servePendingRequests(Priority minimumPriority = Low);
@@ -79 +81 @@
             typedef HashMap<RefPtr<SubresourceLoader>, Request*> RequestMap;
             RequestMap m_requestsLoading;
+            const AtomicString m_name;
             const int m_maxRequestsInFlight;
         };
-        typedef HashMap<String, Host*> HostMap;
+        typedef HashMap<AtomicStringImpl*, Host*> HostMap;
         HostMap m_hosts;
         Host m_nonHTTPProtocolHost;