Handling duplicate importScripts() #1041
Comments
I don't have a strong opinion here. But I think it'd be reasonable to align the behavior to the current For service workers case, we're doing the same thing until the service worker's imported scripts updated flag is set although the default cache behavior has been changed to
Likewise, a recursion to the same script in other workers has the same behavior: Chrome encounters JS stack overflow while Firefox doesn't. It seems we should clarify the expected behavior in this case. Would we want to detect any recursion cycles along the call paths with |
|
I think in FF we persist both |
|
Actually, I take that back. Since the first |
This makes sense to me, especially if Firefox is doing the same. |
Thanks for the feedback. What I like about this over "last one wins" behavior is that the resulting script before installation is the same as the resulting script after installation. I think it'd also be a tad easier for our implementation to not evict/overwrite the original cached script. Also, I think for the recursive importScripts('main-script.js') case, I'll just read the cached script the same as any importScripts for an already-cached script. It'll still cause a stack overflow in Chrome. |
ServiceWorkerContextRequestHandler should emit a network error instead of falling back to network when it cannot handle a request. Otherwise, a service worker can be spawned that did not load via our custom ServiceWorkerWriteToCacheJob/ServiceWorkerReadFromCacheJob jobs, resulting in a running worker whose ServiceWorkerVersion has not been properly initialized. I suspect this can cause the bug 485900. This patch: - Changes network fallback for failure cases to an ERR_FAILED network error. - As an exception, an installed worker loading an unstored script still results in network fallback. This should be deprecated and removed eventually, see w3c/ServiceWorker#1021. - Changes the behavior for a new worker loading an already stored script (i.e., calling importScripts() for the same script multiple times). Before this patch, we would fallback to network for this script. Now, we read the stored script. This is not yet codified in the spec but is expected to have almost no real-world impact and has support on w3c/ServiceWorker#1041 BUG=485900,678899 Review-Url: https://codereview.chromium.org/2602853002 Cr-Commit-Position: refs/heads/master@{#442590}
|
I plan to upstream an WPT test for this although the spec has not yet changed, since Chrome and Firefox pass it: https://bugs.chromium.org/p/chromium/issues/detail?id=678899 |
|
Sounds good. |
ServiceWorkerContextRequestHandler should emit a network error instead of falling back to network when it cannot handle a request. Otherwise, a service worker can be spawned that did not load via our custom ServiceWorkerWriteToCacheJob/ServiceWorkerReadFromCacheJob jobs, resulting in a running worker whose ServiceWorkerVersion has not been properly initialized. I suspect this can cause the bug 485900. This patch: - Changes network fallback for failure cases to an ERR_FAILED network error. - As an exception, an installed worker loading an unstored script still results in network fallback. This should be deprecated and removed eventually, see w3c/ServiceWorker#1021. - Changes the behavior for a new worker loading an already stored script (i.e., calling importScripts() for the same script multiple times). Before this patch, we would fallback to network for this script. Now, we read the stored script. This is not yet codified in the spec but is expected to have almost no real-world impact and has support on w3c/ServiceWorker#1041 BUG=485900,678899 Review-Url: https://codereview.chromium.org/2602853002 Cr-Original-Commit-Position: refs/heads/master@{#442590} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 815dc48c9d00b0142a2421de8f10b7cb9c5b34ab
|
While working on this issue, I found out a point that the spec and the implementation (Chromium) don't agree. The spec says each service worker has its own script resource and the related imported script resource map tacked on it. In Chromium, the service worker versions seem to share the same script resource cache (i.e. the disk cache implementation.) If my understanding is correct, the spec and the implementation have different behavior for update: Let's say scope1 client and scope2 client have different scope but share the same script. They registered the SW. When scope1 client detected and updated the SW and scope2 client tries to update the SW later, the spec will make it proceed to Install as it figure it out as a different resource. On the other hand, the implementation won't trigger Install and return early treating them byte identical resources. I'd like to hear from implementers and sort out the desired behavior we originally expected. /cc @mattto @slightlyoff @wanderview @aliams @cdumez @jakearchibald |
|
Firefox implements what the spec says AFAIK. We key our offline script storage using a UUID which is generated for each version of a particular worker. When we do an update we compare the new scripts to the scripts stored under the currently active worker's UUID. Personally I think the spec behavior makes sense. Skipping install events seems dubious to me. A script could do different things based on |
|
@wanderview, thanks for checking out the behavior.
This is a good point. Could you confirm if Firefox has a separate storage for imported scripts, too? |
We do. Our scripts are stored in Cache API with a Cache name keyed off both the UUID and a magic token which hides it from content script. All the scripts for a given worker end up in the same Cache. |
|
Yes, Chrome's implementation tries to do what the spec and Firefox are doing, so it's an unexpected bug if it's doing something else. Could you file a bug if you can reproduce this? In Chrome's implementation, ServiceWorkerVersion (one service worker version) tracks its scripts in ServiceWorkerScriptCacheMap which maps a URL to a resource in the disk cache. The ServiceWorkerScriptCacheMap of one version shouldn't be sharing resources with the map of another version. You can see it gets populated by ServiceWorkerWriteToCacheJob, which always is given a new resource id for a given version + script url pair. |
|
@mattto, thanks for the clarification. It wasn't that I had come across any unexpected behavior against the spec. I just thought, while looking into the code, different versions would share the same resource_id for the same url. Sorry for my confusion. I'll work on it based on the behavior clarified here. |
This change includes/considers the following: - Include imported scripts to byte-check (for classic scripts). - Compare responses' body instead of source text as per whatwg/html#3316. - Handle duplicate importScripts() as per #1041. - Replace *imported scripts updated flag* referenced in importScripts() by using service worker's state item. - Have Update's perform the fetch steps cover module scripts. - Avoid dobule-download of imported scripts pointed out in #1023 (comment). This change basically makes it check out if the main script resource is identical to the existing resource. If so, it returns; otherwise, it creates a new service worker and evalute it to check out if any imported scripts are changed. It continues with Install only when any of the resources has been changed. With the change, importScripts() returns resources from the cache for any duplicated requests including the request for the main script. Fixes #1041, #1212, #1023.
This change includes/considers the following: - Include imported scripts to byte-check (for classic scripts). - Compare responses' body instead of source text as per whatwg/html#3316. - Handle duplicate importScripts() as per #1041. - Remove imported scripts updated flag referenced in importScripts() by using service worker's state item instead. - Have Update's perform the fetch steps cover module scripts. - Confirm dobule-download of imported scripts pointed out in #1023 (comment) never happens; importing a script again always gets the result from the cache. This change basically gets the imported scripts included to the byte-check for updates. Update continues with Install if any of the (main or imported) resources has been changed. This change also makes any duplicated requests (including the main script) from importScripts() return the result from the cache. Fixes #839, #1041, #1212, #1023.
When we are calling the loadLibs function, which in turn calls: importScripts(settings.mozjpeg_script); importScripts(settings.resize_script); For the media-optimization-worker service worker, we are getting an error in Firefox, which balks at wasm_bindgen, a global variable defined with let, being redefined when the module loads. This causes image processing to fail in Firefox when more than one image is uploaded at a time. The solution to this is to just check whether the scripts are already imported, and if so do not import them again. Chrome doesn't seem to care about this variable redefinition and does not error, and it seems to be expected behaviour that the script can be loaded multiple times (see w3c/ServiceWorker#1041)
When we are calling the loadLibs function, which in turn calls: importScripts(settings.mozjpeg_script); importScripts(settings.resize_script); For the media-optimization-worker service worker, we are getting an error in Firefox, which balks at wasm_bindgen, a global variable defined with let, being redefined when the module loads. This causes image processing to fail in Firefox when more than one image is uploaded at a time. The solution to this is to just check whether the scripts are already imported, and if so do not import them again. Chrome doesn't seem to care about this variable redefinition and does not error, and it seems to be expected behaviour that the script can be loaded multiple times (see w3c/ServiceWorker#1041)
What should happen when registering a service worker that does:
importScripts('script.js'); importScripts('script.js');In Chrome's implementation, the response for the first importScripts() is stored (so that if SW completes installing, that is the version that is persisted), and the second one goes to network ignoring the first one and is not persisted.
That seems reasonable but as part of another bug fix, I'm thinking of changing this so that the second importScripts() returns the first stored version.
Also what should happen when a service worker recursively importScripts() itself? In Chrome's implementation, this causes a JS stack overflow, and each call goes to network, but only the first one is attempted to be stored. I think I'll simply respond with a network error in this case since handling it is wasteful and futile.
Thoughts?
The text was updated successfully, but these errors were encountered: