Bug 76564 - Let MemoryCache reuse cached XHRs
Summary: Let MemoryCache reuse cached XHRs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on: 78810 79026
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-18 13:17 PST by Nate Chapin
Modified: 2012-03-07 16:54 PST (History)
11 users (show)

See Also:


Attachments
First draft (6.16 KB, patch)
2012-01-18 13:23 PST, Nate Chapin
abarth: review-
Details | Formatted Diff | Diff
Second attempt (10.80 KB, patch)
2012-02-09 10:46 PST, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Evict from cache in DOMURL::revokeObjectURL() (11.63 KB, patch)
2012-02-09 14:20 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
Fix inspector issues (17.88 KB, patch)
2012-02-10 16:18 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
Address Antti's comments (17.81 KB, patch)
2012-02-13 11:24 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
Fix failing worker tests (4.06 KB, patch)
2012-02-14 15:40 PST, Nate Chapin
no flags Details | Formatted Diff | Diff
Layout Test to verify Range request behavior. (4.15 KB, patch)
2012-02-21 13:32 PST, Aaron Colwell
no flags Details | Formatted Diff | Diff
Fix Range header handling (6.41 KB, patch)
2012-02-22 15:07 PST, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2012-01-18 13:17:18 PST
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.
Comment 1 Nate Chapin 2012-01-18 13:23:01 PST
Created attachment 122979 [details]
First draft
Comment 2 Adam Barth 2012-01-18 13:49:56 PST
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?
Comment 3 Nate Chapin 2012-01-18 13:52:54 PST
(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.
Comment 4 WebKit Review Bot 2012-01-18 14:13:39 PST
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
Comment 5 Adam Barth 2012-01-25 02:03:09 PST
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.
Comment 6 Adam Barth 2012-01-25 02:03:31 PST
> http/tests/inspector/network/network-disable-cache-xhrs.html

This is probably an interesting failure.
Comment 7 Nate Chapin 2012-02-09 10:46:13 PST
Created attachment 126329 [details]
Second attempt

I think this addresses the failing test and all of abarth's comments.
Comment 8 WebKit Review Bot 2012-02-09 11:38:28 PST
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
Comment 9 Nate Chapin 2012-02-09 12:25:35 PST
(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.
Comment 10 Nate Chapin 2012-02-09 14:20:42 PST
Created attachment 126370 [details]
Evict from cache in DOMURL::revokeObjectURL()
Comment 11 Vsevolod Vlasov 2012-02-09 14:21:52 PST
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.
Comment 12 Nate Chapin 2012-02-09 14:28:15 PST
(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?
Comment 13 Vsevolod Vlasov 2012-02-09 14:33:56 PST
(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.).
Comment 14 Nate Chapin 2012-02-09 14:37:11 PST
(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.
Comment 15 Nate Chapin 2012-02-10 16:18:30 PST
Created attachment 126601 [details]
Fix inspector issues
Comment 16 Antti Koivisto 2012-02-11 13:37:11 PST
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.
Comment 17 Antti Koivisto 2012-02-11 13:38:58 PST
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
Comment 18 Nate Chapin 2012-02-13 11:24:34 PST
Created attachment 126800 [details]
Address Antti's comments
Comment 19 WebKit Review Bot 2012-02-13 22:46:08 PST
Comment on attachment 126800 [details]
Address Antti's comments

Clearing flags on attachment: 126800

Committed r107672: <http://trac.webkit.org/changeset/107672>
Comment 20 WebKit Review Bot 2012-02-13 22:46:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Csaba Osztrogonác 2012-02-14 01:34:38 PST
(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.");
Comment 22 Csaba Osztrogonác 2012-02-14 02:07:31 PST
I skipped them to make our bots happier - http://trac.webkit.org/changeset/107692

Please unskip them with the proper fix. Thanks.
Comment 23 Philippe Normand 2012-02-14 08:10:35 PST
(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.
Comment 24 Nate Chapin 2012-02-14 12:02:30 PST
(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.
Comment 25 Nate Chapin 2012-02-14 15:40:08 PST
Created attachment 127064 [details]
Fix failing worker tests
Comment 26 WebKit Review Bot 2012-02-15 17:19:44 PST
Comment on attachment 127064 [details]
Fix failing worker tests

Clearing flags on attachment: 127064

Committed r107859: <http://trac.webkit.org/changeset/107859>
Comment 27 WebKit Review Bot 2012-02-15 17:19:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Csaba Osztrogonác 2012-02-16 01:32:03 PST
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.
Comment 29 Csaba Osztrogonác 2012-02-16 02:37:52 PST
(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
Comment 30 Vsevolod Vlasov 2012-02-16 07:00:26 PST
Also inspector/debugger/script-formatter-console.html is flaky on chromium.
https://bugs.webkit.org/show_bug.cgi?id=78810
Comment 31 Philippe Normand 2012-02-16 07:38:40 PST
(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.
Comment 32 Aaron Colwell 2012-02-21 13:32:45 PST
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.
Comment 33 Nate Chapin 2012-02-21 13:39:40 PST
(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.
Comment 34 Aaron Colwell 2012-02-21 14:03:52 PST
(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.
Comment 35 Nate Chapin 2012-02-22 15:07:31 PST
Created attachment 128297 [details]
Fix Range header handling
Comment 36 WebKit Review Bot 2012-02-22 16:46:48 PST
Comment on attachment 128297 [details]
Fix Range header handling

Clearing flags on attachment: 128297

Committed r108576: <http://trac.webkit.org/changeset/108576>
Comment 37 WebKit Review Bot 2012-02-22 16:46:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Oliver Hunt 2012-03-05 11:21:53 PST
Apparently this caused a regression: https://bugs.webkit.org/show_bug.cgi?id=76564
Comment 39 Vsevolod Vlasov 2012-03-05 11:23:35 PST
(In reply to comment #38)
> Apparently this caused a regression: https://bugs.webkit.org/show_bug.cgi?id=76564

Wrong bug link
Comment 40 Oliver Hunt 2012-03-05 11:24:39 PST
(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 ;)
Comment 41 Alexey Proskuryakov 2012-03-07 16:54:38 PST
This also caused bug 80551.