1434342 - make ServiceWorker DOM object based mainly on ServiceWorkerDescriptor

Status: RESOLVED FIXED

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

Description

[Ben Kelly [:bkelly, not reviewing]]

This bug is to reduce the dependency between ServiceWorker DOM object and its backing ServiceWorkerInfo.  The majority of its functionality should be based on the ServiceWorkerDescriptor instead.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorker::Create() factory method. r=asuth

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make ServiceWorker store and use a ServiceWorkerDescriptor internally. r=asuth

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make each ServiceWorker DOM object automatically fire its statechange event when appropriate. r=asuth

Note to self, this is undoing a change added in bug 1189685.  We don't need it any more because we now return the same ServiceWorker each time for the same backing ServiceWorkerInfo.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make ServiceWorker operate on an abstract Inner interface that ServiceWorkerInfo implements. r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Support caching the ServiceWorker DOM instance on the global. r=asuth

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Support caching the ServiceWorker DOM instance on the global. r=asuth

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Make ServiceWorker call nsIGlobalObject::AddServiceWorker and RemoveServiceWorker. r=asuth

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Use the global to GetOrCreate the ServiceWorker DOM instance. r=asuth

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8946705 [details] [diff] [review]
P1 Add ServiceWorker::Create() factory method. r=asuth

The first step here is to change things so ServiceWorkerInfo is not directly creating the ServiceWorker object.  This makes us do something a bit silly for the "same process" case.  We have the ServiceWorkerInfo handing, but we go look it up again using the ServiceWorkerDescriptor.  This is silly now, but its effectively what we will have to do in the future when the SWM is in a different process and all we have is a SWD.

In the future the SWM lookup parts will change to create a different remoted object.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8946715 [details] [diff] [review]
P2 Make ServiceWorker store and use a ServiceWorkerDescriptor internally. r=asuth

This just makes the ServiceWorker object use the descriptor internally where it can.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8946717 [details] [diff] [review]
P3 Make each ServiceWorker DOM object automatically fire its statechange event when appropriate. r=asuth

This moves the "statechange" event dispatch into the ServiceWorker instead of a separate call.  We automatically fire the event when the state changes.

This is undoing an old change we made in bug 1189685.  Back then we returned many different ServiceWorker instance objects for the same ServiceWorkerInfo in a single browsing context.  This meant we needed all of them to be in sync with each other.  Now we instead return the same ServiceWorker instance where we can.  This means we don't need to sync across many instances.

Note, this will let some things get out of sync.  For example, if you have ServiceWorker instances in different globals they can not be slightly out of sync.  This is not tested for, though, and it would also be hard to implement in a multi-process world.  We would not be able to operate on each actor individually, but would have to group them by what process they were in.

Whats more, this behavior matches the spec.  See step 3 here:

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

It does not update all ServiceWorker objects before firing any "statechange" events.  Each instance is updated and fires its "statechange" independently.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8946777 [details] [diff] [review]
P4 Make ServiceWorker operate on an abstract Inner interface that ServiceWorkerInfo implements. r=asuth

This patch makes ServiceWorker use an abstract interface instead of ServiceWorkerInfo directly.  This will be where we cut over to the remote implemention instead of the same-process info object.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8946872 [details] [diff] [review]
P5 Support caching the ServiceWorker DOM instance on the global. r=asuth

One of the things ServiceWorkerInfo provides is the GetOrCreateInstance() method.  This is how we always return the same ServiceWorker object for the same backing version of the worker.  Obviously we can't use this in a remote world.

To avoid the problem we'll go back to storing the cached list on the global.  (We used to store it on the inner window.)

This patch adds methods to nsIGlobalObject to allow the ServiceWorker to manage its weak reference on the global.  It also adds a GetOrCreateServiceWorker() method that can be used to de-duplicate the DOM objects.

While we only implement this stuff on window for now I added it to nsIGlobalObject to make it easier to add ServiceWorker to worker contexts in the future.  I want to write as much thread-agnostic code as I can here.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8946873 [details] [diff] [review]
P6 Make ServiceWorker call nsIGlobalObject::AddServiceWorker and RemoveServiceWorker. r=asuth

This make ServiceWorker call the global's methods to manage the weak reference.  Note, we cannot remove the calls to the inner object.

We need those as well so the inner object can find us when it wants to change the state, etc.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8946875 [details] [diff] [review]
P7 Use the global to GetOrCreate the ServiceWorker DOM instance. r=asuth

Finally, this removes the ServiceWorkerInfo::GetOrCreateInstance() method and uses the new global GetOrCreateServiceWorker() method instead.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8946717 [details] [diff] [review]
P3 Make each ServiceWorker DOM object automatically fire its statechange event when appropriate. r=asuth

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

Comment 12 covers this really well, thank you.  (Although anyone reading should s/not/now/ in the phrase "can not be slightly out of sync".)

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8946872 [details] [diff] [review]
P5 Support caching the ServiceWorker DOM instance on the global. r=asuth

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

Restating context: This is what you discussed with :bz at https://mozilla.logbot.info/content/20180124#c14185360 and having me review was authorized by :bz at https://mozilla.logbot.info/content/20180130#c14215961.

Restating:
- ServiceWorkerInfo currently tracks the ServiceWorker binding instances created on a per-global basis so that we don't create duplicate instances.  But ServiceWorkerInfo is on the authoritative parent-process side of the split, so it can't do that anymore.  nsIGlobalObject is the most natural place to stash the per-global bindings, so we're doing that.
- We're using a list (using ServiceWorker::MatchesDescriptor as the key-equivalence test) because a map isn't necessary; we expect a small number of bindings to exist at a time, and even on sites that use prodigious numbers of scopes and invoke getRegistrations() a lot, ServiceWorkerContainer::GetController() and ServiceWorkerRegistrationMainThread::Get{Installing,Waiting,Active} both cache their bindings.  (ClientSource::PostMessage does not, but the postMessage overhead cost dominates, etc.)

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8946873 [details] [diff] [review]
P6 Make ServiceWorker call nsIGlobalObject::AddServiceWorker and RemoveServiceWorker. r=asuth

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

::: dom/serviceworkers/ServiceWorker.cpp
@@ +86,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    mInner->RemoveServiceWorker(this);
> +  nsIGlobalObject* global = GetParentObject();
> +  if (global) {

Restating life-cycle: GetParentObject() only returns null if DOMEventTargetHelper::DisconnectFromOwner() was invoked.  That only happens if ServiceWorker::DisconnectFromOwner() was invoked and called the superclass DETH's implementation.  And so RemoveServiceWorker(this) is appropriately invoked in both cases.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8946875 [details] [diff] [review]
P7 Use the global to GetOrCreate the ServiceWorker DOM instance. r=asuth

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

::: dom/serviceworkers/ServiceWorkerManager.cpp
@@ +2368,5 @@
>      return NS_ERROR_DOM_NOT_FOUND_ERR;
>    }
>  
> +  RefPtr<ServiceWorker> serviceWorker =
> +    aWindow->GetOrCreateServiceWorker(info->Descriptor());

Restating: This change covers the ServiceWorkerRegistrationMainThread::Get{Installing,Waiting,Active} callers indirectly exposed to content, which is why there are no changes to ServiceWorkerRegistration in this patch-set.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add ServiceWorker::Create() factory method. r=asuth

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Make ServiceWorker store and use a ServiceWorkerDescriptor internally. r=asuth

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make each ServiceWorker DOM object automatically fire its statechange event when appropriate. r=asuth

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Make ServiceWorker operate on an abstract Inner interface that ServiceWorkerInfo implements. r=asuth

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Support caching the ServiceWorker DOM instance on the global. r=asuth

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Make ServiceWorker call nsIGlobalObject::AddServiceWorker and RemoveServiceWorker. r=asuth

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Use the global to GetOrCreate the ServiceWorker DOM instance. r=asuth

Rebased the patches around a small change I had to make to P1.  I forgot to `return ref.forget()` and instead had just `ref.forget()`.  Thankfully this made static analysis blow up.  Easy fix and was in a branch that should not have been hit anyway.

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a246a99f932b
P1 Add ServiceWorker::Create() factory method. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb38a0b7b6dd
P2 Make ServiceWorker store and use a ServiceWorkerDescriptor internally. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6444014a8f90
P3 Make each ServiceWorker DOM object automatically fire its statechange event when appropriate. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cf205e9e3b0
P4 Make ServiceWorker operate on an abstract Inner interface that ServiceWorkerInfo implements. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/edfc00999a3a
P5 Support caching the ServiceWorker DOM instance on the global. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b5ed759f753
P6 Make ServiceWorker call nsIGlobalObject::AddServiceWorker and RemoveServiceWorker. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5241785aca
P7 Use the global to GetOrCreate the ServiceWorker DOM instance. r=asuth

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6776907cc2aa
Follow-up to fix bustage on a CLOSED TREE. r=me

Comment 30

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/a246a99f932b
https://hg.mozilla.org/mozilla-central/rev/bb38a0b7b6dd
https://hg.mozilla.org/mozilla-central/rev/6444014a8f90
https://hg.mozilla.org/mozilla-central/rev/1cf205e9e3b0
https://hg.mozilla.org/mozilla-central/rev/edfc00999a3a
https://hg.mozilla.org/mozilla-central/rev/6b5ed759f753
https://hg.mozilla.org/mozilla-central/rev/cd5241785aca
https://hg.mozilla.org/mozilla-central/rev/6776907cc2aa