Source/WebCore: Reuse CachedRawResources (e.g., XHRs) that are stored
authorjaphet@chromium.org <japhet@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Feb 2012 06:45:41 +0000 (06:45 +0000)
committerjaphet@chromium.org <japhet@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 14 Feb 2012 06:45:41 +0000 (06:45 +0000)
in the MemoryCache when appropriate.
https://bugs.webkit.org/show_bug.cgi?id=76564

Reviewed by Antti Koivisto.

No new tests, expected behavior covered by existing tests.

* html/DOMURL.cpp:
(WebCore::DOMURL::revokeObjectURL): Objects shouldn't remain in the
    MemoryCache if revokeObjectURL is called on them.
* inspector/InspectorPageAgent.cpp:
(WebCore::InspectorPageAgent::cachedResourceContent): Add CachedRawResource support.
* inspector/InspectorResourceAgent.cpp:
(WebCore::InspectorResourceAgent::setCacheDisabled): Immediately
    evict resources, rather than waiting for navigation, since XHRs
    should hit the cache if it has been disabled.
* loader/cache/CachedRawResource.cpp:
(CachedRawResourceCallback): Encapsulates the async callback for
    a cache hit for CachedRawResources.
(WebCore::CachedRawResource::sendCallbacks): Do the work defered in didAddClient.
(WebCore::CachedRawResource::didAddClient): Scheduled a CachedRawResourceCallback if
    we already have a response, since async XHRs may not play nicely with receiving
    their data synchronously.
(WebCore::CachedRawResource::removeClient): Ensure we cancel a callback to a client if
    it removes itself.
(WebCore::CachedRawResource::canReuse): Provide some basic rules for when a
    CachedRawResource can be reused.
* loader/cache/CachedRawResource.h:
* loader/cache/CachedResource.h:
* loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::determineRevalidationPolicy): Don't automatically reload
    CachedRawResources, and add a check for whether this request has already been
    made conditional.
* xml/XMLHttpRequest.cpp:

LayoutTests: Test update for https://bugs.webkit.org/show_bug.cgi?id=76564.

Use sync XHRs instead of async in network-content-replacement-xhr.html,
since the inspector pulls async XHR data off of its CachedResource, instead
of a buffer it controls.

Reviewed by Antti Koivisto.

* http/tests/inspector/network/network-content-replacement-xhr-expected.txt:
* http/tests/inspector/network/network-content-replacement-xhr.html:

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/inspector/network/network-content-replacement-xhr-expected.txt
LayoutTests/http/tests/inspector/network/network-content-replacement-xhr.html
Source/WebCore/ChangeLog
Source/WebCore/html/DOMURL.cpp
Source/WebCore/inspector/InspectorPageAgent.cpp
Source/WebCore/inspector/InspectorResourceAgent.cpp
Source/WebCore/loader/cache/CachedRawResource.cpp
Source/WebCore/loader/cache/CachedRawResource.h
Source/WebCore/loader/cache/CachedResource.h
Source/WebCore/loader/cache/CachedResourceLoader.cpp
Source/WebCore/xml/XMLHttpRequest.cpp

index 28637b9..bb11cfe 100644 (file)
@@ -1,3 +1,16 @@
+2012-02-13  Nate Chapin  <japhet@chromium.org>
+
+        Test update for https://bugs.webkit.org/show_bug.cgi?id=76564.
+
+        Use sync XHRs instead of async in network-content-replacement-xhr.html,
+        since the inspector pulls async XHR data off of its CachedResource, instead
+        of a buffer it controls.
+
+        Reviewed by Antti Koivisto.
+
+        * http/tests/inspector/network/network-content-replacement-xhr-expected.txt:
+        * http/tests/inspector/network/network-content-replacement-xhr.html:
+
 2012-02-13  Noel Gordon  <noel.gordon@gmail.com>
 
         [chromium] Rebaseline JPEG image results after r107389
index 242de14..d1ef73f 100644 (file)
@@ -2,11 +2,11 @@ CONSOLE MESSAGE: line 39: Done.
 Tests NetworkResourcesData logic for XHR content replacement.
 
 http://127.0.0.1:8000/inspector/network/resources/resource.php?size=200
-resource.content: 
+resource.content: null
 http://127.0.0.1:8000/inspector/network/resources/resource.php?size=100
 resource.content: ****************************************************************************************************
 http://127.0.0.1:8000/inspector/network/resources/resource.php?size=201
-resource.content: 
+resource.content: null
 http://127.0.0.1:8000/inspector/network/resources/resource.php?size=100
 resource.content: ****************************************************************************************************
 
index a893d09..0c76f8b 100644 (file)
@@ -16,22 +16,22 @@ function loadData()
     // discard third resource content once we see its size exceeds limit,
     // and finally replace first resource content with the last resource content.
 
-    doXHR("GET", "resources/resource.php?size=200", true, xhrLoaded1);
+    doXHR("GET", "resources/resource.php?size=200", false, xhrLoaded1);
 }
 
 function xhrLoaded1()
 {
-    doXHR("GET", "resources/resource.php?size=100", true, xhrLoaded2);
+    doXHR("GET", "resources/resource.php?size=100", false, xhrLoaded2);
 }
 
 function xhrLoaded2()
 {
-    doXHR("GET", "resources/resource.php?size=201", true, xhrLoaded3);
+    doXHR("GET", "resources/resource.php?size=201", false, xhrLoaded3);
 }
 
 function xhrLoaded3()
 {
-    doXHR("GET", "resources/resource.php?size=100", true, allXHRsLoaded);
+    doXHR("GET", "resources/resource.php?size=100", false, allXHRsLoaded);
 }
 
 function allXHRsLoaded()
index 4a6f32c..92ab5b4 100644 (file)
@@ -1,3 +1,41 @@
+2012-02-13  Nate Chapin  <japhet@chromium.org>
+
+        Reuse CachedRawResources (e.g., XHRs) that are stored
+        in the MemoryCache when appropriate.
+        https://bugs.webkit.org/show_bug.cgi?id=76564
+
+        Reviewed by Antti Koivisto.
+
+        No new tests, expected behavior covered by existing tests.
+
+        * html/DOMURL.cpp:
+        (WebCore::DOMURL::revokeObjectURL): Objects shouldn't remain in the
+            MemoryCache if revokeObjectURL is called on them.
+        * inspector/InspectorPageAgent.cpp:
+        (WebCore::InspectorPageAgent::cachedResourceContent): Add CachedRawResource support.
+        * inspector/InspectorResourceAgent.cpp:
+        (WebCore::InspectorResourceAgent::setCacheDisabled): Immediately
+            evict resources, rather than waiting for navigation, since XHRs
+            should hit the cache if it has been disabled.
+        * loader/cache/CachedRawResource.cpp:
+        (CachedRawResourceCallback): Encapsulates the async callback for
+            a cache hit for CachedRawResources.
+        (WebCore::CachedRawResource::sendCallbacks): Do the work defered in didAddClient.
+        (WebCore::CachedRawResource::didAddClient): Scheduled a CachedRawResourceCallback if
+            we already have a response, since async XHRs may not play nicely with receiving
+            their data synchronously.
+        (WebCore::CachedRawResource::removeClient): Ensure we cancel a callback to a client if
+            it removes itself.
+        (WebCore::CachedRawResource::canReuse): Provide some basic rules for when a
+            CachedRawResource can be reused.
+        * loader/cache/CachedRawResource.h:
+        * loader/cache/CachedResource.h:
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::determineRevalidationPolicy): Don't automatically reload
+            CachedRawResources, and add a check for whether this request has already been
+            made conditional.
+        * xml/XMLHttpRequest.cpp:
+
 2012-02-13  Dana Jansens  <danakj@chromium.org>
 
         [chromium] Set opaque flag on SkBitmap in per-tile layer updater
index 086bf77..36a401a 100644 (file)
@@ -34,6 +34,7 @@
 #include "Blob.h"
 #include "BlobURL.h"
 #include "KURL.h"
+#include "MemoryCache.h"
 #include "PublicURLManager.h"
 #include "ScriptExecutionContext.h"
 #include "SecurityOrigin.h"
@@ -89,6 +90,8 @@ void DOMURL::revokeObjectURL(ScriptExecutionContext* scriptExecutionContext, con
         return;
 
     KURL url(KURL(), urlString);
+    if (CachedResource* resource = memoryCache()->resourceForURL(url))
+        memoryCache()->remove(resource);
 
     HashSet<String>& blobURLs = scriptExecutionContext->publicURLManager().blobURLs();
     if (blobURLs.contains(url.string())) {
index 758912e..aaf2606 100644 (file)
@@ -121,7 +121,7 @@ static bool prepareCachedResourceBuffer(CachedResource* cachedResource, bool* ha
 static bool hasTextContent(CachedResource* cachedResource)
 {
     InspectorPageAgent::ResourceType type = InspectorPageAgent::cachedResourceType(*cachedResource);
-    return type == InspectorPageAgent::StylesheetResource || type == InspectorPageAgent::ScriptResource;
+    return type == InspectorPageAgent::StylesheetResource || type == InspectorPageAgent::ScriptResource || type == InspectorPageAgent::XHRResource;
 }
 
 bool InspectorPageAgent::cachedResourceContent(CachedResource* cachedResource, String* result, bool* base64Encoded)
@@ -143,6 +143,7 @@ bool InspectorPageAgent::cachedResourceContent(CachedResource* cachedResource, S
     }
 
     if (cachedResource) {
+        SharedBuffer* buffer = cachedResource->data();
         switch (cachedResource->type()) {
         case CachedResource::CSSStyleSheet:
             *result = static_cast<CachedCSSStyleSheet*>(cachedResource)->sheetText();
@@ -150,6 +151,9 @@ bool InspectorPageAgent::cachedResourceContent(CachedResource* cachedResource, S
         case CachedResource::Script:
             *result = static_cast<CachedScript*>(cachedResource)->script();
             return true;
+        case CachedResource::RawResource:
+            *result = String(buffer->data(), buffer->size());
+            return true;
         default:
             if (hasZeroSize) {
                 *result = "";
@@ -254,6 +258,8 @@ InspectorPageAgent::ResourceType InspectorPageAgent::cachedResourceType(const Ca
         return InspectorPageAgent::StylesheetResource;
     case CachedResource::Script:
         return InspectorPageAgent::ScriptResource;
+    case CachedResource::RawResource:
+        return InspectorPageAgent::XHRResource;
     default:
         break;
     }
index ca3b0e9..e07f64f 100644 (file)
@@ -253,7 +253,7 @@ void InspectorResourceAgent::didReceiveResponse(unsigned long identifier, Docume
             type = InspectorPageAgent::ScriptResource;
         else if (equalIgnoringFragmentIdentifier(response.url(), loader->frameLoader()->icon()->url()))
             type = InspectorPageAgent::ImageResource;
-        else if (equalIgnoringFragmentIdentifier(response.url(), loader->url()) && type == InspectorPageAgent::OtherResource)
+        else if (equalIgnoringFragmentIdentifier(response.url(), loader->url()) && !loader->isCommitted())
             type = InspectorPageAgent::DocumentResource;
 
         m_resourcesData->responseReceived(requestId, m_pageAgent->frameId(loader->frame()), response);
@@ -529,6 +529,8 @@ void InspectorResourceAgent::clearBrowserCookies(ErrorString*)
 void InspectorResourceAgent::setCacheDisabled(ErrorString*, bool cacheDisabled)
 {
     m_state->setBoolean(ResourceAgentState::cacheDisabled, cacheDisabled);
+    if (cacheDisabled)
+        memoryCache()->evictResources();
 }
 
 void InspectorResourceAgent::mainFrameNavigated(DocumentLoader* loader)
index 63f81c9..de6f517 100644 (file)
@@ -37,18 +37,21 @@ namespace WebCore {
 
 CachedRawResource::CachedRawResource(ResourceRequest& resourceRequest)
     : CachedResource(resourceRequest, RawResource)
-    , m_dataLength(0)
+    , m_identifier(0)
 {
 }
 
 void CachedRawResource::data(PassRefPtr<SharedBuffer> data, bool allDataReceived)
 {
     CachedResourceHandle<CachedRawResource> protect(this);
+    if (!m_identifier)
+        m_identifier = m_loader->identifier();
+
     if (data) {
         // If we are buffering data, then we are saving the buffer in m_data and need to manually
         // calculate the incremental data. If we are not buffering, then m_data will be null and
         // the buffer contains only the incremental data.
-        size_t previousDataLength = (m_options.shouldBufferData == BufferData) ? m_dataLength : 0;
+        size_t previousDataLength = (m_options.shouldBufferData == BufferData) ? encodedSize() : 0;
         ASSERT(data->size() >= previousDataLength);
         const char* incrementalData = data->data() + previousDataLength;
         size_t incrementalDataLength = data->size() - previousDataLength;
@@ -61,19 +64,82 @@ void CachedRawResource::data(PassRefPtr<SharedBuffer> data, bool allDataReceived
     }
     
     if (m_options.shouldBufferData == BufferData) {
-        m_dataLength = data ? data->size() : 0;
+        if (data)
+            setEncodedSize(data->size());
         m_data = data;
     }
     CachedResource::data(m_data, allDataReceived);
 }
 
+class CachedRawResourceCallback {
+public:    
+    static CachedRawResourceCallback* schedule(CachedRawResource* resource, CachedRawResourceClient* client)
+    {
+        return new CachedRawResourceCallback(resource, client);
+    }
+
+    void cancel()
+    {
+        if (m_callbackTimer.isActive())
+            m_callbackTimer.stop();
+    }
+
+private:
+    CachedRawResourceCallback(CachedRawResource* resource, CachedRawResourceClient* client)
+        : m_resource(resource)
+        , m_client(client)
+        , m_callbackTimer(this, &CachedRawResourceCallback::timerFired)
+    {
+        m_callbackTimer.startOneShot(0);
+    }
+
+    void timerFired(Timer<CachedRawResourceCallback>*)
+    {
+        m_resource->sendCallbacks(m_client);
+    }
+    CachedResourceHandle<CachedRawResource> m_resource;
+    CachedRawResourceClient* m_client;
+    Timer<CachedRawResourceCallback> m_callbackTimer;
+};
+
+void CachedRawResource::sendCallbacks(CachedRawResourceClient* c)
+{
+    if (!m_clientsAwaitingCallback.contains(c))
+        return;
+    m_clientsAwaitingCallback.remove(c);
+    c->responseReceived(this, m_response);
+    if (!m_clients.contains(c) || !m_data)
+        return;
+    c->dataReceived(this, m_data->data(), m_data->size());
+    if (!m_clients.contains(c) || isLoading())
+       return;
+    c->notifyFinished(this);
+}
+
 void CachedRawResource::didAddClient(CachedResourceClient* c)
 {
-    if (m_data) {
-        static_cast<CachedRawResourceClient*>(c)->responseReceived(this, m_response);
-        static_cast<CachedRawResourceClient*>(c)->dataReceived(this, m_data->data(), m_data->size());
-    }
-    CachedResource::didAddClient(c);
+    if (m_response.isNull())
+        return;
+
+    // CachedRawResourceClients (especially XHRs) do crazy things if an asynchronous load returns
+    // synchronously (e.g., scripts may not have set all the state they need to handle the load).
+    // Therefore, rather than immediately sending callbacks on a cache hit like other CachedResources,
+    // we schedule the callbacks and ensure we never finish synchronously.
+    // FIXME: We also use an async callback on 304 because we don't have sufficient information to
+    // know whether we are receiving new clients because of revalidation or pure reuse. Perhaps move
+    // this logic to CachedResource?
+    CachedRawResourceClient* client = static_cast<CachedRawResourceClient*>(c);
+    ASSERT(!m_clientsAwaitingCallback.contains(client));
+    m_clientsAwaitingCallback.add(client, adoptPtr(CachedRawResourceCallback::schedule(this, client)));
+}
+void CachedRawResource::removeClient(CachedResourceClient* c)
+{
+    OwnPtr<CachedRawResourceCallback> callback = m_clientsAwaitingCallback.take(static_cast<CachedRawResourceClient*>(c));
+    if (callback)
+        callback->cancel();
+    callback.clear();
+    CachedResource::removeClient(c);
 }
 
 void CachedRawResource::allClientsRemoved()
@@ -113,9 +179,15 @@ void CachedRawResource::setDefersLoading(bool defers)
         m_loader->setDefersLoading(defers);
 }
 
-unsigned long CachedRawResource::identifier() const
+bool CachedRawResource::canReuse() const
 {
-    return m_loader ? m_loader->identifier() : 0;
+    if (m_options.shouldBufferData == DoNotBufferData)
+        return false;
+
+    if (m_resourceRequest.httpMethod() != "GET")
+        return false;
+
+    return true;
 }
 
 } // namespace WebCore
index 9b0cab5..1f39967 100644 (file)
@@ -27,6 +27,8 @@
 #include "CachedResourceClient.h"
 
 namespace WebCore {
+class CachedRawResourceCallback;
+class CachedRawResourceClient;
 
 class CachedRawResource : public CachedResource {
 public:
@@ -38,7 +40,12 @@ public:
     virtual void setDefersLoading(bool);
     
     // FIXME: This is exposed for the InpsectorInstrumentation for preflights in DocumentThreadableLoader. It's also really lame.
-    unsigned long identifier() const;
+    unsigned long identifier() const { return m_identifier; }
+
+    bool canReuse() const;
+    void sendCallbacks(CachedRawResourceClient*);
+
+    virtual void removeClient(CachedResourceClient*);
 
 private:
     virtual void didAddClient(CachedResourceClient*);
@@ -54,7 +61,8 @@ private:
     virtual void didDownloadData(int);
 #endif
 
-    size_t m_dataLength;
+    unsigned long m_identifier;
+    HashMap<CachedRawResourceClient*, OwnPtr<CachedRawResourceCallback> > m_clientsAwaitingCallback;
 };
 
 
index b87da71..2a4be43 100644 (file)
@@ -110,7 +110,7 @@ public:
     void setLoadPriority(ResourceLoadPriority);
 
     void addClient(CachedResourceClient*);
-    void removeClient(CachedResourceClient*);
+    virtual void removeClient(CachedResourceClient*);
     bool hasClients() const { return !m_clients.isEmpty(); }
     void deleteIfPossible();
 
index 5df4a0d..9b40bd4 100644 (file)
@@ -523,8 +523,13 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
         return Reload;
     }
 
-    // FIXME: Currently, all CachedRawResources are always reloaded. Some of them should be cacheable.
-    if (existingResource->type() == CachedResource::RawResource)
+    if (existingResource->type() == CachedResource::RawResource && !static_cast<CachedRawResource*>(existingResource)->canReuse())
+         return Reload;
+
+    // Certain requests (e.g., XHRs) might have manually set headers that require revalidation.
+    // FIXME: In theory, this should be a Revalidate case. In practice, the MemoryCache revalidation path assumes a whole bunch
+    // of things about how revalidation works that manual headers violate, so punt to Reload instead.
+    if (request.isConditional())
         return Reload;
     
     // Don't reload resources while pasting.
index 7ed8212..1ea6dd4 100644 (file)
@@ -1037,7 +1037,10 @@ void XMLHttpRequest::didFinishLoading(unsigned long identifier, double)
     // FIXME: Set m_responseBlob to something here in the ResponseTypeBlob case.
 #endif
 
-    InspectorInstrumentation::resourceRetrievedByXMLHttpRequest(scriptExecutionContext(), identifier, m_responseBuilder.toStringPreserveCapacity(), m_url, m_lastSendURL, m_lastSendLineNumber);
+    // For Asynchronous XHRs, the inspector can grab the data directly off of the CachedResource. For sync XHRs, we need to
+    // provide the data here, since no CachedResource was involved.
+    if (!m_async)
+        InspectorInstrumentation::resourceRetrievedByXMLHttpRequest(scriptExecutionContext(), identifier, m_responseBuilder.toStringPreserveCapacity(), m_url, m_lastSendURL, m_lastSendLineNumber);
 
     bool hadLoader = m_loader;
     m_loader = 0;