1220740 - nsIServiceWorkerRegistrationInfo should emit an event when its state changes.

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Eddy Bruel [:ejpbruel]]

For service worker debugging, we currently don't have a way to tell when the installing/waiting/active worker for a given nsIServiceWorkerRegistrationInfo changes.

To fix this, we plan to extend nsIServiceWorkerRegistrationInfo with a listener object that will be called whenever its installing/waiting/active worker changes.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Work in progress

I have a patch that's almost ready, but before I put it up for review, I have a couple of questions:

First off, would you please double check for me that we are calling NotifyListenersOnChange at the appropriate times? I’d like to make sure that we do not end up calling the listener while the service workers are in an inconsistent state. 

I'm wondering if it's safe to use promises in the test the way I did. Since promises are asynchronous, it's theoretically possible for other events to be handled between the time that the onChange handler is called, and the then-handlers for the resolved promise to be called.

With the patch in its current form, is it possible for a call to NotifyListenersOnChange to happen within the same tick of the event loop as a previous call? If so, then the test is racy in it's current form: by the time the then-handlers of the resolved promise are called, the value of installing/waiting/activeWorker may have changed again, so we cannot reliably assert that they have any definite value.

If this is not the case, and calls to NotifyListenersOnChange always happen on different ticks on the event loop, are the then-handlers scheduled as a result of the promise being resolved guaranteed to be handled before any events that are scheduled afterwards? I’m not sure how this works, since promises apparently use micro tasks instead of the normal event queue.

If this is not the case, and events scheduled after then-handlers can be handled before those handlers, then again the test is racy in its current form, for similar reasons.

The test currently checks that we start out with installing/waiting/activeWorker all set to null. It then expects the onChange handler to be called when the service worker starts installing, and again when it becomes active. I then update the worker to make sure the onChange handler is called properly in those cases as well.

What I’m not testing for at the moment is that the onChange handler is called properly when the service worker finishes installing, but needs to wait for the active worker to finish. I’d like to add this to the test, but I’m unsure how to implement it:

My idea was to change serviceworkerregistrationinfo_iframe.html so that instead of waiting on the promise returned by navigator.serviceWorker.register, we wait on the navigator.serviceWorker.ready promise. When that promise resolves, I expected navigator.serviceWorker.controller to be non-null, so I can send the service worker a message, telling it to extend its lifetime.

Apparently, this doesn’t work, since navigator.serviceWorker.controller is still null when the ready promise resolves. Am I doing it wrong? Am I misunderstanding how this works? What would be the easiest way to extend the lifetime of the service worker in this particular test?

Comment 2

[Cătălin Badea (:catalinb)]

I'm going to have a look at your patch first thing tomorrow, it's getting really late here, sorry.

Comment 3

[Cătălin Badea (:catalinb)]

(In reply to Eddy Bruel [:ejpbruel] from comment #1)
> Created attachment 8682998 [details] [diff] [review]
> Work in progress
> 
> I have a patch that's almost ready, but before I put it up for review, I
> have a couple of questions:
> 
> First off, would you please double check for me that we are calling
> NotifyListenersOnChange at the appropriate times? I’d like to make sure that
> we do not end up calling the listener while the service workers are in an
> inconsistent state.
Yes, it looks right to me. However, I wonder if ServiceWorker.onstatechange could
be used instead of adding a new listener.
> 
> I'm wondering if it's safe to use promises in the test the way I did. Since
> promises are asynchronous, it's theoretically possible for other events to
> be handled between the time that the onChange handler is called, and the
> then-handlers for the resolved promise to be called.
> 
> With the patch in its current form, is it possible for a call to
> NotifyListenersOnChange to happen within the same tick of the event loop as
> a previous call? If so, then the test is racy in it's current form: by the
> time the then-handlers of the resolved promise are called, the value of
> installing/waiting/activeWorker may have changed again, so we cannot
> reliably assert that they have any definite value.
I think I made a mistake in one of my emails to you, Activate() may be called
synchronously after the worker transitioned to the waiting state if there isn't
an active worker or the skipWaiting flag was set, which should be the case in
your test. Oh, but you don't check the waiting state being non null.
> 
> If this is not the case, and calls to NotifyListenersOnChange always happen
> on different ticks on the event loop, are the then-handlers scheduled as a
> result of the promise being resolved guaranteed to be handled before any
> events that are scheduled afterwards? I’m not sure how this works, since
> promises apparently use micro tasks instead of the normal event queue.
I don't know either. bz, do you know how this works?

> If this is not the case, and events scheduled after then-handlers can be
> handled before those handlers, then again the test is racy in its current
> form, for similar reasons.
> 
> The test currently checks that we start out with
> installing/waiting/activeWorker all set to null. It then expects the
> onChange handler to be called when the service worker starts installing, and
> again when it becomes active. I then update the worker to make sure the
> onChange handler is called properly in those cases as well.
> 
> What I’m not testing for at the moment is that the onChange handler is
> called properly when the service worker finishes installing, but needs to
> wait for the active worker to finish. I’d like to add this to the test, but
> I’m unsure how to implement it:
> 
> My idea was to change serviceworkerregistrationinfo_iframe.html so that
> instead of waiting on the promise returned by
> navigator.serviceWorker.register, we wait on the
> navigator.serviceWorker.ready promise. When that promise resolves, I
> expected navigator.serviceWorker.controller to be non-null, so I can send
> the service worker a message, telling it to extend its lifetime.
> 
> Apparently, this doesn’t work, since navigator.serviceWorker.controller is
> still null when the ready promise resolves. Am I doing it wrong? 
This is happens because the worker doesn't control pages that were opened before
it became active. There are two options you can take:
1. open an iframe after the first worker is active and you'll have a controlled window.
2. call clients.claim() in the onactivate handler of the worker, this will cause all
current pages that match the worker's scope to be controlled by it.
> Am I
> misunderstanding how this works? What would be the easiest way to extend the
> lifetime of the service worker in this particular test?
Note that when replacing an active worker with the waiting one, we only care about whether it
is controlling documents or if the skipWaiting flag was set. The lifetime timers don't matter
in this case.
So what you need is to have the active worker controlling at least 1 document when you update.

Comment 4

[Cătălin Badea (:catalinb)]

Comment on attachment 8682998 [details] [diff] [review]
Work in progress

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

So I think ServiceWorkerRegistration.onupdatefound and ServiceWorker.onstatechange might be used
to the same effect, but I'm not sure. I guess this will make things easier.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1353,5 @@
>      }
>  
>      mRegistration->mWaitingWorker = mRegistration->mInstallingWorker.forget();
>      mRegistration->mWaitingWorker->UpdateState(ServiceWorkerState::Installed);
> +    mRegistration->NotifyListenersOnChange();

This call will happen in the same runnable as the one for the active worker.

::: dom/workers/test/serviceworkers/test_serviceworkerregistrationinfo.xul
@@ +29,5 @@
> +
> +          promise = waitForRegister(EXAMPLE_URL);
> +          iframe.contentWindow.postMessage("register", "*");
> +          let registration = yield promise;
> +          ok(registration.installingWorker === null);

Shouldn't installing be non null here?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I don't know either. bz, do you know how this works?

When we are processing runnables, after every one we process any pending promise stuff.  Does that answer the question, or am I misunderstanding it?

Comment 6

[Eddy Bruel [:ejpbruel]]

As it turns out, when registering a service worker with a scope for which we already have a registration, the existing object is updated in place. I'm going to extend the scope of this bug to send a notification whenever the state of a registration changes (not just it's workers).

Comment 7

[Eddy Bruel [:ejpbruel]]

Attachment: nsIServiceWorkerRegistrationInfo should emit an event when its scriptSpec property changes.

Based on our discussion on how ServiceWorkerRegistrationInfos are updated in place when a new service worker is registered for an existing scope, I'm going to implement this in two steps.

This first patch refactors ServiceWorkerRegistrationInfo to notify its listeners when its scriptSpec property changes. This will allow us to update the debugger UI for this particular registration, even though the ServiceWorkerManager didn't notify its listeners (this is expected, since technically the *list* of registrations didn't change).

This patch does not yet address notifying the listeners when the worker properties of the registration change. The second patch will take care of that.

Comment 8

[Eddy Bruel [:ejpbruel]]

Attachment: nsIServiceWorkerRegistrationInfo should emit an event when its scriptSpec property changes.

Comment 9

[Eddy Bruel [:ejpbruel]]

> Yes, it looks right to me. However, I wonder if ServiceWorker.onstatechange
> could
> be used instead of adding a new listener.

I forgot to point out that onstatechange lives on ServiceWorker, not on ServiceWorkerRegistrationInfo, where we need it. Moreover this event is only called when the workers change, not when the registration's scriptSpec is updated. It seems to me that it would be easier to just add a listener that exactly meets our needs, rather than shoehorn onstatechange into this role.

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: nsIServiceWorkerRegistrationInfo should emit an event when its worker properties change.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8684891 [details] [diff] [review]
nsIServiceWorkerRegistrationInfo should emit an event when its scriptSpec property changes.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +438,5 @@
> +NS_IMETHODIMP
> +ServiceWorkerRegistrationInfo::AddListener(
> +                            nsIServiceWorkerRegistrationInfoListener *aListener)
> +{
> +  AssertIsOnMainThread();

MOZ_ASSERT(aListener);
MOZ_ASSERT(!mListeners.Contains(aListener));

@@ +447,5 @@
> +}
> +
> +NS_IMETHODIMP
> +ServiceWorkerRegistrationInfo::RemoveListener(
> +                            nsIServiceWorkerRegistrationInfoListener *aListener)

MOZ_ASSERT(mListeners.Contains(aListener);

@@ +2494,5 @@
>  }
>  
>  void
> +ServiceWorkerRegistrationInfo::NotifyListenersOnChange()
> +{

AssertIsMainThread();

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8684929 [details] [diff] [review]
nsIServiceWorkerRegistrationInfo should emit an event when its worker properties change.

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +1679,5 @@
>    swm->InvalidateServiceWorkerRegistrationWorker(this, WhichServiceWorker::WAITING_WORKER);
>  
>    mActiveWorker = activatingWorker.forget();
>    mWaitingWorker = nullptr;
> +  NotifyListenersOnChange();

I guess you want to move it after the next line. Don't you?

Comment 13

[Eddy Bruel [:ejpbruel]]

Try push for the first patch, with comments by baku addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5559bb0807e1

Comment 14

[Eddy Bruel [:ejpbruel]]

Try push for the second patch, with comment by baku addressed:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a969cd44e24

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b427b61512da
https://hg.mozilla.org/integration/mozilla-inbound/rev/781a1dfd83e4

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b427b61512da
https://hg.mozilla.org/mozilla-central/rev/781a1dfd83e4