HTTP Link Header(Preload) assets in memory cache is ignored and downloaded twice
Reported by
vignesh....@gmail.com,
Dec 14 2017
|
||||||
Issue descriptionUserAgent: 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.
,
Dec 15 2017
,
Dec 18 2017
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...!!
,
Jan 9 2018
,
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...
,
Jul 6
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...
,
Jul 6
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
,
Jul 6
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.
,
Jul 9
If changing the order doesn't break anything that would be good I think.
,
Jul 9
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.
,
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
,
Jul 10
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by y...@yoav.ws
, Dec 14 2017