Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

mkruisselbrink
Copy link
Collaborator

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 Spec uses incorrect settings object when fetching a module worker #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.

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>
Copy link
Collaborator Author

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.

Copy link
Contributor

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!

Copy link
Collaborator Author

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.

domenic added a commit to whatwg/html that referenced this pull request Apr 29, 2016
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.
domenic added a commit to whatwg/html that referenced this pull request May 2, 2016
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.
domenic added a commit to whatwg/html that referenced this pull request May 3, 2016
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.
domenic added a commit to whatwg/html that referenced this pull request May 10, 2016
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.
annevk pushed a commit to whatwg/html that referenced this pull request May 11, 2016
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.
@mkruisselbrink
Copy link
Collaborator Author

Actually don't think that this was the right way to go, as jobs will still need a client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants