Blink>Loader for more eyes. Do people agree this is a bug?

"Cache-Aware Loading" seems to have been added in https://codereview.chromium.org/2454983002 with Design Doc https://docs.google.com/document/d/1SKRVFza_USrdVZgJmPnKWmqbzj-kVoxMB2ECH_xLhS4/edit#heading=h.c7ekb7b97r5

It does seem to make sense that it's a bug. The Cache-Aware Loading load is supposed to go directly to disk cache, and if it's not there we restart with a normal load. So at that time service worker can intercept the request.

Not sure if we should skip it. If the site has a SW that wants to return something different for the URL it will break when we have the resource for the original request URL in http cache?

...but toyoshim@ (or kouhei@ or hiroshige@...) should give the real answer, I could be wrong :)

Hm, yea seems like a spec violation to do a load without going through SW. Maybe when the page has a service worker controller, it should not to do the Cache-Aware Load and just do a normal load that goes through SW.

#5 - yeah that sounds more reasonable to me.

In the cache-aware loading, the first load to the disk cache is a kind of an internal optimization, and should not be observed by external JavaScripts of registered SW.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp
We activate the cache-aware loading in ResourceLoader::ActivateCacheAwareLoadingIfNeeded().
So, if we can check if the target URL is captured by SW or not at this point, the fix would be easy. Otherwise, we need modifying something in //net?

falken, kinuko: How does the disk cache work if SW is used for the URL? If the SW is behind the net, and returned value from SW should not be cached, existing code just work as we expect, but if SW captures the request before //net handles it, SW needs to return a cache miss error for the first cache-only load of the cache-aware loading.

> falken, kinuko: How does the disk cache work if SW is used for the URL?

Service worker captures the request before //net. So I think we should just disable cache-aware loading when there is a service worker controller.

Currently there are two internal WebCachePolicy values(not defined in Fetch API spec): this one kReturnCacheDataIfValid, and another one kFrameLoadTypeReloadBypassingCache which is configured to skip SW at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/FrameLoader.cpp?rcl=0bee6f0baaa916721773d58c068c03012dfce5cb&l=258, so could we also do the same thing to kReturnCacheDataIfValid to unify the handling for internal WebCachePolicy?

kFrameLoadTypeReloadBypassingCache is so-called the hard-reload. Could be merged to "reload"?

I'm creating a patch that stop using kReturnCacheDataIfValid if service worker controlls the page.

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

commit 839ea8c6dfe0af07b5b79920d48d3d1fda68c7a0
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Wed Oct 18 09:59:14 2017

Stop activating cache-aware loading on pages controlled by service worker

Cache-aware loading is an internal optimization and implementation
details that should not be exposed to the fetch event. That makes
a hidden request in order to check cache entry existence for legacy
sites, but it is useless for modern sites that enables service worker
to optimize such resource loading by hands.

This patch just stops using the cache-aware loading on service worker
enabled pages. It solves the problem that expose the hidden request
to the fetch event, and does not cause real world performance impact.

Bug:  750612 
Change-Id: I8c698ae259274482d35cdbb5abde84c946652cfd
Reviewed-on: https://chromium-review.googlesource.com/725026
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509727}
[modify] https://crrev.com/839ea8c6dfe0af07b5b79920d48d3d1fda68c7a0/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp

Let me close.
Leon, if you have any other concern, please reopen or file another one with CC-ing us.

I see, Thank you :)