1462772 - route ServiceWorker state changes through ServiceWorkerRegistration

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement, P2)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1462467 I added some code to bring the ServiceWorkerRegistration up-to-date.  That code did a simple set to the latest value.  That was not quite correct.  There are still some problems:

1. registration.installing.state is "parsed" instead of "installing" immediately after a register() call
2. A ServiceWorker can transition straight from "parsed" or "installing" to "activated".  This is incorrect.  It must transition through "installed" and "activating".
3. The registration.installing, .waiting, and .active properties jump to their final state.  Instead they should transition with the ServiceWorker.state values.  So when a ServiceWorker.state is "installed", then that ServiceWorker must be set as the .waiting value.

Its easy to bring the registration and service worker objects incrementally up-to-date independently.  They must be synchronized, though.  That is going to be hard.  Its extra hard considering that the ServiceWorker object can sometimes exist without it associated registration object (e.g. MessageEvent.source).

This will probably take a while to figure out.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Maybe I can make the ServiceWorker accept state updates from the ServiceWorkerRegistration as well.  Then I could have that drive the state updates when its attached to the ServiceWorkerRegistration.  If its not attached to the registration then it would get updates directly from its own actor.  I'll have to think about this a bit more.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

After thinking about this over the weekend I think I've settled on the following plan:

1. Every ServiceWorker binding object that might receive a state update will hold a reference to its associated ServiceWorkerRegistration.
2. On creation the ServiceWorker will try to get the SWR from the global's cache.  If its not there, then it will request a ServiceWorkerRegistrationDescriptor be provided from the inner on creation.
3. Each ServiceWorker object will add itself as a state change listener on its associated ServiceWorkerRegistration.  All state changes come through the registration.
4. The ServiceWorkerManager code will keep a state change list for each registration.
5. Changes will be tracked using a 64-bit serial number.
6. All ServiceWorkerRegistrationDescriptor and ServiceWorkerDescriptor objects will include the serial number representing the version of the object used to create the descriptor.
7. On creation the binding object will receive a list of changes to bring its serial number up-to-date.
8. The change list will be pruned opportunistically to remove changes that are considered "old".  Here "old" probably means anything beyond a few seconds to a minute.  We really just need to handle updating state across some race condition situations.  So we don't need a hugely long history.

This will be slightly sub-optimal for the case where a client receives a postMessage from a service worker it has not otherwise interacted with.  This is a somewhat rare case, though.  In almost all cases the ServiceWorkerRegistration will already exist.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a fallible nsIGlobalObject::GetServiceWorkerRegistration() method. r=baku

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add the registration Id to the ServiceWorkerDescriptor. r=baku

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make ServiceWorker binding objects get or create a handle to their associated registration. r=baku

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make ServiceWorker objects add themselves to the ServiceWorkerRegistration as a weak point back reference. r=baku

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip.patch

Work in progress.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8979619 [details] [diff] [review]
P4 Make ServiceWorker objects add themselves to the ServiceWorkerRegistration as a weak point back reference. r=baku

I don't think we need this one after all.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip.patch

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip.patch

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-version.patch

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-reg-listener.patch

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-version-list.patch

This seems to fix most of the problems in my local testing.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip-before-state-via-reg.patch

I'm going to make this bug just about sending ServiceWorker state changes via the registration for now.  Version list and whatnot will be in a follow-on bug.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4305bb0235b49322632f5857554a32721002c067

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a fallible nsIGlobalObject::GetServiceWorkerRegistration() method. r=mrbkap

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add the registration Id to the ServiceWorkerDescriptor. r=mrbkap

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make ServiceWorker binding objects get or create a handle to their associated registration. r=mrbkap

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Route ServiceWorker state changes through the ServiceWorkerRegistration. r=mrbkap

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove now-unused Listener logic in ServiceWorkerInfo. r=mrbkap

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a57573ba0f2a88bf80446926542df50b137c4b98

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make ServiceWorker binding objects get or create a handle to their associated registration. r=mrbkap

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Route ServiceWorker state changes through the ServiceWorkerRegistration. r=mrbkap

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Remove now-unused Listener logic in ServiceWorkerInfo. r=mrbkap

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84c1749d5c18b1f38d8b840c74cfc8185d2143e9

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8988211 [details] [diff] [review]
P1 Add a fallible nsIGlobalObject::GetServiceWorkerRegistration() method. r=mrbkap

Blake, this patch adds a method to nsIGlobalObject to get the current ServiceWorkerRegistration binding object for a given descriptor.  This is very similar to the existing GetOrCreate*() method, but it simply returns nullptr if there is no object instead of creating one automatically.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8988212 [details] [diff] [review]
P2 Add the registration Id to the ServiceWorkerDescriptor. r=mrbkap

So, every ServiceWorker in the browser must belong to some registration.  This patch adds the registration's ID value to the ServiceWorkerDescriptor.  This allows the ServiceWorkerDescriptor to be mapped back to the exact registration for the given principal/scope.  Without the id value it would be possible to get confused and return the wrong registration if the original was removed and a new registration created for the same scope.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8988273 [details] [diff] [review]
P3 Make ServiceWorker binding objects get or create a handle to their associated registration. r=mrbkap

This patch uses the registration id on the ServiceWorkerDescriptor to make the ServiceWorker look-up and store a reference to its corresponding ServiceWorkerRegistration.  This will create a ref-cycle, but the cycle collector knows about it and will cut it if neither binding object is otherwise referenced.

I'll go into why we want this reference in the next patch.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8988274 [details] [diff] [review]
P4 Route ServiceWorker state changes through the ServiceWorkerRegistration. r=mrbkap

This patch is the main goal of this bug.  It routes all state changes for a ServiceWorker through its corresponding ServiceWorkerRegistration.

Why do we want to do this?  Well, it comes down to how to maintain the state of various binding objects when different events fire.

Consider, we have these properties that can change at various points:

* ServiceWorker.state
* ServiceWorkerRegistration.installing (null or a ServiceWorker)
* ServiceWorkerRegistration.waiting (null or a ServiceWorker)
* ServiceWorkerRegistration.active (null or a ServiceWorker)

We also have these events:

* "statechange" fired by ServiceWorker
* "updatefound" fired by ServiceWorkerRegistration

The spec and WPT tests expect the properties to be in specific states when the events are fired.  Today this is achievable because we keep all this information in the main thread in the same process in the SWM.  We don't expose ServiceWorker in worker globals yet, so we get a pass on OMT there.

With the new IPC model, however, things get a lot harder.  My initial attempt was to simply update the ServiceWorker and ServiceWorkerRegistration independently like we have done in the past.  As long as we fire updates in the correct order, then this should work.

The problem, however, is that we can't fire an update to a binding object if its doesn't exist yet.  And the ServiceWorkerDescriptor used to create the binding object in the child-process may be out-of-date compared to the information stored in SWM in the parent process.

To deal with the "out-of-date on attachment" issue I tried making each binding object self-update.  But we cannot update these objects independently.  The tests require the registration and worker objects to be updated in lockstep together.

So this patch starts to fix this problem by routing all state changes through the ServiceWorkerRegistration.  The ServiceWorkerRegistration then tells each of its ServiceWorker objects about the changes.  After properties have been updated events are fired.

This patch only deals with statechange events so far.  A later bug will incorporate updatefound event into this system as well.

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8988276 [details] [diff] [review]
P5 Remove now-unused Listener logic in ServiceWorkerInfo. r=mrbkap

This patch just removes the now-unused code for dispatching state changes from the ServiceWorkerInfo.  These now all happen through ServiceWorkerRegistrationInfo instead.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8988273 [details] [diff] [review]
P3 Make ServiceWorker binding objects get or create a handle to their associated registration. r=mrbkap

Review of attachment 8988273 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/serviceworkers/ServiceWorker.cpp
@@ +104,5 @@
> +        nsIGlobalObject* global = self->GetParentObject();
> +        NS_ENSURE_TRUE_VOID(global);
> +        RefPtr<ServiceWorkerRegistration> reg =
> +          global->GetOrCreateServiceWorkerRegistration(aDescriptor);
> +        self->MaybeAttachToRegistration(reg);

I'm a bit wary about what happens in the time between when we post this and when we call MaybeAttachToRegistration. I guess we don't need it because there can't be any other service workers to confuse us with?

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8988274 [details] [diff] [review]
P4 Route ServiceWorker state changes through the ServiceWorkerRegistration. r=mrbkap

Review of attachment 8988274 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/serviceworkers/ServiceWorkerRegistration.cpp
@@ +147,5 @@
> +  // to be moved to the redundant state.
> +  AutoTArray<RefPtr<ServiceWorker>, 3> oldWorkerList;
> +  oldWorkerList.AppendElement(mInstallingWorker.forget());
> +  oldWorkerList.AppendElement(mWaitingWorker.forget());
> +  oldWorkerList.AppendElement(mActiveWorker.forget());

Can we use an initializer list here? `... oldWorkerList({ mInstallingWorker.forget(), ... });`?

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Blake Kaplan (:mrbkap) from comment #28)
> ::: dom/serviceworkers/ServiceWorker.cpp
> @@ +104,5 @@
> > +        nsIGlobalObject* global = self->GetParentObject();
> > +        NS_ENSURE_TRUE_VOID(global);
> > +        RefPtr<ServiceWorkerRegistration> reg =
> > +          global->GetOrCreateServiceWorkerRegistration(aDescriptor);
> > +        self->MaybeAttachToRegistration(reg);
> 
> I'm a bit wary about what happens in the time between when we post this and
> when we call MaybeAttachToRegistration. I guess we don't need it because
> there can't be any other service workers to confuse us with?

There are a couple things that can happen, but I think they are all ok:

1. The global is destroyed.  This is caught by the NS_ENSURE_TRUE_VOID(global).
2. The ServiceWorker object is un-referenced and could be GC'd.  The lambda captured self ref will hold it alive until we set the registration.  It will then be destroyed immediately after.  Kind of wasteful, but not critical.

All other state changes must come through the registration.  The only thing that can initiate from the ServiceWorker object itself is a postMessage(), but that is unaffected by the registration attachment.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Blake Kaplan (:mrbkap) from comment #29)
> ::: dom/serviceworkers/ServiceWorkerRegistration.cpp
> @@ +147,5 @@
> > +  // to be moved to the redundant state.
> > +  AutoTArray<RefPtr<ServiceWorker>, 3> oldWorkerList;
> > +  oldWorkerList.AppendElement(mInstallingWorker.forget());
> > +  oldWorkerList.AppendElement(mWaitingWorker.forget());
> > +  oldWorkerList.AppendElement(mActiveWorker.forget());
> 
> Can we use an initializer list here? `... oldWorkerList({
> mInstallingWorker.forget(), ... });`?

Wow, that works?  I had no idea.  I'll try it.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Route ServiceWorker state changes through the ServiceWorkerRegistration. r=mrbkap

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e79b0494417b
P1 Add a fallible nsIGlobalObject::GetServiceWorkerRegistration() method. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a025ad59747
P2 Add the registration Id to the ServiceWorkerDescriptor. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/f35e21f7239e
P3 Make ServiceWorker binding objects get or create a handle to their associated registration. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ba0b36ac85d
P4 Route ServiceWorker state changes through the ServiceWorkerRegistration. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9353d80e2f9
P5 Remove now-unused Listener logic in ServiceWorkerInfo. r=mrbkap

Comment 34

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e79b0494417b
https://hg.mozilla.org/mozilla-central/rev/5a025ad59747
https://hg.mozilla.org/mozilla-central/rev/f35e21f7239e
https://hg.mozilla.org/mozilla-central/rev/2ba0b36ac85d
https://hg.mozilla.org/mozilla-central/rev/c9353d80e2f9

Comment 35

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e79b0494417b
https://hg.mozilla.org/mozilla-central/rev/5a025ad59747
https://hg.mozilla.org/mozilla-central/rev/f35e21f7239e
https://hg.mozilla.org/mozilla-central/rev/2ba0b36ac85d
https://hg.mozilla.org/mozilla-central/rev/c9353d80e2f9