Changeset 188150 in webkit

Timestamp:

Aug 7, 2015 1:10:42 PM (9 years ago)

Author:

aestes@apple.com

Message:

Crash when following a Google search link to Twitter with Limit Adult Content enabled
​https://bugs.webkit.org/show_bug.cgi?id=147651

Reviewed by Brady Eidson.

When a loaded CachedRawResource gets a new client, it synthesizes the callbacks that the new client would have
received while the resource was loading. Unlike a real network load, it synthesizes these callbacks in a single
run loop iteration. When DocumentLoader receives a redirect, and finds substitute data in the app cache for the
redirect URL, it schedules a timer that removes DocumentLoader as a client of the CachedRawResource then
synthesizes its own set of CachedRawResourceClient callbacks. But since CachedRawResource has already delivered
client callbacks before the app cache timer fires, DocumentLoader unexpectedly ends up getting two sets of
client callbacks and badness ensues.

The fix is to let CachedRawResource detect if a redirect will trigger the client to load substitute data. If so,
stop delivering client callbacks.

Layout test to follow.

• loader/DocumentLoader.cpp:

(WebCore::DocumentLoader::syntheticRedirectReceived): If there is valid substitute data, do not continue.

• loader/DocumentLoader.h:
• loader/cache/CachedRawResource.cpp: Returned early if syntheticRedirectReceived() said not to continue.

(WebCore::CachedRawResource::didAddClient):

• loader/cache/CachedRawResourceClient.h:

(WebCore::CachedRawResourceClient::syntheticRedirectReceived):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• loader/DocumentLoader.cpp (modified) (1 diff)
• loader/DocumentLoader.h (modified) (1 diff)
• loader/cache/CachedRawResource.cpp (modified) (1 diff)
• loader/cache/CachedRawResourceClient.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2015-08-07  Andy Estes  <aestes@apple.com>
+
+        Crash when following a Google search link to Twitter with Limit Adult Content enabled
+        https://bugs.webkit.org/show_bug.cgi?id=147651
+
+        Reviewed by Brady Eidson.
+
+        When a loaded CachedRawResource gets a new client, it synthesizes the callbacks that the new client would have
+        received while the resource was loading. Unlike a real network load, it synthesizes these callbacks in a single
+        run loop iteration. When DocumentLoader receives a redirect, and finds substitute data in the app cache for the
+        redirect URL, it schedules a timer that removes DocumentLoader as a client of the CachedRawResource then
+        synthesizes its own set of CachedRawResourceClient callbacks. But since CachedRawResource has already delivered
+        client callbacks before the app cache timer fires, DocumentLoader unexpectedly ends up getting two sets of
+        client callbacks and badness ensues.
+
+        The fix is to let CachedRawResource detect if a redirect will trigger the client to load substitute data. If so,
+        stop delivering client callbacks.
+
+        Layout test to follow.
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::syntheticRedirectReceived): If there is valid substitute data, do not continue.
+        * loader/DocumentLoader.h:
+        * loader/cache/CachedRawResource.cpp: Returned early if syntheticRedirectReceived() said not to continue.
+        (WebCore::CachedRawResource::didAddClient):
+        * loader/cache/CachedRawResourceClient.h:
+        (WebCore::CachedRawResourceClient::syntheticRedirectReceived):
+
 2015-08-06  Dean Jackson  <dino@apple.com>
 

trunk/Source/WebCore/loader/DocumentLoader.cpp

@@ -490 +490 @@
 }
 
+void DocumentLoader::syntheticRedirectReceived(CachedResource* resource, ResourceRequest& request, const ResourceResponse& redirectResponse, bool& shouldContinue)
+{
+    redirectReceived(resource, request, redirectResponse);
+
+    // If we will soon remove our reference to the CachedRawResource in favor of a SubstituteData load, we don't want to continue receiving synthetic CachedRawResource callbacks.
+    shouldContinue = !m_substituteData.isValid();
+}
+
 void DocumentLoader::willSendRequest(ResourceRequest& newRequest, const ResourceResponse& redirectResponse)
 {

trunk/Source/WebCore/loader/DocumentLoader.h

@@ -307 +307 @@
         void mainReceivedError(const ResourceError&);
         WEBCORE_EXPORT virtual void redirectReceived(CachedResource*, ResourceRequest&, const ResourceResponse&) override;
+        WEBCORE_EXPORT virtual void syntheticRedirectReceived(CachedResource*, ResourceRequest&, const ResourceResponse&, bool& /* shouldContinue */) override;
         WEBCORE_EXPORT virtual void responseReceived(CachedResource*, const ResourceResponse&) override;
         WEBCORE_EXPORT virtual void dataReceived(CachedResource*, const char* data, int length) override;

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

@@ -133 +133 @@
         RedirectPair redirect = m_redirectChain[i];
         ResourceRequest request(redirect.m_request);
-        client->redirectReceived(this, request, redirect.m_redirectResponse);
-        if (!hasClient(c))
+        bool shouldContinue = true;
+        client->syntheticRedirectReceived(this, request, redirect.m_redirectResponse, shouldContinue);
+        if (!hasClient(c) || !shouldContinue)
             return;
     }

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

@@ -42 +42 @@
     virtual void dataReceived(CachedResource*, const char* /* data */, int /* length */) { }
     virtual void redirectReceived(CachedResource*, ResourceRequest&, const ResourceResponse&) { }
+
+    // In response to a redirect, some clients wish to receive no futher callbacks, but cannot immediately remove themselves as a client.
+    // Those clients can express their desire to recieve no futher callbacks by setting shouldContinue to false.
+    virtual void syntheticRedirectReceived(CachedResource* resource, ResourceRequest& request, const ResourceResponse& response, bool& /* shouldContinue */) { redirectReceived(resource, request, response); }
 #if USE(SOUP)
     virtual char* getOrCreateReadBuffer(CachedResource*, size_t /* requestedSize */, size_t& /* actualSize */) { return 0; }