Repository navigation
Update shouldn't fail due to MIME check or 404 for the incumbent's worker scripts #1374
Description
The update check spiders the script resource map of the incumbent worker scripts, but then fails immediately if one script has a non-OK status or fails the MIME check. This means if the incumbent worker is using import-script.js which now 404s, and the updated worker doesn't use import-script.js, the update will fail.
Instead these checks shouldn't fail the whole update, since it's not known whether the new worker will use them.
For each url → storedResponse of newestWorker’s script resource map:
...
7. Extract a MIME type from the fetchedResponse’s header list. If this MIME type (ignoring parameters) is not a JavaScript MIME type, asynchronously complete these steps with a network error.
8. If fetchedResponse’s type is "error", or fetchedResponse’s status is not an ok status, asynchronously complete these steps with a network error.
Activity
mfalken commentedon Dec 19, 2018
Proposal: When spidering |newestWorker|'s script resource map, always write the response into |updatedResourceMap|. On MIME failure, network error, or byte-for-byte body diff, set |hasUpdatedResources| to true. This ensures the diff translates into starting the installing worker, which is correct since the worker scripts are different. When the installing worker does importScripts() a MIME failure or network failure will throw SecurityError/NetworkError so in effect the installation will usually fail. We also need to remove unused resources from the installing worker's map that ended up not being used, or these will be used in the next update check against this new worker. So augment the script resource map with a flag about whether the script was loaded. After installation finishes, drop unused scripts from the map.
Don't fail update upon error fetching the incumbent worker's import s…
mfalken commentedon Jan 17, 2019
At #1377, @wanderview pointed out that Firefox's implementation considers error importScripts to be "no change" on the update check, while the patch will consider a change of success/error to be a "change".
I think Firefox's implementation could be reasonable. The importScript update check will result in "no change" on error importScripts, expecting the main script to change to remove the offending import. I did not consider this when filing this issue.
But there is some inconsistency in the current spec because the normal install/update doesn't require the importScripts to be non-errors. E.g., suppose the worker is
try { importScripts('a.js`); importScripts('b.js'); } catch () {};. Then the site changes so 'a.js' is a 404 and 'b.js' content changes. The worker would not be updated because the 404 aborts the byte-for-byte check.A simpler issue is
try { importScripts('404.js`); } catch () {};would not update if '404.js' is changed to serve something, since 404.js wouldn't be in the service worker's script resource map (only non-error importScripts() are stored).I guess my patch makes for a more consistent system since error importScripts() are catchable so don't necessarily fail installation. But it comes at a cost of complexity: you must store the error imports now (or at least record that they were errors) and it's incompatible with Firefox's implementation.
I'm a bit undecided but I think I'm ok abandoning this and living with the inconsistency. One gentle change (which may be in-line with Firefox's implementation) is to consider failures to be "no change" but continue the update check. That would allow the first case above to update.
So the rule would be:
@makotoshimazu: do you know what Chrome's implementation is doing for error importScripts?
@jakearchibald @wanderview @asutherland WDYT?
mfalken commentedon Jan 17, 2019
Ah I think I found Chrome's (in-progress) implementation is doing the ""no change" and continue on to the next script" already: https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_single_script_update_checker.cc?rcl=e2f59f49da06e49c476bd0d32913d7349d61a0d4&l=175
makotoshimazu commentedon Jan 17, 2019
Yeah, it's implemented as no change tentatively. this is the latest code, which treats network error differently from 200 and identical case, but it behaves similarly for now.
Treating network error on comparison as "no change" sounds reasonable to me too.
tingshao commentedon Jan 17, 2019
I'm watching this issue for some time, I just updated chrome's code for byte to byte check several days ago. If a network error is found for a script, the failure result is recorded but it's not treated as "Changed". This error result may be useful in future to distinguish it from a real not changed script if we want to do more work on it.
Currently, only an explicit content change leads to "Changed". Treat the error as "no change" also sounds reasonable to me.
wanderview commentedon Jan 17, 2019
I believe this is consistent with the firefox implementation.
asutherland commentedon Jan 18, 2019
I am (unsurprisingly) okay with doing what Firefox already does here. ("no change" on error). And a big thank you to wanderview for raising the point!
Don't fail update upon error fetching the incumbent worker's import s…
Don't fail update upon error fetching the incumbent worker's import s… (
mfalken commentedon Mar 5, 2019
Fixed by #1377