Check for ServiceWorker updates at reasonable intervals.

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: nsm, Assigned: Ehsan)

Tracking

33 Branch
mozilla40
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

12.02 KB, patch
nsm
: review+
Details | Diff | Splinter Review
7.01 KB, patch
nsm
: review+
Details | Diff | Splinter Review
8.95 KB, patch
nsm
: review+
Details | Diff | Splinter Review
10.16 KB, patch
nsm
: review+
Details | Diff | Splinter Review
7.38 KB, patch
nsm
: review+
Details | Diff | Splinter Review
Is this going to be configurable via pref?
It can be if you implement it ;)

On a serious note, yes we can make it so.
Assignee: nobody → ehsan
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?
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(nsm.nikhil)
I see, OK.  I can ignore this issue for now in my tests, just wanted to make sure that I'm not completely crazy!  :-)
I almost have a fix for the issue in comment 3.
Flags: needinfo?(amarchesini)
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?
Attachment #8592235 - Flags: feedback?(nsm.nikhil)
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?
Attachment #8592230 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8592231 - Flags: review?(nsm.nikhil)
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");
}
Attachment #8592235 - Flags: feedback?(nsm.nikhil)
(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 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...
Attachment #8592938 - Attachment is obsolete: true
Attachment #8592938 - Flags: review?(nsm.nikhil)
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?
Flags: needinfo?(mrbkap)
(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.
Flags: needinfo?(mrbkap)
(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.
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.
Attachment #8593393 - Flags: review?(nsm.nikhil)
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.
Attachment #8593406 - Flags: review?(nsm.nikhil)
This is much better than testing just the
nsIServiceWorkerManager::UpdateAllRegistrations API.
Attachment #8593500 - Flags: review?(nsm.nikhil)
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.
This finally happened on a debug build!  Based on the log <http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/eakhgari@mozilla.com-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.
See Also: → 1151974
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.
(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 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.
Attachment #8593393 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8593004 - Flags: review?(nsm.nikhil) → review+
Attachment #8593406 - Flags: review?(nsm.nikhil) → review+
(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 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!
Attachment #8593500 - Flags: review?(nsm.nikhil) → review+
(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..
Follow-ups filed with patches: bug 1155987, bug 1155988 and bug 1155989.
Depends on: 1155987, 1155988, 1155989
See Also: → 1156092
You need to log in before you can comment on or make changes to this bug.