New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 794958 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 10
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

HTTP Link Header(Preload) assets in memory cache is ignored and downloaded twice

Reported by vignesh....@gmail.com, Dec 14 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce the problem:
1. Go to https://node-tailor-pwvyocwojb.now.sh/index
2. Open DevTools -> Network Tab
3. Click Disable Cache & Hard Reload multiple times

Check the http link header being ignored. 

What is the expected behavior?
Should Preload the JS asset and should ignore the script tag later. 

What went wrong?
- The JS assets are loaded twice
- Even after doing hard reload, the JS asset was still in memory cache and totally being ignored and requested.  

(I am not sure if it dev tools reporting is wrong here) 

Did this work before? N/A 

Does this work in other browsers? N/A

Chrome version: 62.0.3202.94  Channel: stable
OS Version: OS X 10.12.6
Flash Version: 

Works properly when injected the preload using <link> element.
 
Screen Shot 2017-12-14 at 14.38.49.png
163 KB View Download

Comment 1 by y...@yoav.ws, Dec 14 2017

Owner: y...@yoav.ws
Labels: Needs-Bisect Needs-Milestone
Labels: -Needs-Bisect M-65 Triaged-ET OS-Linux OS-Windows
Status: Untriaged (was: Unconfirmed)
Able to reproduce this issue on Mac 10.12.6, Win-10 and Ubuntu 14.04 using chrome reported version #62.0.3202.94 and latest canary #65.0.3297.0.
This is a non-regression issue as it is observed from M50 old builds. 

Hence, marking it as untriaged to get more inputs from dev team.

Thanks...!!
Status: Assigned (was: Untriaged)

Comment 5 by y...@yoav.ws, Jan 10 2018

Is the original host (https://node-tailor-pwvyocwojb.now.sh/index) still up for testing? I'm trying it now, but it's not responding...
Cc: yhirano@chromium.org
OK, so I'm able to reproduce this, and when the page is hard-reloaded (CMD+r), I see double downloads. I've analyzed this and it seems like the following is happening:
* ResourceFetcher::preloads_ gets cleared between navigations
* The Memory cache is not.
* During RequestResource of the preload requests, they are not being added back into preloads_, as the policy is kUse so InsertAsPreloadIfNecessary is not called.
* When devtools are open with "disable cache", in one of the requests, MemoryCache::ResourceForURL fails to find the resource map (with an empty cache identifier).

It seems like when "disable cache" is on, MemoryCache::EvictResources is sometimes being called from InspectorNetworkAgent::DidCommitLoad *after* the first preload link header was processed, and removes that resource from the memory cache.

So, a couple of open questions:
* Should we insert preloads to preloads_ if they are not there, even if the policy is kUse?
* Can we make sure that InspectorNetworkAgent::DidCommitLoad is called before the link headers are processed?
* Is this testable? currently it requires a bunch of manual manipulations to reproduce...

OK, so finally I remembered that I fixed a very similar issue before: https://codereview.chromium.org/2385313002/

However, that solution relies on the resource indicating that it's an unused preload. Because the resource was used in the previous page load, it's not indicated as such and therefore gets evicted

yhirano@ - thoughts about how the preloads_ container should behave in this case?

One way to fix this is to flip on the IsUnusedPreload flag on a resource that's being re-used for preload.
Another is to make sure that those header preloads make it into preloads_ and stay there, even if evicted from MemoryCache.
Yet another would be to change the order in https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/loader/document_loader.cc?sq=package:chromium&dr=CSs&g=0&l=991 and make sure the cache eviction happens before the preloads. I'm not sure this won't have some side effects.
If changing the order doesn't break anything that would be good I think.
I agree that we should change the order if we can (and this is what https://chromium-review.googlesource.com/c/chromium/src/+/1127942 does).

I wonder though if the preloads_ container is behaving as it should in the reload case, as even if we reverse the order, the preloads will be fetched from the memcache, not the preloads_ container.
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3017ad45e90c729b4a8a295e449cd46fb52863da

commit 3017ad45e90c729b4a8a295e449cd46fb52863da
Author: Yoav Weiss <yoav@yoav.ws>
Date: Tue Jul 10 04:57:41 2018

[preload] Fix double downloads with devtools, disabled cache & reloads

The network inspector calling MemoryCache::EvictResources() when the
document was committed, *after* the link header preloads were
dispatched.

Even though a mechanism to avoid the eviction of unused preloads in
this case was introduced in https://codereview.chromium.org/2385313002,
it wasn't useful when the page was reloaded *twice*, as the preloaded
resource was used in the first load, and wasn't considered an unused
preload anymore.

This CL handles that issue by changing the order of the calls, making
sure that EvictResources is called before the header preloads are
processed. It also removes the older, now-unnecessary mechanism.

Bug:  794958 
Change-Id: Ia095862edb9c0e7b2fadf2b159864b675407c52a
Reviewed-on: https://chromium-review.googlesource.com/1127942
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Nate Chapin <japhet@chromium.org>
Commit-Queue: Yoav Weiss <yoav@yoav.ws>
Cr-Commit-Position: refs/heads/master@{#573616}
[add] https://crrev.com/3017ad45e90c729b4a8a295e449cd46fb52863da/third_party/WebKit/LayoutTests/http/tests/devtools/network/network-disable-cache-preloads-twice-expected.txt
[add] https://crrev.com/3017ad45e90c729b4a8a295e449cd46fb52863da/third_party/WebKit/LayoutTests/http/tests/devtools/network/network-disable-cache-preloads-twice.js
[modify] https://crrev.com/3017ad45e90c729b4a8a295e449cd46fb52863da/third_party/blink/renderer/core/inspector/inspector_network_agent.cc
[modify] https://crrev.com/3017ad45e90c729b4a8a295e449cd46fb52863da/third_party/blink/renderer/core/loader/document_loader.cc
[modify] https://crrev.com/3017ad45e90c729b4a8a295e449cd46fb52863da/third_party/blink/renderer/platform/loader/fetch/memory_cache.cc
[modify] https://crrev.com/3017ad45e90c729b4a8a295e449cd46fb52863da/third_party/blink/renderer/platform/loader/fetch/memory_cache.h

Status: Fixed (was: Assigned)

Sign in to add a comment