Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 652228 Requests with useStreamOnResponse flag don't reuse preloaded resources
Starred by 13 users Project Member Reported by y...@yoav.ws, Oct 3 Back to list
Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment
Version: M55
OS: Mac (but probably all)

What steps will reproduce the problem?
(1) http://paulirish.github.io/web-feature-availability/
(2) Look at downloaded resources

What is the expected output?

data.json should be downloaded once

What do you see instead?
It is downloaded twice.

Looking into why, it seems that this resource is loaded via `fetch()`, which turns on the useStreamOnResponse flag, which means that `ResourceFetcher::determineRevalidationPolicy()` says it needs to be reloaded.

Trying to naively remove that restriction results in assertion failures.

We need to figure out a way to preload `fetch()` based resources without triggering double downloads (otherwise, `fetch()` would be significantly slower than the equivalent XHR).

 
Summary: Requests with useStreamOnResponse flag don't reuse preloaded resources (was: Requests with useStreamOnResponseFlag don't reuse preloaded resources)
Components: Blink>Loader Blink>Network>FetchAPI
Components: -Blink>Loader
Loading triager here. Removing from loading as this seems like a fetch issue. 
Seems like XHR has the same issue http://output.jsbin.com/kusapoq/quiet
Cc: igrigo...@chromium.org
Comment 7 by y...@yoav.ws, Nov 15
XHR is a slightly different issue, and fwiw, I'm able to reuse XHRs when they don't have a `crossorigin` attribute and their withCredentials flag is set to true. Not sure that's the correct behavior, tbh.
Cc: ericbidelman@chromium.org
Components: Blink>Network>XHR
Adding XHR component since this is also an issue there.
Cc: surma@chromium.org
Cc: -yhirano@chromium.org
Owner: yhirano@chromium.org
Status: Assigned
Cc: yhirano@chromium.org
Owner: y...@yoav.ws
Talked w/yoav. He is making efforts to resolve this issue. Thanks!
Dug around the code a bit and I'd need some guidance from fetch()/streaming folks in order to proceed.

Current situation is that if we do match fetch() requests with previously preloaded resources, we get a response with a nullptr WebDataConsumerHandle in FetchManager::Loader::didReceiveResponse() (which ASSERTS)

If I ignore that assert, it seems like the handle is required to create BytesConsumerForDataConsumerHandle which is required for BodyStreamBuffer
which is in turn required for FetchResponseData::createWithBuffer().

The source of the nullptr handle is RawResource::didAddClient() which called client->responseReceived() with a nullptr handle. I don't think there's a way to create a handle there, as the current handle implementation in created in content/. I'm also not sure such a thing would make sense.

So, one possible way to fix this would be to create a BytesConsumer that doesn't require a WebDataConsumerHandle, and let it handle situations where the handle is nullptr.

Does that make sense? Would that actually work?


Cc: addyo@chromium.org
yhirano@ - friendly ping :) Thoughts on the proposed way to tackle this?
Another friendly ping. For context, the new code review system Chromium is switching to within a few months would benefit *highly* (on the order of seconds) from this and issue 678429 by preloading fetch requests to the API.
Cc: y...@yoav.ws
Owner: yhirano@chromium.org
We talked about this topic recently and I would like to sort things (MemoryCache, RawResource[Client], ThreadableLoader[Client], etc) out to fix this issue.
Awesome. Thanks for the update, Yoav!
Oops. I meant yhirano. 😑
Issue 702727 has been merged into this issue.
Cc: keanulee@chromium.org
Project Member Comment 24 by bugdroid1@chromium.org, Apr 20
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5705c2783128e015cf8c9da3f7a1b188d81101b8

commit 5705c2783128e015cf8c9da3f7a1b188d81101b8
Author: yhirano <yhirano@chromium.org>
Date: Thu Apr 20 12:52:09 2017

Evict "fallback" RawResource from MemoryCache

A service worker may ask the original renderer to re-fetch the request when a
fetch-mode=CORS request was not handled. In most cases ResourceLoader restarts
the resource, but in some cases it lets DocumentThreadableLoader handle the
response. In such a case, we can see a Resource whose response's
WasFallbackRequiredByServiceWorker is true.

Previously ResourceFetcher::DetermineRevalidationPolicy forbade to reuse such
a resource. This CL removes the logic. Instead, RawResource evicts itself when
receiving such a response.

BUG=652228
R=horo@chromium.org, tyoshino@chromium.org

Review-Url: https://codereview.chromium.org/2830753003
Cr-Commit-Position: refs/heads/master@{#465987}

[modify] https://crrev.com/5705c2783128e015cf8c9da3f7a1b188d81101b8/third_party/WebKit/Source/platform/loader/fetch/RawResource.cpp
[modify] https://crrev.com/5705c2783128e015cf8c9da3f7a1b188d81101b8/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp

Project Member Comment 25 by bugdroid1@chromium.org, Apr 25 (2 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b1688086e55683b586a5f00908266ea4e5a1296

commit 0b1688086e55683b586a5f00908266ea4e5a1296
Author: yhirano <yhirano@chromium.org>
Date: Tue Apr 25 05:19:38 2017

Give |allow_stale_resources_| a lower priority in DetermineRevalidationPolicy

This CL changes DetermineRevalidationPolicy a bit to simplify it. With this
CL, |allow_stale_resources_| will be checked after
ResourceLoaderOptions::CanReuseRequest is checked. CanReuseRequest checks
the synchronous policy and the cors policy, and neither of them is related
to staleness. Moreover, the synchronous policy was once checked BEFORE
allow_stale_resources_ (see [1]). Also, any user of
ResourceCacheValidationSuppressor, the class used to enable
|allow_stale_resource_|, doesn't seem to be interested in CORS. Hence I expect
no behavior change from this CL.

1: https://crrev.com/733a476495c43c1f472e5770c9d8aeeb94c5caca

BUG=652228
R=hiroshige@chromium.org, japhet@chromium.org

Review-Url: https://codereview.chromium.org/2828243002
Cr-Commit-Position: refs/heads/master@{#466880}

[modify] https://crrev.com/0b1688086e55683b586a5f00908266ea4e5a1296/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp

Project Member Comment 26 by bugdroid1@chromium.org, Apr 25 (2 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58197789bfafee408d8fc728c0ed2f1da81bbbed

commit 58197789bfafee408d8fc728c0ed2f1da81bbbed
Author: yhirano <yhirano@chromium.org>
Date: Tue Apr 25 09:03:39 2017

Move |is_static_data| check down in DetermineRevalidationPolicy

This CL moves

 if (is_static_data)
   return kUse;

section down in DetermineRevalidationPolicy as a preparation to factor out some
|return kReload;| statements.

BUG=652228, 714574
R=hiroshige@chromium.org

Review-Url: https://codereview.chromium.org/2829043002
Cr-Commit-Position: refs/heads/master@{#466931}

[modify] https://crrev.com/58197789bfafee408d8fc728c0ed2f1da81bbbed/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp

Sign in to add a comment