-
Notifications
You must be signed in to change notification settings - Fork 312
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't associate a job with a client, instead just have a referrer. #889
Conversation
The client of a job was used for two purposes: 1) To set as client on the requests for fetching the service worker script. This was replaced with setting the referrer of the request, while leaving the client null. This is consistent with how regular workers and shared workers fetch their source (with the difference that there the client of the request is the worker itself, and the referrer is the page/worker that created the worker). This is still somewhat problematic for module type workers though. The algorithm for fetching a module script tree requires a client to store for a module map to store the modules on (and also doesn't have a separate referrer, which might or might not be intentional). Not sure what to do here, but the previous spec was wrong anyway, since it could pass a null client and should be passing something representing the service worker. This is issue 849. 2) To verify that the client from which a service worker was registered or unregistered was same origin with the service worker. For this we now just use the origin of the job's referrer.
<dt><em>"<code>module</code>"</em></dt> | ||
<dd><p><a>Fetch a module script tree</a> given <var>job</var>’s <a>serialized</a> <a href="#dfn-job-script-url">script url</a>, "<code>omit</code>", "<code>serviceworker</code>", and <var>job</var>’s <a href="#dfn-job-client">client</a>.</p></dd> | ||
<dd><p><a>Fetch a module script tree</a> given <var>job</var>’s <a>serialized</a> <a href="#dfn-job-script-url">script url</a>, "<code>omit</code>", "<code>serviceworker</code>", and some settings object.</p></dd> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@domenic any suggestions on how to best address this? For dedicated and shared workers a settings object is created for the worker before "fetch a module script tree" is invoked, and then that settings object is used to get a module map to store the fetched modules on. For service workers we generally don't create a settings object until the worker actually is started.
Also I would somewhat expect the main script of a module worker to be fetched with the document/worker that created the worker as referrer, similar to how class workers use that as referrer. With the current spec of "Fetch a module script tree" that isn't possible/what happens though. All modules, including the main module of a worker are fetched with the worker itself as referrer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't start a worker until its global scope/settings object have been created; it doesn't really make sense. Why do you think it's necessary in this case?
It sounds like there's a bug in the referrer handling though; filing an issue at whatwg/html would be much appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't start a worker until its global scope/settings object have been created; it doesn't really make sense. Why do you think it's necessary in this case?
Currently service workers do: fetch, create settings object, start
, while other workers do: create settings object, fetch, start
. So in both cases start happens after creating a settings object, it's just the fetching that happens differently.
The way the algorithms are currently written we don't create a settings object until after an update check has decided that a new script was indeed downloaded and we really need to start the worker. But yeah, it might actually be a lot clearer/easier to understand if we always create a settings object/global scope for the service worker before we initiate any kind of fetch/update check. If the update check fails we just throw the settings object away, and if the update check succeeded we can then reuse it to fire the install event at etc. Additionally at the end of a successful update check we can store the settings object's module map, similarly to how we currently store the workers script (and when we have to restart the same worker we can prepopulate the settings object's module map with that same module map). That would also address a couple of other places where the spec currently doesn't make sense around module type workers.
Actually rewriting things like that would be a fairly major undertaking, but probably the best way to actually make this all make sense...
It sounds like there's a bug in the referrer handling though; filing an issue at whatwg/html would be much appreciated!
Will do.
As discussed in #1122, #1111, and in w3c/ServiceWorker#889 (comment), a number of problems are caused by the current setup of using the settings object of the worker itself as the feth client. Instead, we use the incumbent settings object to do the fetching. (Incumbent, instead of e.g. current, because almost everything else in the (Shared)Worker constructors uses the incumbent settings object.) This fixes #1111 since now module workers are fetched with the correct client, and thus automatically get the correct referrer.
As discussed in #1122, #1111, and in w3c/ServiceWorker#889 (comment), a number of problems are caused by the current setup of using the settings object of the worker itself as the feth client. Instead, we use the incumbent settings object to do the fetching. (Incumbent, instead of e.g. current, because almost everything else in the (Shared)Worker constructors uses the incumbent settings object.) This fixes #1111 since now module workers are fetched with the correct client, and thus automatically get the correct referrer.
As discussed in #1122, #1111, and in w3c/ServiceWorker#889 (comment), a number of problems are caused by the current setup of using the settings object of the worker itself as the feth client. Instead, we use the incumbent settings object to do the fetching. (Incumbent, instead of e.g. current, because almost everything else in the (Shared)Worker constructors uses the incumbent settings object.) This fixes #1111 since now module workers are fetched with the correct client, and thus automatically get the correct referrer.
As discussed in #1122, #1111, and in w3c/ServiceWorker#889 (comment), a number of problems are caused by the current setup of using the settings object of the worker itself as the feth client. Instead, we use the incumbent settings object to do the fetching. (Incumbent, instead of e.g. current, because almost everything else in the (Shared)Worker constructors uses the incumbent settings object.) Notably, for fetching module script trees, this necessitates separating the fetch client settings object from the one used for the module map. This fixes #1111 since now module workers are fetched with the correct client, and thus automatically get the correct referrer.
As discussed in #1122, #1111, and in w3c/ServiceWorker#889 (comment), a number of problems are caused by the current setup of using the settings object of the worker itself as the fetch client. Instead, we use the incumbent settings object to do the fetching. (Incumbent, instead of e.g. current, because almost everything else in the (Shared)Worker constructors uses the incumbent settings object.) Notably, for fetching module script trees, this necessitates separating the fetch client settings object from the one used for the module map. This fixes #1111 since now module workers are fetched with the correct client, and thus automatically get the correct referrer. Dependencies, however, require further work which will happen as part of #1150.
Actually don't think that this was the right way to go, as jobs will still need a client. |
The client of a job was used for two purposes:
To set as client on the requests for fetching the service worker script.
This was replaced with setting the referrer of the request, while leaving
the client null. This is consistent with how regular workers and shared
workers fetch their source (with the difference that there the client of
the request is the worker itself, and the referrer is the page/worker that
created the worker).
This is still somewhat problematic for module type workers though. The
algorithm for fetching a module script tree requires a client to store
for a module map to store the modules on (and also doesn't have a
separate referrer, which might or might not be intentional). Not sure
what to do here, but the previous spec was wrong anyway, since it could
pass a null client and should be passing something representing the
service worker. This is issue Spec uses incorrect settings object when fetching a module worker #849.
To verify that the client from which a service worker was registered or
unregistered was same origin with the service worker. For this we now
just use the origin of the job's referrer.