Send synchronous loads through the cache.

Send synchronous loads through the cache.

The logic for actually performing the sync request now lives in ResourceLoader.

BUG=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158065

[Nate Chapin, 2013-09-12 23:28:45]

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/fast/dom/xmlht...
File
LayoutTests/fast/dom/xmlhttprequest-constructor-in-detached-document-expected.txt
(right):

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/fast/dom/xmlht...
LayoutTests/fast/dom/xmlhttprequest-constructor-in-detached-document-expected.txt:1:
CONSOLE MESSAGE: line 15: Uncaught NetworkError: A network error occurred.
This is benign, the load was getting cancelled before, we just weren't logging
it.

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/http/tests/app...
File LayoutTests/http/tests/appcache/resources/remove-cache-frame-2.html
(right):

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/http/tests/app...
LayoutTests/http/tests/appcache/resources/remove-cache-frame-2.html:20:
req.setRequestHeader("Cache-control", "no-store");
Header added to make sure this gets all the way to the appcache

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/http/tests/app...
File LayoutTests/http/tests/appcache/resources/remove-cache-frame.html (right):

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/http/tests/app...
LayoutTests/http/tests/appcache/resources/remove-cache-frame.html:23:
req.setRequestHeader("Cache-control", "no-store");
Header added to make sure this gets all the way to the appcache

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/http/tests/ins...
File
LayoutTests/http/tests/inspector/network/network-content-replacement-xhr-expected.txt
(right):

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/http/tests/ins...
LayoutTests/http/tests/inspector/network/network-content-replacement-xhr-expected.txt:9:
resource.content:
********************************************************************************************************************************************************************************************************
I'm not sure if this change is right or if this test is still valid. I think
this might be testing content handling for objects that aren't stored in
MemoryCache. If that's the case, this test is meaningless in a world where sync
XHRs go in the cache.

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/http/tests/ins...
File LayoutTests/http/tests/inspector/network/network-cyrillic-xhr.html (right):

https://codereview.chromium.org/23702040/diff/2001/LayoutTests/http/tests/ins...
LayoutTests/http/tests/inspector/network/network-cyrillic-xhr.html:29:
InspectorTest.makeSimpleXHR("GET", "resources/cyrillic.html?sync", false,
firstXHRLoaded);
Here and in several other places, add queries to ensure requests aren't served
from the cache when the test doesn't want it.

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
File Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/Resource.cpp:186: if (!m_loader) {
This if() was already dead code, ResourceLoader doesn't return 0 anymore.

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
File Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/Resource.cpp:184: m_status = Pending;
Set pending before rather than after, so it doesn't stomp whatever a sync load
set it to.

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:577: return
request.options().synchronousPolicy == RequestSynchronously ? resource : 0;
This is kind of lame, wherein we let both sync and async loads fail
synchronously, but only return a null Resource for async loads, because
otherwise sync loads have no way to get the information they require (e.g., the
ResourceError).

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
File Source/core/fetch/ResourceLoader.cpp (left):

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceLoader.cpp:66: loader->start();
This is moved to Resource.cpp so that Resource::m_loader is guaranteed non-null
when start() is called. Otherwise, if a sync request tries to revalidate a
resource and the revalidation fails, we might use-after-free when
Resource::canDelete() doesn't see a valid m_loader.

https://codereview.chromium.org/23702040/diff/2001/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (left):

https://codereview.chromium.org/23702040/diff/2001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:802:
request.setPriority(ResourceLoadPriorityVeryHigh);
This is now set for all sync requests in ResourceFetcher.cpp

[abarth-chromium, 2013-09-13 23:48:28]

I'm not sure about the inspector aspects.  We should probably have someone who
knows about the inspector take a closer look.

This looks great!  My only real concern is about whether you've introduced an
extra memcpy.  :)

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:220: options.synchronousPolicy =
RequestSynchronously;
Should the async pathway ASSERT that the options aren't set to synchronous?

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:222: return requestResource(Resource::Raw,
request);
Should this function return a ResourcePtr<RawResource> ?

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:577: return
request.options().synchronousPolicy == RequestSynchronously ? resource : 0;
On 2013/09/12 23:28:46, Nate Chapin wrote:
> This is kind of lame, wherein we let both sync and async loads fail
> synchronously, but only return a null Resource for async loads, because
> otherwise sync loads have no way to get the information they require (e.g.,
the
> ResourceError).

I might add this information as a comment.

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
File Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceLoader.cpp:47: #include "wtf/CurrentTime.h"
Is this still needed?

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceLoader.cpp:417: didReceiveData(0, dataOut.data(),
dataOut.size(), resourceLoadInfo ? resourceLoadInfo->encodedDataLength : -1);
Does this introduce a memcpy?  Previously, we were able to move the data
directly from the network stack to |data|.  Now we're passing a pointer and a
length into didReceiveData.  Is it copied into a SharedBuffer by that function? 
We can look at the revision history, but I remember that this memcpy was blowing
through a ton of memory on some pages.

https://codereview.chromium.org/23702040/diff/2001/Source/core/loader/Documen...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/23702040/diff/2001/Source/core/loader/Documen...
Source/core/loader/DocumentThreadableLoader.cpp:474: SharedBuffer* data =
resource->resourceBuffer();
See, the resource has a shared buffer inside of it.  We should be able to move
the data from the network stack into the resource's shared buffer without a
memcpy.

https://codereview.chromium.org/23702040/diff/2001/Source/core/loader/Documen...
Source/core/loader/DocumentThreadableLoader.cpp:475: if (data)
Can |data| be zero here?  Should we call didReceiveData with a zero size?  Maybe
we don't do that in the async case...

[Nate Chapin, 2013-09-16 20:32:50]

New version coming soon...

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
File Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:220: options.synchronousPolicy =
RequestSynchronously;
On 2013/09/13 23:48:28, abarth wrote:
> Should the async pathway ASSERT that the options aren't set to synchronous?

There are a lot of pathways :/

I'd say we should assert in requestResource() that sync requests are only done
on raw resources, since there are assumptions about that.

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:222: return requestResource(Resource::Raw,
request);
On 2013/09/13 23:48:28, abarth wrote:
> Should this function return a ResourcePtr<RawResource> ?

Probably not? Technically it's a raw resource under the hood, but that's only so
that the primary user of synchronous fetching (XHRs) can share data between sync
and async loads. There's nothing in how the synchronously loaded resource is
used that requires the special stuff in RawResource, which mostly just lets the
client hook the resource load callbacks for async loads.

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceFetcher.cpp:577: return
request.options().synchronousPolicy == RequestSynchronously ? resource : 0;
On 2013/09/13 23:48:28, abarth wrote:
> On 2013/09/12 23:28:46, Nate Chapin wrote:
> > This is kind of lame, wherein we let both sync and async loads fail
> > synchronously, but only return a null Resource for async loads, because
> > otherwise sync loads have no way to get the information they require (e.g.,
> the
> > ResourceError).
> 
> I might add this information as a comment.

Will do.

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
File Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/23702040/diff/2001/Source/core/fetch/Resource...
Source/core/fetch/ResourceLoader.cpp:417: didReceiveData(0, dataOut.data(),
dataOut.size(), resourceLoadInfo ? resourceLoadInfo->encodedDataLength : -1);
On 2013/09/13 23:48:28, abarth wrote:
> Does this introduce a memcpy?  Previously, we were able to move the data
> directly from the network stack to |data|.  Now we're passing a pointer and a
> length into didReceiveData.  Is it copied into a SharedBuffer by that
function? 
> We can look at the revision history, but I remember that this memcpy was
blowing
> through a ton of memory on some pages.

Yeah, I think it will memcpy. I guess we could expose a
setResourceBuffer(PassRefPtr<SharedBuffer>) on Resource for this.

https://codereview.chromium.org/23702040/diff/2001/Source/core/loader/Documen...
File Source/core/loader/DocumentThreadableLoader.cpp (right):

https://codereview.chromium.org/23702040/diff/2001/Source/core/loader/Documen...
Source/core/loader/DocumentThreadableLoader.cpp:475: if (data)
On 2013/09/13 23:48:28, abarth wrote:
> Can |data| be zero here?  Should we call didReceiveData with a zero size? 
Maybe
> we don't do that in the async case...

The layout tests say it can, it crashed on several cases without the null check.
I'm all but certain it's possible to go straight from didReceiveResponse to
didFinishLoading without a didReceiveData in between, based on the asserts in
ResourceLoader::didFinishLoading.

[Nate Chapin, 2013-09-16 21:19:04]

New version up. Unfortunately, the patch set delta broke for several of the
files, so it's not as easy to read as I was hoping.

[abarth-chromium, 2013-09-17 18:01:19]

LGTM

I'm not super confident in the inspector parts of this change.  Please use your
judgement as to whether you should get a review from inspector specialists.

[Nate Chapin, 2013-09-17 21:42:56]

pfeldman, would you mind taking a look at the inspector changes?

I believe that with this patch, synchronous XHRs will send identical callbacks
to asynchronous XHRs, so special handling for the sync case should no longer be
required.

Also, I'm not sure if
http/tests/inspector/network/network-content-replacement-xhr.html is still
meaningful, since synchronous XHR data will be stored in a memory cache
Resource.

[pfeldman, 2013-09-18 05:49:06]

inspector lgtm.

> Also, I'm not sure if
> http/tests/inspector/network/network-content-replacement-xhr.html is still
> meaningful, since synchronous XHR data will be stored in a memory cache
> Resource.

Is there a way to force-collect this Resource? While inspector is opened, we are
saving Resource data when it is being destroyed in our own cache that has size
limit. This test checks that limits are respected.

[Nate Chapin, 2013-09-19 21:19:04]

On 2013/09/18 05:49:06, pfeldman wrote:
> inspector lgtm.
> 
> > Also, I'm not sure if
> > http/tests/inspector/network/network-content-replacement-xhr.html is still
> > meaningful, since synchronous XHR data will be stored in a memory cache
> > Resource.
> 
> Is there a way to force-collect this Resource? While inspector is opened, we
are
> saving Resource data when it is being destroyed in our own cache that has size
> limit. This test checks that limits are respected.

I don't think there's currently a way to force it in a layout test. I suppose we
could add an Internals function that forces the Resource to be evicted, but it
would also be dependent on forcing GC.

[commit-bot: I haz the power, 2013-09-19 22:03:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/23702040/31001

[commit-bot: I haz the power, 2013-09-19 23:13:02]

Change committed as 158065