1112469 - Check for ServiceWorker updates at reasonable intervals.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Comment 1

[Francisco Jordano [:arcturus] [:francisco]]

Is this going to be configurable via pref?

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

It can be if you implement it ;)

On a serious note, yes we can make it so.

Comment 3

[(no longer active)]

I'm trying to test my patches here but something is puzzling me.  It seems that with bug 931249, once you register a SW, we download the source for the script twice, once from ScriptLoader and once from CompareNetwork.  Similarly, we also download the script twice when we try to update the service worker.

This seems wrong to me.  baku/nsm: is this desired and/or intentional?

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

This bug also affects Bug 1134329. The ideal fix would be to have the comparison hook into ScriptLoader so we only had to download once. That said, ScriptLoader is complicated code and we'd have to think of a clean API. One option is to have the ScriptLoader always perform equality checks for ServiceWorkers when some custom workers::LoadInfo flag is set, then remove the comparison from ServiceWorkerManager.

Comment 5

[(no longer active)]

I see, OK.  I can ignore this issue for now in my tests, just wanted to make sure that I'm not completely crazy!  :-)

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I almost have a fix for the issue in comment 3.

Comment 7

[(no longer active)]

Attachment: Part 1: Implement an XPCOM service responsible to trigger daily updates of service workers

Comment 8

[(no longer active)]

Attachment: Part 2: Update the service workers in the parent and all child processes every day

Comment 9

[(no longer active)]

Attachment: Part 3: Add a unit test for ensuring that the nsIServiceWorkerManager.updateAllRegistrations() API works correctly

So this test doesn't work yet, and both of the checks in it fail.

It seems that by the time that we receive the statechange event that says that the worker is activated, the old version of the worker is still being used.

I have some debugging dumps in place that show that the new version of the SW is indeed downloaded and being installed, and as far as I can tell, I'm using the SW API correctly, but it seems that this scenario is untested right now, so I can't decide whether I'm doing something wrong, or whether the update scenario doesn't work at all.  Nikhil, do you have any ideas?

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8592230 [details] [diff] [review]
Part 1: Implement an XPCOM service responsible to trigger daily updates of service workers

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

::: dom/workers/ServiceWorkerPeriodicUpdater.cpp
@@ +17,5 @@
> +
> +StaticRefPtr<ServiceWorkerPeriodicUpdater>
> +ServiceWorkerPeriodicUpdater::sInstance;
> +
> +already_AddRefed<ServiceWorkerPeriodicUpdater>

If we are guaranteed that Updater is always initialized and due to the use of the static ptr will survive till the end, this could just return a rawptr right?

::: dom/workers/ServiceWorkerPeriodicUpdater.h
@@ +20,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIOBSERVER
> +
> +  ServiceWorkerPeriodicUpdater();

private?

Comment 11

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8592231 [details] [diff] [review]
Part 2: Update the service workers in the parent and all child processes every day

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

Please add a comment to ServiceWorkerPeriodicUpdater.h that explains which processes the cpp and js run in.

Is app-startup guaranteed to happen before idle-daily if idle-daily is about to be sent on startup? If idle-daily can be sent on startup, we may not have the registrations loaded from disk by that time, in which case you may want to flip a boolean if mActor is not available and then when the actor is created, run UpdateAll if the boolean is set.

Dropping review for using Registrar directly.

::: dom/workers/ServiceWorkerManager.cpp
@@ +3112,5 @@
> +ServiceWorkerManager::UpdateAllRegistrations()
> +{
> +  AssertIsOnMainThread();
> +
> +  nsRefPtr<ServiceWorkerRegistrar> swr = ServiceWorkerRegistrar::Get();

This should probably use the in-memory registrations.

::: dom/workers/ServiceWorkerPeriodicUpdater.cpp
@@ +63,5 @@
> +      nullObj.setUndefined();
> +      nullObjects.setUndefined();
> +      DebugOnly<nsresult> rv =
> +        messageBroadcaster->BroadcastAsyncMessage(NS_LITERAL_STRING("periodicserviceworkerupdate"),
> +                                                  nullObj, nullObjects,

You can just use JS::UndefinedHandleValue here and get rid of the jsapi cruft above.

@@ +65,5 @@
> +      DebugOnly<nsresult> rv =
> +        messageBroadcaster->BroadcastAsyncMessage(NS_LITERAL_STRING("periodicserviceworkerupdate"),
> +                                                  nullObj, nullObjects,
> +                                                  jsAPI.cx(), 1);
> +      NS_WARN_IF(NS_FAILED(rv));

Last I know this doesn't compile on windows and needs to be
if (NS_WARN_IF(NS_FAILED(rv)) {
}

but great if we fixed it.

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8592235 [details] [diff] [review]
Part 3: Add a unit test for ensuring that the nsIServiceWorkerManager.updateAllRegistrations() API works correctly

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

Discussed this with ehsan, he will upload a new patch.

::: dom/workers/test/serviceworkers/periodic/unregister.html
@@ +3,5 @@
> +  navigator.serviceWorker.getRegistration(".").then(function(registration) {
> +    registration.unregister().then(function(success) {
> +      if (success) {
> +        parent.callback();
> +      }

else {
  dump("Unregister failed\n");
}

Comment 13

[(no longer active)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #10)
> ::: dom/workers/ServiceWorkerPeriodicUpdater.cpp
> @@ +17,5 @@
> > +
> > +StaticRefPtr<ServiceWorkerPeriodicUpdater>
> > +ServiceWorkerPeriodicUpdater::sInstance;
> > +
> > +already_AddRefed<ServiceWorkerPeriodicUpdater>
> 
> If we are guaranteed that Updater is always initialized and due to the use
> of the static ptr will survive till the end, this could just return a rawptr
> right?

I suppose so, but unless you care strongly about this, I prefer to keep it as is since I see no downside.

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #11)
> Is app-startup guaranteed to happen before idle-daily if idle-daily is about
> to be sent on startup?

Yes.

> ::: dom/workers/ServiceWorkerPeriodicUpdater.cpp
> @@ +63,5 @@
> > +      nullObj.setUndefined();
> > +      nullObjects.setUndefined();
> > +      DebugOnly<nsresult> rv =
> > +        messageBroadcaster->BroadcastAsyncMessage(NS_LITERAL_STRING("periodicserviceworkerupdate"),
> > +                                                  nullObj, nullObjects,
> 
> You can just use JS::UndefinedHandleValue here and get rid of the jsapi
> cruft above.

Awesome!

> @@ +65,5 @@
> > +      DebugOnly<nsresult> rv =
> > +        messageBroadcaster->BroadcastAsyncMessage(NS_LITERAL_STRING("periodicserviceworkerupdate"),
> > +                                                  nullObj, nullObjects,
> > +                                                  jsAPI.cx(), 1);
> > +      NS_WARN_IF(NS_FAILED(rv));
> 
> Last I know this doesn't compile on windows and needs to be
> if (NS_WARN_IF(NS_FAILED(rv)) {
> }
> 
> but great if we fixed it.

Thanks for the warning, I'll double check this builds on try.

Comment 14

[(no longer active)]

Attachment: Part 2: Update the service workers in the parent and all child processes every day

Comment 15

[(no longer active)]

Attachment: Part 3: Add a unit test for ensuring that the nsIServiceWorkerManager.updateAllRegistrations() API works correctly

Comment 16

[(no longer active)]

Attachment: Part 3: Add a unit test for ensuring that the nsIServiceWorkerManager.updateAllRegistrations() API works correctly

Comment 17

[(no longer active)]

Comment on attachment 8592938 [details] [diff] [review]
Part 2: Update the service workers in the parent and all child processes every day

Not quite ready yet...

Comment 18

[(no longer active)]

There's one issue with e10s which I haven't figured out yet.  Right now I try to inject a framescript on app startup (please see ServiceWorkerPeriodicUpdater::Observe in the obsoleted part 2 patch), but if you don't have a content process at that time (for example after you've crashed where we only show about:sessionrestore which is loaded in the parent), the framescript doesn't get injected.

Is there a better hook for injecting a frame script into all content processes no matter how/when we start them?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #18)
> There's one issue with e10s which I haven't figured out yet.  Right now I
> try to inject a framescript on app startup (please see
> ServiceWorkerPeriodicUpdater::Observe in the obsoleted part 2 patch), but if
> you don't have a content process at that time (for example after you've
> crashed where we only show about:sessionrestore which is loaded in the
> parent), the framescript doesn't get injected.

This sounds a lot like bug 1141661 which I plan on very soon. Delayed frame scripts would be the way to do this...

That being said, I don't think you need the indirection of the frame script in this case. You could, instead, add a notification to PContent.ipdl and iterate over all ContentParents calling your notification function on each of them and handling them in ContentChild. That could then stay in C++ calling into the actual implementation. See all of the code related to SendBidiKeyboardNotify for some example code that broadcasts a message (in C++) to all processes.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #19)
> This sounds a lot like bug 1141661 which I plan on very soon. Delayed frame

plan on *fixing* very soon.

Comment 21

[(no longer active)]

Attachment: Part 2: Update the service workers in the parent and all child processes every day

Sorry for the long delay, testing this took much longer than I would have liked since a lot of the testing needed to be done manually, but this works very well now in both e10s and non-e10s.

Comment 22

[(no longer active)]

Attachment: Part 4: Disable periodic updates of service workers when running tests

This is meant to avoid random issues with Gecko suddenly kicking off the
periodic updates in the middle of our test runs.  This uses a hidden pref
intentionally since nobody should be able to discover this pref by going
to about:config.

Comment 23

[(no longer active)]

Attachment: Part 5: Actually test the code path that handles the idle-daily message

This is much better than testing just the
nsIServiceWorkerManager::UpdateAllRegistrations API.

Comment 24

[(no longer active)]

I've been running this on try, and there is an intermittent test failure on Windows <https://treeherder.mozilla.org/#/jobs?repo=try&revision=f99d6761b81f>.  I haven't figured it out yet, but I'm planning to land with the test disabled on Windows for now if I don't understand what's happening here by tomorrow.

Comment 25

[(no longer active)]

This finally happened on a debug build!  Based on the log <http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f99d6761b81f/try-win64-debug/try_win8_64-debug_test-mochitest-4-bm110-tests1-windows-build434.txt.gz>, the test failure seems to be caused by the same underlying reason as bug 1151974 (which is, the cache CreateDBSchema function fails, presumably as we're trying to store the downloaded SW script in the cache.

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

I have a patch for bug 1151974 mostly done.  Just doing some overnight try builds to verify it triggers the condition I think it does.

Comment 27

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #26)
> I have a patch for bug 1151974 mostly done.  Just doing some overnight try
> builds to verify it triggers the condition I think it does.

Awesome, thank you!  :-)

Comment 28

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8593393 [details] [diff] [review]
Part 2: Update the service workers in the parent and all child processes every day

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +3115,5 @@
> +  auto This = static_cast<ServiceWorkerManager*>(aUserArg);
> +  MOZ_ASSERT(!aInfo->mScope.IsEmpty());
> +  nsresult res = This->Update(NS_ConvertUTF8toUTF16(aInfo->mScope));
> +  if (NS_WARN_IF(NS_FAILED(res))) {
> +  }

Nit: I like to add a comment in the block body stating the block is just to satisfy windows compilers.

Comment 29

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8593004 [details] [diff] [review]
Part 3: Add a unit test for ensuring that the nsIServiceWorkerManager.updateAllRegistrations() API works correctly

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

::: dom/workers/test/serviceworkers/periodic/register.html
@@ +7,5 @@
> +      isDone = true;
> +    }
> +  }
> +
> +  navigator.serviceWorker.register("../periodic.sjs", {scope: "."})

Have you considered the following form rather than all the trickery in the promise callback?
navigator.serviceWorker.ready.then(done);
navigator.serviceWorker.register(...);

When the register() leads to active, the ready promise will resolve and you can fire done. You don't *have* to make the change, just a suggestion.

::: dom/workers/test/serviceworkers/periodic/unregister.html
@@ +7,5 @@
> +      } else {
> +        dump("Unregister failed\n");
> +      }
> +    });
> +  });

Please add a error branch for the promise that also logs to dump(). Lack of that is a major barrier to analyzing failing tests.

Comment 30

[(no longer active)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #28)
> Comment on attachment 8593393 [details] [diff] [review]
> Part 2: Update the service workers in the parent and all child processes
> every day
> 
> Review of attachment 8593393 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +3115,5 @@
> > +  auto This = static_cast<ServiceWorkerManager*>(aUserArg);
> > +  MOZ_ASSERT(!aInfo->mScope.IsEmpty());
> > +  nsresult res = This->Update(NS_ConvertUTF8toUTF16(aInfo->mScope));
> > +  if (NS_WARN_IF(NS_FAILED(res))) {
> > +  }
> 
> Nit: I like to add a comment in the block body stating the block is just to
> satisfy windows compilers.

It's not just Windows.  I also got an error when building opt on Mac.  Still want me to add a comment?

Comment 31

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8593500 [details] [diff] [review]
Part 5: Actually test the code path that handles the idle-daily message

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

Thanks for the excellent set of patches! I didn't know about idle-daily before and was going to concoct a more complex solution.
Is this blocked on the hit-network-twice bug? Otherwise, ship it!

Comment 32

[(no longer active)]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #29)
> ::: dom/workers/test/serviceworkers/periodic/register.html
> @@ +7,5 @@
> > +      isDone = true;
> > +    }
> > +  }
> > +
> > +  navigator.serviceWorker.register("../periodic.sjs", {scope: "."})
> 
> Have you considered the following form rather than all the trickery in the
> promise callback?
> navigator.serviceWorker.ready.then(done);
> navigator.serviceWorker.register(...);
> 
> When the register() leads to active, the ready promise will resolve and you
> can fire done. You don't *have* to make the change, just a suggestion.

This is a good suggestion!  But this pattern is prevalent in our tests right now, I'll fix it all in a follow-up.

> ::: dom/workers/test/serviceworkers/periodic/unregister.html
> @@ +7,5 @@
> > +      } else {
> > +        dump("Unregister failed\n");
> > +      }
> > +    });
> > +  });
> 
> Please add a error branch for the promise that also logs to dump(). Lack of
> that is a major barrier to analyzing failing tests.

I'll also do this in a follow-up, since we have a bunch of these sprayed everywhere now..

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/986bcbc1da69
https://hg.mozilla.org/integration/mozilla-inbound/rev/0de78a468bca
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdfca48c0862
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fa52c096268
https://hg.mozilla.org/integration/mozilla-inbound/rev/64fac0349710

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/efc47b3755ca

Comment 35

[(no longer active)]

Follow-ups filed with patches: bug 1155987, bug 1155988 and bug 1155989.

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/986bcbc1da69
https://hg.mozilla.org/mozilla-central/rev/0de78a468bca
https://hg.mozilla.org/mozilla-central/rev/bdfca48c0862
https://hg.mozilla.org/mozilla-central/rev/2fa52c096268
https://hg.mozilla.org/mozilla-central/rev/64fac0349710
https://hg.mozilla.org/mozilla-central/rev/efc47b3755ca