Bug 76564 – Let MemoryCache reuse cached XHRs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Nate Chapin]

https://bugs.webkit.org/show_bug.cgi?id=61225 re-plumbed ThreadableLoaderClients (XHRs et al) so that they are loaded through the MemoryCache infrastructure.  As of right now, we cache all of these resources, but we always through away those cached results.  Now that this plumbing appears to have solidified, I'd like to turn on caching for certain classes of these requests.

I'm going to try to err on the side of caution in what resources are considered for reuse, but I'd appreciate any suggestions on what the rules should be.

First attempt coming shortly.

[Nate Chapin]

Created attachment 122979 [details]
First draft

[Adam Barth]

Comment on attachment 122979 [details]
First draft

View in context: https://bugs.webkit.org/attachment.cgi?id=122979&action=review

> Source/WebCore/loader/cache/CachedRawResource.cpp:89
> +        delete this;

Consider using an OwnPtr member to own yourself.

> Source/WebCore/loader/cache/CachedRawResource.cpp:92
> +    CachedResourceClient* m_client;

What keeps the client alive?

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 122979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122979&action=review
> 
> > Source/WebCore/loader/cache/CachedRawResource.cpp:89
> > +        delete this;
> 
> Consider using an OwnPtr member to own yourself.

Sounds good.

> 
> > Source/WebCore/loader/cache/CachedRawResource.cpp:92
> > +    CachedResourceClient* m_client;
> 
> What keeps the client alive?

That part is a little subtle, and I haven't figured out a clear way to document it. The first thing CachedRawResource::sendCallbacks() does is see if the CachedResourceClient* is still in the resource's list of clients. If it's in the client list, it should be guaranteed to be alive. If it's not, we shouldn't be sending callbacks to it, regardless of whether it's alive.

[WebKit Review Bot]

Comment on attachment 122979 [details]
First draft

Attachment 122979 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11211788

New failing tests:
http/tests/inspector/network/network-disable-cache-xhrs.html

[Adam Barth]

Comment on attachment 122979 [details]
First draft

View in context: https://bugs.webkit.org/attachment.cgi?id=122979&action=review

This is looking really close.  I think we should just polish up some of the memory management idioms.

>>> Source/WebCore/loader/cache/CachedRawResource.cpp:92
>>> +    CachedResourceClient* m_client;
>> 
>> What keeps the client alive?
> 
> That part is a little subtle, and I haven't figured out a clear way to document it. The first thing CachedRawResource::sendCallbacks() does is see if the CachedResourceClient* is still in the resource's list of clients. If it's in the client list, it should be guaranteed to be alive. If it's not, we shouldn't be sending callbacks to it, regardless of whether it's alive.

I'm not sure that's exactly right.  If m_client gets deallocated and another CachedResourceClient gets allocated in its place, we could be in trouble, right?

> Source/WebCore/loader/cache/CachedRawResource.cpp:100
> +    static_cast<CachedRawResourceClient*>(c)->responseReceived(this, m_response);

I would make a local variable of type CachedRawResourceClient to avoid having to cast this variable so many times.

[Adam Barth]

> http/tests/inspector/network/network-disable-cache-xhrs.html

This is probably an interesting failure.

[Nate Chapin]

Created attachment 126329 [details]
Second attempt

I think this addresses the failing test and all of abarth's comments.

[WebKit Review Bot]

Comment on attachment 126329 [details]
Second attempt

Attachment 126329 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11485321

New failing tests:
fast/files/workers/worker-apply-blob-url-to-xhr.html

[Nate Chapin]

(In reply to comment #8)
> (From update of attachment 126329 [details])
> Attachment 126329 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11485321
> 
> New failing tests:
> fast/files/workers/worker-apply-blob-url-to-xhr.html

This failure is legitimate, though I'm not sure why it didn't happen with the first version.

I think the fix is for DOMURL::revokeObjectURL to check MemoryCache for the given url and evict that CachedResource if present.

[Nate Chapin]

Created attachment 126370 [details]
Evict from cache in DOMURL::revokeObjectURL()

[Vsevolod Vlasov]

Comment on attachment 126370 [details]
Evict from cache in DOMURL::revokeObjectURL()

View in context: https://bugs.webkit.org/attachment.cgi?id=126370&action=review

> LayoutTests/http/tests/inspector/network/network-disable-cache-xhrs.html:88
> +        InspectorTest.assertTrue(resource2.content.length == 0, "Second resource should have no content due to 304");

So you make content for the second request unavailable for inspector users and even test that this regression was successful :)
I suggest that you don't change this test in this patch, but instead add a small change to InspectorResourceAgent, making sure it never uses CachedResource to get XHR's content.

I think the following change to "if (loader) { ... }" block in InspectorResourceAgent::didReceiveResponse should be enough for this test to pass:

if (loader) {
    bool isXHR = m_loadingXHRSynchronously || m_resourcesData->resourceType(requestId) == InspectorPageAgent::XHRResource;
    CachedResource* cachedResource = InspectorPageAgent::cachedResource(loader->frame(), response.url());
    if (cachedResource) {
        cachedResourceSize = cachedResource->encodedSize();
        // FIXME: We should use CachedResource for XHR's content when possible. 
        if (!isXHR) {
            type = InspectorPageAgent::cachedResourceType(*cachedResource);
            // Use mime type from cached resource in case the one in response is empty.
            if (resourceResponse && response.mimeType().isEmpty())
                resourceResponse->setString("mimeType", cachedResource->response().mimeType());
            m_resourcesData->addCachedResource(requestId, cachedResource);
        }
    }
    if (isXHR)
        type = InspectorPageAgent::XHRResource;
    else if (m_resourcesData->resourceType(requestId) == InspectorPageAgent::ScriptResource)
        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)
        type = InspectorPageAgent::DocumentResource;

    m_resourcesData->responseReceived(requestId, m_pageAgent->frameId(loader->frame()), response);
}

I didn't check this code, though.

[Nate Chapin]

(In reply to comment #11)
> (From update of attachment 126370 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126370&action=review
> 
> > LayoutTests/http/tests/inspector/network/network-disable-cache-xhrs.html:88
> > +        InspectorTest.assertTrue(resource2.content.length == 0, "Second resource should have no content due to 304");
> 
> So you make content for the second request unavailable for inspector users and even test that this regression was successful :)
> I suggest that you don't change this test in this patch, but instead add a small change to InspectorResourceAgent, making sure it never uses CachedResource to get XHR's content.
> 

I have perhaps misunderstood. What should the inspector show in the case of a 304? The response body was empty, after all. Should it then get populated with the contents of the original response?

[Vsevolod Vlasov]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 126370 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=126370&action=review
> > 
> > > LayoutTests/http/tests/inspector/network/network-disable-cache-xhrs.html:88
> > > +        InspectorTest.assertTrue(resource2.content.length == 0, "Second resource should have no content due to 304");
> > 
> > So you make content for the second request unavailable for inspector users and even test that this regression was successful :)
> > I suggest that you don't change this test in this patch, but instead add a small change to InspectorResourceAgent, making sure it never uses CachedResource to get XHR's content.
> > 
> 
> I have perhaps misunderstood. What should the inspector show in the case of a 304? The response body was empty, after all. Should it then get populated with the contents of the original response?

We should show the body of the original response like we do for other resources (scripts, stylesheets, etc.).

[Nate Chapin]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 126370 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=126370&action=review
> > > 
> > > > LayoutTests/http/tests/inspector/network/network-disable-cache-xhrs.html:88
> > > > +        InspectorTest.assertTrue(resource2.content.length == 0, "Second resource should have no content due to 304");
> > > 
> > > So you make content for the second request unavailable for inspector users and even test that this regression was successful :)
> > > I suggest that you don't change this test in this patch, but instead add a small change to InspectorResourceAgent, making sure it never uses CachedResource to get XHR's content.
> > > 
> > 
> > I have perhaps misunderstood. What should the inspector show in the case of a 304? The response body was empty, after all. Should it then get populated with the contents of the original response?
> 
> We should show the body of the original response like we do for other resources (scripts, stylesheets, etc.).

Ok, current patch obsoleted. I'll try to figure out why that isn't just working.

[Nate Chapin]

Created attachment 126601 [details]
Fix inspector issues

[Antti Koivisto]

Comment on attachment 126601 [details]
Fix inspector issues

View in context: https://bugs.webkit.org/attachment.cgi?id=126601&action=review

Nice, a comment though.

> Source/WebCore/loader/cache/CachedRawResource.cpp:106
> +    OwnPtr<CachedRawResourceCallback> m_this;

Not a fan of the lifetime management here. It seems to me that this could be owned by m_clientsAwaitingCallback and remove itself when completed.

[Antti Koivisto]

Comment on attachment 126601 [details]
Fix inspector issues

View in context: https://bugs.webkit.org/attachment.cgi?id=126601&action=review

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:533
> +    // Certain requests (e.g., XHRs) might have manually set headers that require revalidation.
> +    // 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;

This could be a FIXME

[Nate Chapin]

Created attachment 126800 [details]
Address Antti's comments

[WebKit Review Bot]

Comment on attachment 126800 [details]
Address Antti's comments

Clearing flags on attachment: 126800

Committed r107672: <http://trac.webkit.org/changeset/107672>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #19)
> (From update of attachment 126800 [details])
> Clearing flags on attachment: 126800
> 
> Committed r107672: <http://trac.webkit.org/changeset/107672>

It broke 4 tests on Qt:
- fast/workers/worker-crash-with-invalid-location.html:
- http/tests/inspector/inspect-element.html
- http/tests/inspector/network/network-shared-worker.html
- http/tests/inspector/network/network-worker.html

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/workers/worker-crash-with-invalid-location-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/workers/worker-crash-with-invalid-location-actual.txt 
@@ -1,4 +1,3 @@
-Blocked access to external URL http://example.com/worker.js
 Blocked access to external URL http://example.com/worker.js
 Test worker fetch of blocked url. Should print a "PASS" statement.
 


--- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/inspector/inspect-element-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/inspector/inspect-element-actual.txt 
@@ -1,5 +1,6 @@
 Tests that inspect element action works for iframe children (https://bugs.webkit.org/show_bug.cgi?id=76808).
 
 
+Uncaught exception in inspector front-end: TypeError: 'null' is not an object [undefined:0]
 div#div
 


--- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/inspector/network/network-shared-worker-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/inspector/network/network-shared-worker-actual.txt 
@@ -3,7 +3,7 @@
 
 Bug 65929
 http://127.0.0.1:8000/inspector/network/resources/shared-worker.js
-resource.type: 4
+resource.type: 5
 resource.content before requesting content: undefined
 resource.content after requesting content: onconnect = function(event)
 {


--- /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/inspector/network/network-worker-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/http/tests/inspector/network/network-worker-actual.txt 
@@ -3,7 +3,7 @@
 
 Bug 65929
 http://127.0.0.1:8000/inspector/network/resources/worker.js
-resource.type: 4
+resource.type: 5
 resource.content before requesting content: undefined
 resource.content after requesting content: postMessage("Done.");

[Csaba Osztrogonác]

I skipped them to make our bots happier - http://trac.webkit.org/changeset/107692

Please unskip them with the proper fix. Thanks.

[Philippe Normand]

(In reply to comment #22)
> I skipped them to make our bots happier - http://trac.webkit.org/changeset/107692
> 
> Please unskip them with the proper fix. Thanks.

GTK is also affected.

[Nate Chapin]

(In reply to comment #21)
> (In reply to comment #19)
> - fast/workers/worker-crash-with-invalid-location.html:
> - http/tests/inspector/inspect-element.html
> - http/tests/inspector/network/network-shared-worker.html
> - http/tests/inspector/network/network-worker.html

All of those are clearly failures caused by this patch except inspect-element.html

inspect-element.html has been consistently failing on most platforms: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=http%2Ftests%2Finspector%2Finspect-element.html&group=%40ToT%20-%20webkit.org

When I --run-singly locally, the failure goes away, so I'm guessing that it's not playing nicely with some other test.

Patch to fix the other 3 failures coming soon.

[Nate Chapin]

Created attachment 127064 [details]
Fix failing worker tests

[WebKit Review Bot]

Comment on attachment 127064 [details]
Fix failing worker tests

Clearing flags on attachment: 127064

Committed r107859: <http://trac.webkit.org/changeset/107859>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because fast/workers/worker-crash-with-invalid-location.html still fails on Qt platform. I don't know what about the other platforms, because build.webkit.org is unavailable now.

--- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/workers/worker-crash-with-invalid-location-expected.txt 
+++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/workers/worker-crash-with-invalid-location-actual.txt 
@@ -1,4 +1,3 @@
-Blocked access to external URL http://example.com/worker.js
 Blocked access to external URL http://example.com/worker.js
 Test worker fetch of blocked url. Should print a "PASS" statement.

[Csaba Osztrogonác]

(In reply to comment #28)
> Reopen, because fast/workers/worker-crash-with-invalid-location.html still fails on Qt platform. I don't know what about the other platforms, because build.webkit.org is unavailable now.
> 
> --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/workers/worker-crash-with-invalid-location-expected.txt 
> +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/workers/worker-crash-with-invalid-location-actual.txt 
> @@ -1,4 +1,3 @@
> -Blocked access to external URL http://example.com/worker.js
>  Blocked access to external URL http://example.com/worker.js
>  Test worker fetch of blocked url. Should print a "PASS" statement.

I skipped it again - http://trac.webkit.org/changeset/107916/trunk/LayoutTests/platform/qt/Skipped

[Vsevolod Vlasov]

Also inspector/debugger/script-formatter-console.html is flaky on chromium.
https://bugs.webkit.org/show_bug.cgi?id=78810

[Philippe Normand]

(In reply to comment #28)
> Reopen, because fast/workers/worker-crash-with-invalid-location.html still fails on Qt platform. I don't know what about the other platforms, because build.webkit.org is unavailable now.
> 
> --- /ramdisk/qt-linux-64-release/build/layout-test-results/fast/workers/worker-crash-with-invalid-location-expected.txt 
> +++ /ramdisk/qt-linux-64-release/build/layout-test-results/fast/workers/worker-crash-with-invalid-location-actual.txt 
> @@ -1,4 +1,3 @@
> -Blocked access to external URL http://example.com/worker.js
>  Blocked access to external URL http://example.com/worker.js
>  Test worker fetch of blocked url. Should print a "PASS" statement.

Same diff on GTK as well. Will skip again.

[Aaron Colwell]

Created attachment 128034 [details]
Layout Test to verify Range request behavior.

I believe the patch for this bug broke Range requests for XHR. I've attached a layout test that reproduces the problem. It looks like the Range header is completely ignored when we have cached data.

[Nate Chapin]

(In reply to comment #32)
> Created an attachment (id=128034) [details]
> Layout Test to verify Range request behavior.
> 
> I believe the patch for this bug broke Range requests for XHR. I've attached a layout test that reproduces the problem. It looks like the Range header is completely ignored when we have cached data.

I figured there must be something like this I was missing. :)

How do you think we should handle this? My inclination would be to punt at the MemoryCache layer and say we can't cache requests with Range headers.

[Aaron Colwell]

(In reply to comment #33)
> (In reply to comment #32)
> > Created an attachment (id=128034) [details] [details]
> > Layout Test to verify Range request behavior.
> > 
> > I believe the patch for this bug broke Range requests for XHR. I've attached a layout test that reproduces the problem. It looks like the Range header is completely ignored when we have cached data.
> 
> I figured there must be something like this I was missing. :)
> 
> How do you think we should handle this? My inclination would be to punt at the MemoryCache layer and say we can't cache requests with Range headers.

Not caching requests with Range headers sounds fine with me. Trying to be "smart" about handling Ranges would likely lead to WAY more complexity than you want to deal with.

[Nate Chapin]

Created attachment 128297 [details]
Fix Range header handling

[WebKit Review Bot]

Comment on attachment 128297 [details]
Fix Range header handling

Clearing flags on attachment: 128297

Committed r108576: <http://trac.webkit.org/changeset/108576>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Oliver Hunt]

Apparently this caused a regression: https://bugs.webkit.org/show_bug.cgi?id=76564

[Vsevolod Vlasov]

(In reply to comment #38)
> Apparently this caused a regression: https://bugs.webkit.org/show_bug.cgi?id=76564

Wrong bug link

[Oliver Hunt]

(In reply to comment #39)
> (In reply to comment #38)
> > Apparently this caused a regression: https://bugs.webkit.org/show_bug.cgi?id=76564
> 
> Wrong bug link

/me whistles nonchalantly

I meant https://bugs.webkit.org/show_bug.cgi?id=79325 of course ;)

[Alexey Proskuryakov]

This also caused bug 80551.