service worker pass-through breaks code caching |
|||||||||
Issue descriptionCurrently if a service worker implements a pass-through FetchEvent handler like: evt.respondWith(fetch(evt.request)); Any code caching that the main thread v8 compiler would perform will not work. This is for at least these reasons, but possible more: 1) Resource::CreateCachedMetadataSender() sets a NullCachedMetadataSender if service worker provided resources that do not come from cache_storage. [1] This prevents v8 from storing anything. 2) FetchManager::Loader does not implement ThreadableLoaderClient::DidReceiveCachedMetadata. [2] This means the fetch() Response in the service worker will ignore any code cache available from http cache. 3) After (2) is fixed, service worker code would need to look for cached metadata on the Response and pass it on to the outer loader. I see this impacting google docs spreadsheet loads when it gets a cache_storage miss and has to fall back to the network. I'm going to see if I can at least get a proof-of-concept working. [1]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource.cc?l=556&rcl=15b703187fdc9202f09668b0107bb7dfc79673f2 [2]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/loader/threadable_loader_client.h?l=63&rcl=15b703187fdc9202f09668b0107bb7dfc79673f2
,
Dec 21
It seems getting this to work may also need some integration work with the IsolatedCodeCache feature. I think I can get it working with isolated code cache disabled, but I haven't been able to get the code cache written and read back in isolated mode yet. For example, this section doesn't seem quite accurate with regard to service workers: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc?l=276-283&rcl=4dcb10c9b199341e5c687d98eb9d91795d212d33 It seems to assume responses returned from service worker can only come from cache_storage. A service worker can also return responses produced from http cache via a fetch() call. Of course, fixing this check wasn't enough so I must be missing something. +mythria for any insight. Thanks!
,
Dec 22
This is extremely hacky, but it does allow code caching to work on my google docs test case: https://chromium-review.googlesource.com/c/chromium/src/+/1389323 Again, this doesn't work with IsolatedCodeCache yet and you have to disable it on the command line. I'll work to fix that missing feature and otherwise clean the CL up after the holiday in January. (Unless someone wants to run with this while I'm out... please feel free!)
,
Dec 31
Re: comment#2 The logic behind that code is if the request was serviced from the cache storage and not using the fallback path, then it is Ok to remove any entry from the code cache since the service worker already has a copy of it and we would never use the data from code cache as long as the cache storage provides the data. I am happy to remove that part where we clear the cache. Though In my understanding I think we should fix the following check: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc?g=0&l=825 response.WasFetchedViaServiceWorker() is set to false when the service worker didn't call FetchEvent.respondWith() and the request falls back on to the network. Maybe we should somehow also indicate the cases where service worker implemented a fallback itself? adding horo@ who knows a lot more about service workers.
,
Jan 2
I was able to get this working with IsolatedCodeCache enabled by commenting out the following conditions: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc?l=332-340&rcl=badd8a0fab485b41aee70b4ed463868b7e218359 and https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc?l=351-359&rcl=badd8a0fab485b41aee70b4ed463868b7e218359 These conditions were disabling the loading of code cache for the raw resources created by `fetch()` within the service worker context. Without loading the code cache in these cases we cannot pass it on to the outer context for `fetchEvent.respondWith(fetch())` type fetch handlers. It appears this is a known possible concern per bug 895850. I'll make a comment over there. Also it seems we can safely remove the main thread check since bug 867347 was fixed. If there is a concern about triggering wasted code cache requests for main thread XHR requests we could make a new condition here that filters out raw requests when on main thread and wasm modules are disabled. Mythri, what do you think?
,
Jan 3
On further thought, maybe we don't need any of the SW plumbing in the IsolatedCodeCache case since it fetches it out-of-band anyway. Also, I need to think about if the SW can do evil things in this architecture by passing through to a different url fetch(). What does the shipping schedule for IsolatedCodeCache look like? Maybe it's not worth working on the case where it's disabled...
,
Jan 3
The main thread check is going away in this cl: https://chromium-review.googlesource.com/c/chromium/src/+/1260209. It was just blocked on adding tests. We had some issues on adding tests for non-main thread cases. bbudge@ now added tests and landing it soon. Removing the check for Service worker controlled requests is fine with me. When we weren't sure which would be better. If it helps docs, then I would be happy to remove it. Though I think horo@ and kinuko@ are the right people to comment on that. Do we really need to remove the Raw resource check [raw_resource_check]. I am fine with it, since Wasm caches would be enabled by default soonish. So, it will go away anyway. I was just curious why it is needed here. I thought we don't generate any code caches for things other than Script and MainResource script types. Site Isolated code caches is already enabled by default in 72. We plan to remove the flag and all the checks once 72 hits stable and we don't see any problems. [raw_resource_check]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc?rcl=badd8a0fab485b41aee70b4ed463868b7e218359&l=332
,
Jan 3
horo@, kinuko@ any thoughts on removing: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc?l=351-359&rcl=badd8a0fab485b41aee70b4ed463868b7e218359 For me it seems reasonable, if it helps docs.
,
Jan 3
> Do we really need to remove the Raw resource check [raw_resource_check].
I think we can probably get away with leaving that. I removed it in an attempt to get the code cache populated in the fetch()'s Response on the service worker. That was before I realized that IsolatedCodeCache uses a completely separate mojo read path. So it may not be necessary to plumb anything through the service worker layer.
My main concern is with cases like:
<!-- main thread window -->
<script src="a.js">
// service worker thread
addEventListener("fetch", evt => {
if (evt.request.url.contains("a.js")) {
evt.respondWith(fetch("b.js"));
}
});
In this case, do we try reading/writing code cache for a.js or b.js? I think for safety it really needs to be b.js.
Anyway, I'll investigate this stuff more today. Thanks!
,
Jan 3
,
Jan 3
Here is a new CL with a much simpler approach: https://chromium-review.googlesource.com/c/chromium/src/+/1394740
,
Jan 4
So my simpler CL here: https://chromium-review.googlesource.com/c/chromium/src/+/1394740 Works and I'm now trying to add a test. I'm trying to adapt this test: https://cs.chromium.org/chromium/src/third_party/blink/web_tests/http/tests/devtools/service-workers/service-worker-v8-cache.js I'm having difficulty testing cases where the code cache should *not* be used. It seems v8 is still happily claiming to consume code cache. My theory is this is from memory cache. But maybe I'm not understanding the output. Mythri, can you help me understand the outptu from the isolated code cache here: https://cs.chromium.org/chromium/src/third_party/blink/web_tests/http/tests/devtools/isolated-code-cache/same-origin-test.js AFAICT the "differentURLLoadScript" case also generates a consumeCodeCache entry. Should it? Or are you looking for something else in the expected output to verify that this has been excluded?
,
Jan 5
> AFAICT the "differentURLLoadScript" case also generates a consumeCodeCache entry. Should it? Or are you looking for something else in the expected output to verify that this has been excluded? I think what I wrote here is wrong and unrelated to my problem. That test is trying to show that a single script read from another URL iframe does not use the same code cache populated on the first iframe. In my case, I'm trying to show that we don't actually store the code cache under certain conditions. I was hoping to do this by doing repeated script loads and showing that we never consumer the cache in the timeline output. Unfortunately, though, the test happily pulls the code cache out the memory cache. So I still get the consumer markers in the timeline. Does anyone have any suggestions on how to test these negative cases? I've tried calling `setCacheDisabled(true)` to disable the memory cache, but it doesn't seem to do anything. I've also tried using separate iframes, etc. I don't think I can create a fully separate process since this is a content shell. With instrumentation I can definitely see that the service worker the resource is only loading once from the service worker, so I'm pretty confident this is a memory cache issue confusing the test and not a different bug. Any advice on how to test this would be appreciated. Thanks!
,
Jan 7
I am not sure I understand the problem correctly. If we are not supposed to produce/store the code cache we should not consume it either. I don't know much about the loading cycle but in my understanding if the parameters change wouldn't we reload it and not use the cached entry from memory cache? From the expectations file I see compile events only for the passthrough case and none for any other cases. In my understanding we should see compile events for others as well. hiroshige@/horo@ might know more about this?
,
Jan 7
One possibility I can think of is to split this test into multiple tests. So we start from a clean slate (hence nothing in the memory cache) for each case. From my prior experience, if we use "printTimelineRecordsWithDetails" multiple times, the tests tend to timeout. The start and stop timeline are a bit expensive, so it is good to split the test anyway in my opinion. Just to add, I am not an owner on any of these areas :).
,
Jan 7
Also for the case, where service worker responds with a different URL would there be any other security implications? Is it only allowed to respond with a different URL as long as it is in the same origin? If it is not the same origin, shouldn't we handle it as a redirect request?
,
Jan 7
> I am not sure I understand the problem correctly. If we are not supposed to produce/store the code cache we should not consume it either. I don't know much about the loading cycle but in my understanding if the parameters change wouldn't we reload it and not use the cached entry from memory cache? Even if we null route the code cache to the browser process here: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource.cc?l=558&rcl=6a5bea657b117689d1465eb854c3f1ccd4fb48da The v8 compiler side has no way of really knowing that AFAICT. So it will still report that it produced that code cache even though it was not stored to disk. Also, the code cache is still stored in the Resource that is kept alive for some time in the memory cache in the renderer. So the subsequent loads in my test end up consuming that local code cache from the memory cache Resource. > From the expectations file I see compile events only for the passthrough case and none for any other cases. In my understanding we should see compile events for others as well. Sorry, I never finished updating the expectations because I was not getting results that verified what I was trying to test. > Also for the case, where service worker responds with a different URL would there be any other security implications? Is it only allowed to respond with a different URL as long as it is in the same origin? If it is not the same origin, shouldn't we handle it as a redirect request? Its complicated, unfortunately. Originally service workers ignored the Response url from the service worker completely. The spec was changed to use the Response url and pass it through to the outer context in most cases. Chrome does not yet fully implement that across all consumers, though, so its possible for the outer context to still think its using the original URL. Therefore I wanted to add some extra safety checks for this case to make sure we don't try to store things against the wrong URL. Of course, the response timestamp matching requirement is another check which should also limit this risk AIUI. Thanks your help! I will look into splitting the tests or maybe I will fall back to a simpler unit test instead.
,
Jan 7
The code I was modifying was just refactored over in bug 855189. Also, I'm going to give up on the web_test approach for now.
,
Jan 8
> The v8 compiler side has no way of really knowing that AFAICT. So it will still report that it produced that code cache even though it was not stored to disk. Yes, I think you are right. I didn't realize NullMetadataSender just doesn't send the data to the disk. May be we should change it so, it doesn't store anything at all. Something like NullCachedMetadataHandler, but that's a different issue.
,
Jan 8
Also Thanks for the explanation
,
Jan 10
Okay, finally catching up with the discussion. And yes, as far as the data shows positive result I think we can be fine with removing the check mentioned in #c8. Will comment on the actual CL.
,
Jan 10
Sorry, I just noticed this issue. If we remove this check (https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc?l=351-359&rcl=badd8a0fab485b41aee70b4ed463868b7e218359), I think ScriptCachedMetadataHandler::SetSerializedCachedMetadata() could be called twice (from IsolatedCodeCache and from CacheStorage). So DCHECK() in ScriptCachedMetadataHandler::SetSerializedCachedMetadata() fails. https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/loader/fetch/script_cached_metadata_handler.cc?l=52-55&rcl=badd8a0fab485b41aee70b4ed463868b7e218359 I'm not 100% sure but I think it is OK to remove the DCHECK() in ScriptCachedMetadataHandler::SetSerializedCachedMetadata(). The cached data is used in ScriptCompiler::CompileUnboundInternal() only once.
,
Jan 10
#c22 - we shouldn't set cached metadata for IsolatedCodeCache when the response comes from CacheStorage, this can be checked when we get responses (and Ben's patch seems to take care of it iiuc).
,
Jan 10
Note, I discovered today that IsolatedCodeCache also disables code cache for fallback service workers where no respondWith() is called. The legacy code cache support in 71 does support the fallback case, but this will regress when IsolatedCodecache ships in 72. Its a bit difficult to reason about if this will be significant globally across all sites, but its definitely possible this will cause regressions on some sites use service workers. Our performance advice to sites has been to use fallback handlers when they cannot load a script from cache_storage. The fallback case is fixed by the CL in progress for this bug, but that is currently only targeted for 73. What do people think about the regressed performance for the fallback case that is currently scheduled to ship in 72?
,
Jan 11
#c24: yeah that's a known issue when we made SW-related changes in IsolatedCodeCache. I haven't really thought that this'd be huge, because with regular code cache the same resource needs to be loaded at least twice in a few days to trigger code caching, and I didn't see good reasons critical script resources that are accessed often are not being cached in Cache Storage. But if this impacts important sites (like docs) cases we may need to worry. (Ben, do you have a sense that how much this could impact?)
,
Jan 11
I don't have a good sense at the moment. I'm asking around. I was recommending to docs they try to use a fallback instead of the passthrough they currently have to get code caching, but that has not been implemented yet afaik. I'll investigate some other large sites tomorrow.
,
Jan 11
#c26-- I see, (feeling a bit sorry that the change may temporarily contradict to your recommendation) /cc bashi@ and falken@ for perf discussion around code caching. (Also if you have any comments about the topic discussed in #c24 and #c25, i.e. M72 may have regression for fallback javascript requests)
,
Jan 11
(Actually adding)
,
Jan 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cc10ceadb80cdd060a804d8989dfbba372899008 commit cc10ceadb80cdd060a804d8989dfbba372899008 Author: Ben Kelly <wanderview@chromium.org> Date: Fri Jan 11 14:12:00 2019 Support IsolatedCodeCache for service worker pass-through responses. Previously code caching of scripts served from service workers was only supported if the Response was produced from cache_storage. If the service worker implemented a pass-through handler like: evt.respondWith(fetch(evt.request)) Then the v8 compiler could not store or load code cache for the script. With IsolatedCodeCache shipping it becomes much simpler to now support this feature. This CL enables support by: 1) Fetching the code cache for service worker controlled scripts. 2) Allowing the code cache to be used for pass-through responses. 3) Storing any code cache produced for pass-through responses. We still explicitly disable code cache for service worker handlers that produce either: a) Synthetic `new Response()` objects. b) Response objects produced by `fetch()` to a URL different from the original request URL. Bug: 917414 Change-Id: I4efdc852a27069d2937056af0133a986e745b2be Reviewed-on: https://chromium-review.googlesource.com/c/1394740 Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Commit-Queue: Ben Kelly <wanderview@chromium.org> Cr-Commit-Position: refs/heads/master@{#621993} [modify] https://crrev.com/cc10ceadb80cdd060a804d8989dfbba372899008/third_party/blink/renderer/platform/loader/fetch/cached_metadata_handler.cc [modify] https://crrev.com/cc10ceadb80cdd060a804d8989dfbba372899008/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc [modify] https://crrev.com/cc10ceadb80cdd060a804d8989dfbba372899008/third_party/blink/renderer/platform/loader/fetch/resource_loader.h [modify] https://crrev.com/cc10ceadb80cdd060a804d8989dfbba372899008/third_party/blink/renderer/platform/loader/fetch/resource_loader_test.cc [modify] https://crrev.com/cc10ceadb80cdd060a804d8989dfbba372899008/third_party/blink/renderer/platform/loader/fetch/resource_response.cc [modify] https://crrev.com/cc10ceadb80cdd060a804d8989dfbba372899008/third_party/blink/renderer/platform/loader/fetch/resource_response.h [modify] https://crrev.com/cc10ceadb80cdd060a804d8989dfbba372899008/third_party/blink/renderer/platform/loader/fetch/resource_test.cc
,
Jan 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fcdae8ac98ca623b2f0d6b13376defa2888a1b17 commit fcdae8ac98ca623b2f0d6b13376defa2888a1b17 Author: Roman Sorokin [CET] <rsorokin@chromium.org> Date: Fri Jan 11 14:25:37 2019 Revert "Support IsolatedCodeCache for service worker pass-through responses." This reverts commit cc10ceadb80cdd060a804d8989dfbba372899008. Reason for revert: Broke the build (see https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Fuchsia%20ARM64/50818 ) Original change's description: > Support IsolatedCodeCache for service worker pass-through responses. > > Previously code caching of scripts served from service workers was only > supported if the Response was produced from cache_storage. If the > service worker implemented a pass-through handler like: > > evt.respondWith(fetch(evt.request)) > > Then the v8 compiler could not store or load code cache for the script. > > With IsolatedCodeCache shipping it becomes much simpler to now > support this feature. This CL enables support by: > > 1) Fetching the code cache for service worker controlled scripts. > 2) Allowing the code cache to be used for pass-through responses. > 3) Storing any code cache produced for pass-through responses. > > We still explicitly disable code cache for service worker handlers that > produce either: > > a) Synthetic `new Response()` objects. > b) Response objects produced by `fetch()` to a URL different from the > original request URL. > > Bug: 917414 > Change-Id: I4efdc852a27069d2937056af0133a986e745b2be > Reviewed-on: https://chromium-review.googlesource.com/c/1394740 > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Commit-Queue: Ben Kelly <wanderview@chromium.org> > Cr-Commit-Position: refs/heads/master@{#621993} TBR=kinuko@chromium.org,mythria@chromium.org,wanderview@chromium.org Change-Id: I966c165c6978121a6ca68823d10ec697d0fa94f6 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 917414 Reviewed-on: https://chromium-review.googlesource.com/c/1406995 Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org> Commit-Queue: Roman Sorokin [CET] <rsorokin@chromium.org> Cr-Commit-Position: refs/heads/master@{#621994} [modify] https://crrev.com/fcdae8ac98ca623b2f0d6b13376defa2888a1b17/third_party/blink/renderer/platform/loader/fetch/cached_metadata_handler.cc [modify] https://crrev.com/fcdae8ac98ca623b2f0d6b13376defa2888a1b17/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc [modify] https://crrev.com/fcdae8ac98ca623b2f0d6b13376defa2888a1b17/third_party/blink/renderer/platform/loader/fetch/resource_loader.h [modify] https://crrev.com/fcdae8ac98ca623b2f0d6b13376defa2888a1b17/third_party/blink/renderer/platform/loader/fetch/resource_loader_test.cc [modify] https://crrev.com/fcdae8ac98ca623b2f0d6b13376defa2888a1b17/third_party/blink/renderer/platform/loader/fetch/resource_response.cc [modify] https://crrev.com/fcdae8ac98ca623b2f0d6b13376defa2888a1b17/third_party/blink/renderer/platform/loader/fetch/resource_response.h [modify] https://crrev.com/fcdae8ac98ca623b2f0d6b13376defa2888a1b17/third_party/blink/renderer/platform/loader/fetch/resource_test.cc
,
Jan 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cced1867ffd64eb77f7b3a144bdc13ea156035ab commit cced1867ffd64eb77f7b3a144bdc13ea156035ab Author: Ben Kelly <wanderview@chromium.org> Date: Fri Jan 11 17:31:32 2019 Reland "Support IsolatedCodeCache for service worker pass-through responses." This is a reland of cc10ceadb80cdd060a804d8989dfbba372899008 The previous patch failed to build after the ResourceFetcher constructor arguments changed. This reland includes a mechanical fix to use the new constructor form. Original change's description: > Support IsolatedCodeCache for service worker pass-through responses. > > Previously code caching of scripts served from service workers was only > supported if the Response was produced from cache_storage. If the > service worker implemented a pass-through handler like: > > evt.respondWith(fetch(evt.request)) > > Then the v8 compiler could not store or load code cache for the script. > > With IsolatedCodeCache shipping it becomes much simpler to now > support this feature. This CL enables support by: > > 1) Fetching the code cache for service worker controlled scripts. > 2) Allowing the code cache to be used for pass-through responses. > 3) Storing any code cache produced for pass-through responses. > > We still explicitly disable code cache for service worker handlers that > produce either: > > a) Synthetic `new Response()` objects. > b) Response objects produced by `fetch()` to a URL different from the > original request URL. > > Bug: 917414 > Change-Id: I4efdc852a27069d2937056af0133a986e745b2be > Reviewed-on: https://chromium-review.googlesource.com/c/1394740 > Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> > Commit-Queue: Ben Kelly <wanderview@chromium.org> > Cr-Commit-Position: refs/heads/master@{#621993} TBR=kinuko@chromium.org Bug: 917414 Change-Id: I547f0b410c59a14a1f9fff8f328b008ee7b1bb79 Reviewed-on: https://chromium-review.googlesource.com/c/1407029 Reviewed-by: Ben Kelly <wanderview@chromium.org> Commit-Queue: Ben Kelly <wanderview@chromium.org> Cr-Commit-Position: refs/heads/master@{#622049} [modify] https://crrev.com/cced1867ffd64eb77f7b3a144bdc13ea156035ab/third_party/blink/renderer/platform/loader/fetch/cached_metadata_handler.cc [modify] https://crrev.com/cced1867ffd64eb77f7b3a144bdc13ea156035ab/third_party/blink/renderer/platform/loader/fetch/resource_loader.cc [modify] https://crrev.com/cced1867ffd64eb77f7b3a144bdc13ea156035ab/third_party/blink/renderer/platform/loader/fetch/resource_loader.h [modify] https://crrev.com/cced1867ffd64eb77f7b3a144bdc13ea156035ab/third_party/blink/renderer/platform/loader/fetch/resource_loader_test.cc [modify] https://crrev.com/cced1867ffd64eb77f7b3a144bdc13ea156035ab/third_party/blink/renderer/platform/loader/fetch/resource_response.cc [modify] https://crrev.com/cced1867ffd64eb77f7b3a144bdc13ea156035ab/third_party/blink/renderer/platform/loader/fetch/resource_response.h [modify] https://crrev.com/cced1867ffd64eb77f7b3a144bdc13ea156035ab/third_party/blink/renderer/platform/loader/fetch/resource_test.cc
,
Jan 15
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by aaronsn@google.com
, Dec 21