Does changing updateViaCache mint a new ServiceWorkerRegistration or update them all?

[mfalken]

Suppose you have something like this:

var reg;
navigator.serviceWorker.getRegistration(url).then(() => { reg = doc; });
reg.updateViaCache;  // 'all';
register(sw, {updateViaCache: 'none'}).then(r => {
  reg === r;  // true?
  reg.updateViaCache; // 'all' or 'none'?
});

I'm leaning toward updating all existing ServiceWorkerRegistration objects, because otherwise ServiceWorkerGlobalScope#registration would have to mutate to a brand new registration object after register() settled, which doesn't feel right.

However, updating them all is a bit troublesome and seems to lead to race conditions (one of them has been updated but the others haven't). Would it be worth considering changing reg.updateViaCache to a promise similar to NavigationPreloadManager state?

@wanderview what does Firefox do?

[mfalken]

(The spec doesn't seem to address this, we get to "Invoke Update algorithm passing job as the argument" but nothing there updates updateViaCache AFAICT)

[jakearchibald]

Good catch. The other properties are live, so it'd be confusing to make other properties a snapshot.

We could add getState() which we could add other settings to later.

const { updateViaCache } = await reg.getState();

[wanderview]

We only just recently landed this in firefox nightly. There I think you will get:

var reg = await navigator.serviceWorker.getRegistration(url);
reg.updateViaCache;  // 'all';
var r = await register(sw, {updateViaCache: 'none'});
reg === r;  // false
reg.updateViaCache; // 'all'
r.updateViaCache; // 'none'

I think this behavior is determined by step 5.3 here in the spec:

https://w3c.github.io/ServiceWorker/#register-algorithm

You should skip the short-circuit because the cache mode is different and get a new registration.

EDIT: This comment is wrong...

[wanderview]

And the update algorithm gets the cache mode out of the newest worker in step 3 and step 7.2 here:

https://w3c.github.io/ServiceWorker/#update-algorithm

So I'm not sure I understand the particular problem here.

Edit: This comment is wrong...

[wanderview]

Errr... my last comment makes no sense because updateViaCache is on the registration, not the worker.

I think changing the updateViaCache property is analogous to changing the scriptURL. The problem is we put updateViaCache on the registration instead of the worker itself.

[wanderview]

In firefox we change the updateViaCache value immediately during the start of the register job here:

http://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerRegisterJob.cpp#42

We didn't deal with exposing it on the worker thread yet because there are lots of things we already do not expose there.

I agree we need a spec change somewhere here.

[jungkees]

Putting updateViaCache on the registration seems fine to me. We're registering or updating a registration's state. So, such processes using the last value of the registration seems good. If it had been on each service worker, the values used for the installation of existing workers wouldn't have been changed anyway.

I think this property should be live as the other properties though. The spec currently doesn't update the value in Update algorithm, but I think the value should be updated by referencing job's update via cache mode before fetching.

The expected behavior that I think of is:

var reg = await navigator.serviceWorker.getRegistration(url);
reg.updateViaCache;  // 'all';
var r = await register(sw, {updateViaCache: 'none'});
reg === r;  // true
reg.updateViaCache; // 'none'
r.updateViaCache; // 'none'

I think the update via cache mode isn't part of the identity of a registration but just a strategy clients set. WDYT?

[wanderview]

Do we have other persistent state on the service worker that is mutable without installing a new service worker? I don't think we do right now.

[wanderview]

Also, I find it weird that register() would change state, but not advertise it did so by creating a new registration. Right now if register() resolves the current registration then we know it short-circuited and nothing changed.

[jungkees]

Do we have other persistent state on the service worker that is mutable without installing a new service worker? I don't think we do right now.

No. Not until updating. My point was adding that info to a service worker instead of a registration wouldn't be more useful.

Also, I find it weird that register() would change state, but not advertise it did so by creating a new registration. Right now if register() resolves the current registration then we know it short-circuited and nothing changed.

Interesting. It seems you think of a JS registration object a thing that represent the state of the underlying registration struct. I think we discussed this a way back and settled on the behavior the JS objects for the same registration struct pass equality check. And the registration object's attribute getters return the latest values set by register/update operations. The current implementations reflect that behavior, right?

[mfalken]

Well there is the navigation preload state that can be mutated.

Yes ServiceWorkerRegistration objects representing the same registration entity pass the JS equality check (assuming they're from the same document/worker context, otherwise they are not equal due to different prototypes).

I'm not sure about creating a new registration just for the updateViaCache change. I'd find it surprising for ServiceWorkerGlobalScope#registration to change over the lifetime of a the service worker.

[wanderview]

I'd find it surprising for ServiceWorkerGlobalScope#registration to change over the lifetime of a the service worker.

You would get a new service worker version with the new registration. So this would not happen.

Well there is the navigation preload state that can be mutated.

True, but AFAICT that state is not set as part of a register() call. Maybe we should let updateViaCache be set through a method like navigationPreload.enable().

FWIW, though, we are currently planning to ship the behavior in #1189 (comment) in FF57. I feel like this is a bit of a corner case, so I think we can change it later as long as we maintain an updateViaCache in register().

[wanderview]

Maybe we should discuss at TPAC.

FWIW, my personal preference here would be to try to treat all values passed to register() in a similar fashion. So try to treat updateViaCache similar to the script URL. It can be changed via registration, but creates a new registration.

Edit: And I would prefer to move updateViaCache to the ServiceWorker instead of the registration just like scriptURL.

@jakearchibald do you have an opinion on this from a developer ergonomics point of view?

[mfalken]

Good topic for TPAC :)

You would get a new service worker version with the new registration. So this would not happen.

Just to confirm, suppose there is an active worker worker.js running and updateViaCache is 'all'. You do reg = await register('worker.js', { updateViaCache: 'none' });. reg.active would be worker.js and reg.updateViaCache would be 'none', but inside the worker registration.active is worker.js and registration.updateViaCache is still all? That kinda seems surprising.

So try to treat updateViaCache similar to the script URL. It can be changed via registration, but creates a new registration.

Ah this might be key. In Chromium script URL is just a property on the service worker. So a registration can have multiple script URLs. We don't mint a new registration when you register a worker with a different script URL to the newest worker:

reg = await navigator.serviceWorker.register('active.js');
// let it become active and have a client
reg2 = await navigator.serviceWorker.register('waiting.js');
reg == reg2; // true
reg.active.scriptURL;  // 'active.js'
reg.waiting.scriptURL; // 'waiting.js'

[wanderview]

Ah this might be key. In Chromium script URL is just a property on the service worker. So a registration can have multiple script URLs. We don't mint a new registration when you register a worker with a different script URL to the newest worker:

Right. You get a new ServiceWorker with the new scriptURL, not a new registration. I think I want updateViaCache to work the same way.

Since I've been so confused on this issue I'm going to write a glitch to see what we currently have implemented.

[wanderview]

Ok, so I'm just completely wrong and should probably go jump in a lake. We do what @jungkees suggested in #1189 (comment).

I wrote a glitch to test what we currently do:

https://sw-change-updateviacache.glitch.me/

Which on current firefox nightly 57 produces:

==> service worker script evaluation
==> reg1.updateViaCache: imports
==> service worker script event: install
==> service worker script event: activate
==> reg2.updateViaCache: none
==> reg1 === reg2: true
==> reg2.installing: null
==> reg2.waiting: null
==> reg2.active: [object ServiceWorker]

So we don't create a new registration. We also don't create a new ServiceWorker.

Looking at our code, though, I do think we trigger an update check at this point. We don't do the short circuit, but our update doesn't find any changes and just keeps the current service worker.

[jakearchibald]

My aim with this stuff is to try and reduce the amount of independently-set state as much as possible. This means if you update your call to .register, your registration will have the settings you gave it.

My worry with something like reg.setUpdateViaCache('all'), is that removing that line leaves users with whatever their current setting is. Whereas removing it from .register resets them to the default.

I wanted to do something similar with navigation preload, where you'd have to declare it per service worker instance (in the install or activate event), but we lost the ability to do that when we gained additional state – the header value.

The service worker registration object is already "live", since reg.active, reg.waiting and reg.active update. It'd be really confusing if we then add some objects that are snapshots.

When the registration changes, we already update registration objects if it results in a new .installing. Is it a big problem for performance/implementation to update reg.updateViaCache in a similar way?

If so, I'm happy to look at a getState solution, like we do with navigation preload.

[mfalken]

Is it a big problem for performance/implementation to update reg. updateViaCache in a similar way?

My implementor feedback is that it's not a big problem. If it makes sense from the ergonomics POV I support doing it this way. Of course async getState() would probably be simpler to implement, but you're right that we already do the live update for active/waiting/installing.

[wanderview]

Just to clarify, we do trigger an immediate update check if updateViaCache is changed. Another glitch with a timestamp service worker script:

https://sw-change-updateviacache-with-update.glitch.me/

Gives:

==> reg1.updateViaCache: imports
==> reg2.updateViaCache: none
==> reg1 === reg2: true
==> reg2.installing: [object ServiceWorker]
==> reg2.waiting: null
==> reg2.active: [object ServiceWorker]

I think this is reasonable to do, but not sure if it should be required by the spec.

[mfalken]

I see Firefox 57 is shipping updateViaCache. It sounds like the basic behavior is: when only updateViaChanges changes, don't mint a new ServiceWorkerRegistration or a new ServiceWorker (unless the byte-for-byte content changed), and ServiceWorkerRegistration#updateViaCache is a live property?

Do we have spec language and WPT tests for this? registration-updateviacache.https.html doesn't seem to test ServiceWorkerRegistration equality.

[wanderview]

I don't think this case is tested by a WPT yet, no.

Also, we have an issue where trying to access .updateViaCache after unregister() throws InvalidStateError at the moment. (Actually it crashes right now, but we are going to make it throw as a bandaid.)

I think the correct long term for this access after unregister is just to return the last value cached on the DOM object.

[mfalken]

Thanks. Would be good to have spec language and WPT tests soon. Chrome is (still) trying to implement this. The behavior about not minting a new registration and making updateViaCache live sounds good to me.

Checking updateViaCache after unregister seems pretty niche... either throwing or returning the last value sounds fine.

[wanderview]

If we are doing the "post a task to update the updateViaCache value on the right thread" thing, then I think just returning the last known value makes the most sense. The reason we are throwing is because we are implementing the live value by finding the registration when accessed (because we don't support access on worker threads yet).

[jakearchibald]

F2F: Keep same registration object, but update property value.

[mfalken]

We have consensus to make updateViaCache a live property, but no spec or WPT for that yet. Chrome is now implementing this. Would people be OK with us landing a WPT for it now (adding it to the existing registration-updateviacache test)? Or would you rather we not land WPT changes until the spec text is also updated?

[wanderview]

Just to clarify, by "live property" you mean updating a property on the binding object via a task, right?

A WPT would be great. I actually implemented this behavior while refactoring some other things in gecko. See P9 of this bug shipping in FF60:

https://bugzilla.mozilla.org/show_bug.cgi?id=1434701

Sorry I neglected to add a test at that time.

[mfalken]

I was a bit wrong. We have basic tests that the registration object remains the same and the updateViaCache property is updated after register() is called with a new value. I wanted to add a test that all registration objects for the same registration entity are updated. So the test would like:

let reg  = await register('sw.js');
reg.updateViaCache; // 'imports'
let frameReg = await frame.contentWindow.navigator.serviceWorker.getRegistration('sw.js');
await register('sw.js', {updateViaCache: 'none'});
reg.updateViaCache; // 'none'
frameReg.updateViaCache; // 'none'

I'm not sure when the spec guarantees that frameReg.updateViaCache is updated. So our test might use wait_for_activation_on_dummy_scope() to try to wait for the update to happen.

I'd also like to add a test for the unregister() case, just to ensure we don't crash.

We could do these as Chromium only tests since the spec might be vague here, but I prefer WPT tests by default.

[wanderview]

Sounds like a good WPT to me.

FWIW, in gecko I'm updating it whenever we hit "update registration state":

https://w3c.github.io/ServiceWorker/#update-registration-state-algorithm

And I effectively added to step 5 in Register:

https://w3c.github.io/ServiceWorker/#register-algorithm

These steps:

0. Set registration's updateViaCache to job's update via cache mode.
   0.5) Run the update registration state algorithm passing registration, null and null as the arguments.

Edit: One difference in gecko's "update registration state" is that we effectively sync the entire state there. We only fire statechange events and whatnot if something changes on the binding object. So we pretty much ignore the arguments passed.

[jakearchibald]

F2F: Need to update the spec for this.

[mfalken]

updateViaCache is a live property. WPT was added at https://github.com/web-platform-tests/wpt/blob/master/service-workers/service-worker/registration-updateviacache.https.html

[mfalken]

Addressed in #1411