Include imported scripts to byte-check #1023
Conversation
| : Output | ||
| :: True or false, a boolean | ||
|
|
||
| 1. If |sourceScript| and |targetScript| are not both [=classic scripts=] or not both [=module scripts=], return false. |
| 1. If |targetResponse|'s [=response/type=] is "<code>error</code>", or |targetResponse|'s [=response/status=] is not an [=ok status=], return false. | ||
| 1. If the result of [=UTF-8 decoding=] |response|'s [=response/body=] is not a byte-for-byte match with the result of [=UTF-8 decoding=] |targetResponse|'s [=response/body=], return false. | ||
| 1. If |sourceScript| is a [=module script=], then: | ||
| 1. If |sourceScript|’s [=module script/module record=]'s \[[ECMAScriptCode]] is not a byte-for-byte match with |targetScript|’s [=module script/module record=]'s \[[ECMAScriptCode]], return false. |
There was a problem hiding this comment.
Shouldn't we check imported scripts too?
There was a problem hiding this comment.
importScripts() is not supported in module scripts: https://html.spec.whatwg.org/#import-scripts-into-worker-global-scope step 1.
There was a problem hiding this comment.
Yeah, I meant imported as in import whatever from "./foo.js"
There was a problem hiding this comment.
I thought those urls are fetched, parsed and stored as part of the module record, but maybe I'm wrong. I noticed an environment settings object has a module map which is referenced throughout traversing the module script graph. I'll check the algorithms again.
There was a problem hiding this comment.
Oh does it flatten it somehow? I couldn't figure it out 😞
There was a problem hiding this comment.
@jakearchibald, I think you're right. I can't find any step that flattens the entire imported assets. It seems each module script has module record that has the parsed source text and info to the required modules (record's [[RequestedModules]] in ECMAScript language.) Getting to ModuleEvaluation() when running a module script, it evaluates all the related module scripts recursively finding the dependencies.
And I think we should compare all the module scripts stored in an environment settings object's module map. I'd want to confirm this with @domenic before updating this part.
@domenic, SW intends to compare all the script assets including the main script and the imported scripts for both classic and module scripts. For classic scripts, SW has it's own imported scripts map that's used for this byte-by-byte check. But for module scripts, I think I misunderstood that the main script's module record has a flattened asset that include all the imported scripts in the graph. I'd like to check with you whether this is wrong, and if so, iterating an environment setting's object's module map and compare the source texts of all the module scripts in the map would be a right way for our purpose?
There was a problem hiding this comment.
Hmm, [[ECMAScriptCode]] appears to be a "parse result" according to the spec, but the spec doesn't define what that is 😢 . But since it's the result of parsing it doesn't feel appropriate for a byte-for-byte check.
The way ModuleDeclarationInstantiation performs "For each ImportEntry" makes me think that there isn't any flattening going on. Maybe HTML's module map is an easier way in, since it's at least a flattened view of the imported scripts - although this may contain dynamically imported items that we're not interested in.
If I'm right that [[ECMAScriptCode]] isn't the source text, and source text is only available on classic scripts, we need to store this ourselves.
"Perform the fetch" is called for the top level script, but also each module fetch, so we could use this hook to store our own collection of source texts for byte-by-byte comparison.
@domenic does the above sound right at all? Our intent is to refetch the top-level script and all its imports, and see if any are byte-different to the original.
There was a problem hiding this comment.
@jungkees whoa, I replied before seeing your message. Weird how similar they are!
There was a problem hiding this comment.
Right, there is no flattened representation of the graph. I think checking everything in the module map is more correct.
I guess [[ECMAScriptCode]] is indeed not correct :-/. However, what we can do is update HTML to move "source text" from classic scripts only to the base script type. We'd probably want to add a note saying something like "source text is only used for classic scripts in this specification, but the Service Workers specification needs to store it for reasons". I don't think you all should create a separate parallel registry :).
There was a problem hiding this comment.
That sounds good. Moving the source text to the base script type will help us use the source text of scripts for both classic and module script cases. I'll spec it to checking everything in the module map with that change.
Another thing that needs a solution is the lifetime of the scripts and the map for SW. With other web workers, the lifetime of the scripts is tied to the WorkerGlobalScope object, so it doesn't store the main script and only stores the module scripts in the module map on the global. But with SW, the lifetime of the scripts outlives those globals, so it stores the main script and the imported scripts on a service worker concept rather than on the SW global. Because of this difference, I think we need to discuss around how to make the fetch a classic worker script and the fetch a module worker script graph work more correctly with SW. There's an opened issue for this: #1013.
| 1. Let |sourceMap| be |sourceScript|'s [=script resource/imported scripts map=]. | ||
| 1. [=map/For each=] |url| → |response| of |sourceMap|: | ||
| 1. Let |request| be a new [=/request=] whose [=request/url=] is |url|, [=request/client=] is |job|'s [=job/client=], [=request/type=] is "<code>script</code>", [=request/destination=] is "<code>script</code>", [=request/parser metadata=] is "<code>not parser-inserted</code>", [=request/synchronous flag=] is set, and whose [=request/use-URL-credentials flag=] is set. | ||
| 1. Set |request|'s [=request/cache mode=] to "<code>no-cache</code>" if any of the following are true: |
There was a problem hiding this comment.
This is going to cause a double-download. I'll explain this in another comment…
|
When the imported scripts are checked, we're doing it with I hadn't thought about this until reading this PR, but I think it's something we need to solve. "Check If Service Worker Resources Are Identical" could return a boolean as it does now, but also a map of all the scripts it fetched. Then, |
Right. I concerned about this too, but it was still better than invoking Run Service Worker for every navigation.
It seems a bit irony that we should introduce this kind of temporary cache to support bypassing HTTP cache. But yeah I think this is definitely a way to avoid the double-fetch of the seemingly identical resource (unless the content is dynamically changing in the server.) I'd like to also hear opinions from @wanderview and @mattto. |
Hah, yeah. But right now we fetch the main script, do the comparison, then reuse that script we fetched. It seems weird to reuse for the main script but refetch imported scripts. Also, this double-fetch means the scripts we byte-check could be different to the ones we end up running, which seems weird. |
How about:
Note, step (3) would also fix any issues where we go to network/http cache multiple times if script eval calls |
|
I think @jakearchibald and @wanderview largely suggested the same thing. Basically byte-check will populate a temporary map if the fetched resource is new. I think if we define and maintain the temporary map on a service worker registration, we can just add and update entries throughout the registration's lifetime. I'll add this change to the PR. (*) This is the SW's script resource map in the current spec. I suggested changing the name and the location of the concept as such through this PR for clarity and a better interface to other algorithms. |
This changes the behavior of the service worker script resource comparison. Before this, only the main service worker script was compared to a new version. With this change, all the imported scripts stored in the imported scripts map as well as the main script are inspected against the corresponding network resources (based on the urls.) Note: - Service worker's script resource map has been renamed and moved to service worker's script resource's imported scritps map. - registration's last update check time's always updated whenever the response is fetched from the network (regardless it's a main script or an imported script.) Fixes #839.
jungkees
force-pushed
the
include-imported-scripts-for-byte-check
branch
from
7cd7668
to
548e59a
Compare
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.
|
#1283 obsoleted this PR. |
This changes the behavior of the service worker script resource
comparison. Before this, only the main service worker script was
compared to a new version. With this change, all the imported scripts
stored in the imported scripts map as well as the main script are
inspected against the corresponding network resources (based on the
urls.)
Note:
service worker's script resource's imported scritps map.
response is fetched from the network (regardless it's a main script or
an imported script.)
Fixes #839.