Changeset 76701 in webkit

Timestamp:

Jan 26, 2011 11:07:15 AM (13 years ago)

Author:

bweinstein@apple.com

Message:

Source/WebCore: Crashes loading pages when cancelling subresource loads through WebKit
​https://bugs.webkit.org/show_bug.cgi?id=53123
<rdar://problem/8914361>

Reviewed by Antti Koivisto.

Fix a crash that happened when cancelling subresource loads through WebKit.

When a load is cancelled synchronously (via the WebKit client), CachedResourceLoader::requestResource
can be called recursively on the same function, either leading to infinite recursion, or deleting
an object when it is not done being used.

The fix for this was to call checkForPendingPreloads and servePendingRequests asynchronously when
CachedResourceLoader::loadDone was called synchronously (due to the load being cancelled synchronously).

Test: fast/loader/willSendRequest-null-for-preload.html

• loader/DocumentLoader.cpp:

(WebCore::DocumentLoader::setRequest): Only dispatch didReceiveServerRedirectForProvisionalLoadForFrame

if our new URL is non-null.

• loader/cache/CachedResourceLoader.cpp:

(WebCore::CachedResourceLoader::CachedResourceLoader): Initialize our timer.
(WebCore::CachedResourceLoader::loadDone): If the CachedResource we were passed in was 0, that means this

function was called synchronously
from CachedResourceRequest::load, and we don't want to call into checkForPendingPreloads synchronously,
so put it on a 0-delay timer to make the calls to checkForPendingPreloads and servePendingRequests asynchronous.

(WebCore::CachedResourceLoader::loadDonePendingActionTimerFired): Call checkForPendingPreloads and servePendingRequests.
(WebCore::CachedResourceLoader::checkForPendingPreloads): m_pendingPreloads is now a Deque instead of a Vector,

so use Deque methods.

• loader/cache/CachedResourceLoader.h: Add the timer, the timer callback function, and make m_pendingPreloads a Deque.

Source/WebKit2: Crashes loading pages when cancelling subresource loads through WebKit
​https://bugs.webkit.org/show_bug.cgi?id=53123
<rdar://problem/8914361>

Reviewed by Antti Koivisto.

• WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:

(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForMIMEType): If our URL is null, return early instead of dispatching

a message.

LayoutTests: Reviewed byAntti Koivisto.

Crashes loading pages when cancelling subresource loads through WebKit
​https://bugs.webkit.org/show_bug.cgi?id=53123
<rdar://problem/8914361>

Add tests for crashing when cancelling subresource loads through WebKit via setWillSendRequestReturnsNull.

• fast/loader/willSendRequest-null-for-preload-expected.txt: Added.
• fast/loader/willSendRequest-null-for-preload.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/loader/willSendRequest-null-for-preload-expected.txt (added)
• LayoutTests/fast/loader/willSendRequest-null-for-preload.html (added)
• LayoutTests/platform/win/fast/loader (added)
• LayoutTests/platform/win/fast/loader/willSendRequest-null-for-preload-expected.txt (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/loader/DocumentLoader.cpp (modified) (1 diff)
• Source/WebCore/loader/cache/CachedResourceLoader.cpp (modified) (3 diffs)
• Source/WebCore/loader/cache/CachedResourceLoader.h (modified) (3 diffs)
• Source/WebKit2/ChangeLog (modified) (1 diff)
• Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2011-01-25  Brian Weinstein  <bweinstein@apple.com>
+
+        Reviewed byAntti Koivisto.
+
+        Crashes loading pages when cancelling subresource loads through WebKit
+        https://bugs.webkit.org/show_bug.cgi?id=53123
+        <rdar://problem/8914361>
+        
+        Add tests for crashing when cancelling subresource loads through WebKit via setWillSendRequestReturnsNull.
+
+        * fast/loader/willSendRequest-null-for-preload-expected.txt: Added.
+        * fast/loader/willSendRequest-null-for-preload.html: Added.
+
 2011-01-26  Ryosuke Niwa  <rniwa@webkit.org>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2011-01-25  Brian Weinstein  <bweinstein@apple.com>
+
+        Reviewed by Antti Koivisto.
+
+        Crashes loading pages when cancelling subresource loads through WebKit
+        https://bugs.webkit.org/show_bug.cgi?id=53123
+        <rdar://problem/8914361>
+        
+        Fix a crash that happened when cancelling subresource loads through WebKit.
+        
+        When a load is cancelled synchronously (via the WebKit client), CachedResourceLoader::requestResource 
+        can be called recursively on the same function, either leading to infinite recursion, or deleting 
+        an object when it is not done being used.
+        
+        The fix for this was to call checkForPendingPreloads and servePendingRequests asynchronously when 
+        CachedResourceLoader::loadDone was called synchronously (due to the load being cancelled synchronously).
+
+        Test: fast/loader/willSendRequest-null-for-preload.html
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::setRequest): Only dispatch didReceiveServerRedirectForProvisionalLoadForFrame 
+            if our new URL is non-null.
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::CachedResourceLoader): Initialize our timer.
+        (WebCore::CachedResourceLoader::loadDone): If the CachedResource we were passed in was 0, that means this 
+            function was called synchronously
+            from CachedResourceRequest::load, and we don't want to call into checkForPendingPreloads synchronously, 
+            so put it on a 0-delay timer to make the calls to checkForPendingPreloads and servePendingRequests asynchronous.
+        (WebCore::CachedResourceLoader::loadDonePendingActionTimerFired): Call checkForPendingPreloads and servePendingRequests.
+        (WebCore::CachedResourceLoader::checkForPendingPreloads): m_pendingPreloads is now a Deque instead of a Vector, 
+            so use Deque methods.
+        * loader/cache/CachedResourceLoader.h: Add the timer, the timer callback function, and make m_pendingPreloads a Deque.
+
 2011-01-25  Pavel Podivilov  <podivilov@chromium.org>
 

trunk/Source/WebCore/loader/DocumentLoader.cpp

@@ -169 +169 @@
     m_request = req;
 
-    // Only send webView:didReceiveServerRedirectForProvisionalLoadForFrame: if URL changed.
+    // Only send webView:didReceiveServerRedirectForProvisionalLoadForFrame: if URL changed (and is non-null).
     // Also, don't send it when replacing unreachable URLs with alternate content.
-    if (!handlingUnreachableURL && oldURL != req.url())
+    if (!handlingUnreachableURL && !req.url().isNull() && oldURL != req.url())
         frameLoader()->didReceiveServerRedirectForProvisionalLoadForFrame();
 }

trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp

@@ -81 +81 @@
     : m_document(document)
     , m_requestCount(0)
+    , m_loadDoneActionTimer(this, &CachedResourceLoader::loadDoneActionTimerFired)
     , m_autoLoadImages(true)
     , m_loadFinishing(false)
@@ -520 +521 @@
     if (frame())
         frame()->loader()->loadDone();
+
+    if (!request) {
+        // If the request passed to this function is null, loadDone finished synchronously from when
+        // the load was started, so we want to kick off our next set of loads (via checkForPendingPreloads
+        // and servePendingRequests) asynchronously.
+        m_loadDoneActionTimer.startOneShot(0);
+        return;
+    }
+
+    performPostLoadActions();
+}
+
+void CachedResourceLoader::loadDoneActionTimerFired(Timer<CachedResourceLoader>*)
+{
+    performPostLoadActions();
+}
+
+void CachedResourceLoader::performPostLoadActions()
+{
     checkForPendingPreloads();
     resourceLoadScheduler()->servePendingRequests();
@@ -584 +604 @@
 void CachedResourceLoader::checkForPendingPreloads() 
 {
-    unsigned count = m_pendingPreloads.size();
-    if (!count || !m_document->body() || !m_document->body()->renderer())
-        return;
-    for (unsigned i = 0; i < count; ++i) {
-        PendingPreload& preload = m_pendingPreloads[i];
+    if (m_pendingPreloads.isEmpty() || !m_document->body() || !m_document->body()->renderer())
+        return;
+    while (!m_pendingPreloads.isEmpty()) {
+        PendingPreload preload = m_pendingPreloads.takeFirst();
         // Don't request preload if the resource already loaded normally (this will result in double load if the page is being reloaded with cached results ignored).
         if (!cachedResource(m_document->completeURL(preload.m_url)))

trunk/Source/WebCore/loader/cache/CachedResourceLoader.h

@@ -31 +31 @@
 #include "CachePolicy.h"
 #include "ResourceLoadPriority.h"
+#include "Timer.h"
+#include <wtf/Deque.h>
 #include <wtf/HashMap.h>
 #include <wtf/HashSet.h>
@@ -118 +120 @@
     void notifyLoadedFromMemoryCache(CachedResource*);
     bool canRequest(CachedResource::Type, const KURL&);
+
+    void loadDoneActionTimerFired(Timer<CachedResourceLoader>*);
+
+    void performPostLoadActions();
     
     HashSet<String> m_validatedURLs;
@@ -134 +140 @@
         String m_charset;
     };
-    Vector<PendingPreload> m_pendingPreloads;
+    Deque<PendingPreload> m_pendingPreloads;
+
+    Timer<CachedResourceLoader> m_loadDoneActionTimer;
     
     //29 bits left

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2011-01-25  Brian Weinstein  <bweinstein@apple.com>
+
+        Reviewed by Antti Koivisto.
+
+        Crashes loading pages when cancelling subresource loads through WebKit
+        https://bugs.webkit.org/show_bug.cgi?id=53123
+        <rdar://problem/8914361>
+
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForMIMEType): If our URL is null, return early instead of dispatching
+            a message.
+
 2011-01-25  Chris Fleizach  <cfleizach@apple.com>
 

trunk/Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp

@@ -618 +618 @@
     uint64_t listenerID = m_frame->setUpPolicyListener(function);
     const String& url = request.url().string(); // FIXME: Pass entire request.
+    if (!url)
+        return;
 
     bool receivedPolicyAction;