1156092 - Implement periodic service worker updates for B2G

Status: RESOLVED WONTFIX

(Core :: DOM: Service Workers, defect)

Description

[(no longer active)]

Bug 1112469 implemented the periodic service worker updates for desktop and Android.

This doesn't yet work on b2g since we cannot depend on the app content process to be running when the idle-daily event fires, so we'll need another approach.  I don't know how to make some code start the right content process for an app, which is needed to make sure that the SW will get updated in the correct process.

Fabrice, can you please help with this, or help us find a good person to work on this?  Thanks!

Comment 1

[Andrea Marchesini [:baku]]

As far as I know, we already have this problem with the Push notification and the RequestSync API. And we will have the same issue with the BackgroundSync API.
I think for now we can open the app in background and close it after the update (if the use didn't make it visible). Then we can find a generic solution that can work for all the existing APIs.

How does it sound?

Comment 2

[[:fabrice] Fabrice Desré]

Even on b2g you don't necessarily need a new process to run as an app. Just creating an <iframe mozapp=$manifestURL> is enough.

I tend to think that not involving gaia would make things simpler, and one option there is to create these iframes in shell.html (either remote or not).

Is it enough to load a well-known url to trigger the service worker update? how do we know that the update completed?

Comment 3

[(no longer active)]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Even on b2g you don't necessarily need a new process to run as an app. Just
> creating an <iframe mozapp=$manifestURL> is enough.

How well does that play with the same app being loaded normally at that time?

> I tend to think that not involving gaia would make things simpler, and one
> option there is to create these iframes in shell.html (either remote or not).

Sounds good to me, if the answer to the above question is yes.

> Is it enough to load a well-known url to trigger the service worker update?

No.  We'd need to do that programmatically.

> how do we know that the update completed?

Well, it depends on how you do it.  For example, if you just use the Web exposed SW API, you can use the updatefound event.  See the test for bug 1112469.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #3)
> (In reply to Fabrice Desré [:fabrice] from comment #2)
> > Even on b2g you don't necessarily need a new process to run as an app. Just
> > creating an <iframe mozapp=$manifestURL> is enough.
> 
> How well does that play with the same app being loaded normally at that time?

There should be no difference with "normal" pages, so I think it's fine. But I'm also sure this is unchartered territory ;)

Comment 5

[Andrea Marchesini [:baku]]

Maybe this is totally fine but, what about if we start the updating of a SW as fabrice is suggesting and then the user opens the same app? I guess we should keep the previous SW until the updating operation is fully completed.

Comment 6

[(no longer active)]

(In reply to Andrea Marchesini (:baku) from comment #5)
> Maybe this is totally fine but, what about if we start the updating of a SW
> as fabrice is suggesting and then the user opens the same app? I guess we
> should keep the previous SW until the updating operation is fully completed.

That should be taken care of by the normal SW lifecycle, right?

Comment 7

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> (In reply to Andrea Marchesini (:baku) from comment #5)
> > Maybe this is totally fine but, what about if we start the updating of a SW
> > as fabrice is suggesting and then the user opens the same app? I guess we
> > should keep the previous SW until the updating operation is fully completed.
> 
> That should be taken care of by the normal SW lifecycle, right?

Yes

Comment 8

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

Couple of things:
Since the Cache API is used for SW scripts, we can't run the update in the parent process, nor can we run it in an arbitrary child (<browser remote="true"> launches a new child). We *have* to run this in a child that has the same principal/origin/appid/isInBrowser and other identifiers. Unfortunately our process launching and window management code is hairy in both desktop and b2g, so any ideas about how to expose nicer APIs for them are welcome. For example, I have no idea how to launch such a process as I described before, inject a frame script into it and have the child run the ServiceWorkerManager.

That said, I'm removing this as a v1 blocker because we aren't shipping b2g any time soon and we shouldn't block shipping SW in 41 due to this.

Comment 9

[Noemí Freire (:noemi)]

Let's block then v2 and see how these b2g blocking bugs can be prioritized. Thanks!

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Nikhil Marathe [:nsm] (back on July 13) from comment #8)
> Couple of things:
> Since the Cache API is used for SW scripts, we can't run the update in the
> parent process, nor can we run it in an arbitrary child (<browser
> remote="true"> launches a new child). We *have* to run this in a child that
> has the same principal/origin/appid/isInBrowser and other identifiers.
> Unfortunately our process launching and window management code is hairy in
> both desktop and b2g, so any ideas about how to expose nicer APIs for them
> are welcome. For example, I have no idea how to launch such a process as I
> described before, inject a frame script into it and have the child run the
> ServiceWorkerManager.
> 

I can confirm that this requirements is true. This is actually happening right now with our implementation of about:sw in the Settings app. When we request an update for a sw from the Settings app, it only gets updated when the application owning that sw is running. I'll be fixing this at bug 1181544
> That said, I'm removing this as a v1 blocker because we aren't shipping b2g
> any time soon and we shouldn't block shipping SW in 41 due to this.

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: Allow chrome registrations on RequestSyncService

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: Allow chrome registrations on RequestSyncService

Andrea, could you take a look at this patch, please?

This allows chrome requestSync registrations. The notifications are done via the Observer API instead of SystemMessages as we do for content registrations.

I'll be adding a test in another patch.

Thanks!

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Schedule periodic SW updates

This patch makes use of the RequestSyncService to schedule periodic sw updates on B2G.

I still need to test this more and write some comments before asking for revision.

Unfortunately, this depends on bug 1181544 which also depends on bug 1182117 to work in all cases (i.e. when the owner of the sw being updated is not opened).

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8637201 [details] [diff] [review]
Part 2: Schedule periodic SW updates

Andrea, I could use some feedback about the usage of the RequestSyncService. Thanks!

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: RequestSyncService test

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 8637819 [details] [diff] [review]
Part 3: RequestSyncService test

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

::: dom/requestsync/tests/test_request_sync_service_chrome_registration.html
@@ +29,5 @@
> +
> +function go() {
> +  let requestSyncMinInterval;
> +  try {
> +    requestSyncMinInterval = Services.prefs.getIntPref("dom.requestSync.minInterval");

can you do this:

SpecialPowers.pushPrefEnv({"set": [["dom.requestSync.minInterval", 1]]}, function() {
  .. all the rest...
});

@@ +35,5 @@
> +
> +  ok(RequestSyncService, "RequestSyncService exists");
> +  let principal = Services.scriptSecurityManager.getSystemPrincipal();
> +  let key = RequestSyncService.principalToKey(principal);
> +  ok(RequestSyncService._registrations[key] === undefined,

is(RequestSyncService._registrations[key] undefined, ...

@@ +42,5 @@
> +  Services.prefs.setIntPref("dom.requestSync.minInterval", 1);
> +
> +  let observer = (subject, topic, data) => {
> +    Services.obs.removeObserver(observer, "request-sync-test");
> +    ok(topic == "request-sync-test",

is

@@ +45,5 @@
> +    Services.obs.removeObserver(observer, "request-sync-test");
> +    ok(topic == "request-sync-test",
> +        "Observed request-sync-test notification");
> +    let dataObj = JSON.parse(data);
> +    ok(dataObj.data == "data",

is

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8637198 [details] [diff] [review]
Part 1: Allow chrome registrations on RequestSyncService

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

I would like to see it again with this comments applied. Thanks!

::: dom/requestsync/RequestSyncService.jsm
@@ +332,2 @@
>      if (!("wakeUpPage" in aParams) ||
> +        aParams.wakeUpPage.length == 0 &&

why this?

@@ +332,3 @@
>      if (!("wakeUpPage" in aParams) ||
> +        aParams.wakeUpPage.length == 0 &&
> +        !("observerTopic" in aParams)) {

a || b && c is confusing
I think you mean: a || (b && c)

@@ +355,5 @@
>      if (!this.validateRegistrationParams(aData.params)) {
> +      if (aTarget) {
> +        aTarget.sendAsyncMessage("RequestSync:Register:Return",
> +                                 { requestID: aData.requestID,
> +                                   error: "ParamsError" } );

I don't like this use of register. What about if:

1. you rename register, unregister and all the other methods used in 'receiveMessage' like: registerReceived, unregisterReceived, and so on...

2. the exposed API will have 'register' without aTarget

3. register and registerReceived will do 2 different validations and 2 callbacks if success or failure. these 2 callbacks can be used in line 398 and line 405.

@@ +668,5 @@
> +
> +      if (!manifestURL || !pageURL) {
> +        dump("ERROR!! RequestSyncService - Failed to create URI for the page or the manifest\n");
> +        aObj.active = false;
> +        this.updateObjectInDB(aObj);

Moving this block you break the logic because we don't remove the timer.

@@ +723,5 @@
> +      }, function() {
> +        debug("promise rejected");
> +        taskCompleted();
> +      });
> +    } else if (aObj.data.observerTopic) {

we are 100% sure we have the observerTopic here. Right?

@@ +725,5 @@
> +        taskCompleted();
> +      });
> +    } else if (aObj.data.observerTopic) {
> +      // Chrome sync request.
> +      Services.obs.notifyObservers(null, aObj.data.observerTopic,

instead having null, you can use the target.

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8637201 [details] [diff] [review]
Part 2: Schedule periodic SW updates

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

::: b2g/components/ServiceWorkers.jsm
@@ +128,5 @@
> +        if (Services.prefs.prefHasUserValue(RSYNC_MIN_INTERVAL_PREF)) {
> +          minInterval = Services.prefs.getIntPref(RSYNC_MIN_INTERVAL_PREF);
> +        }
> +      } catch(e) {
> +        dump(e);

debug(e); // or something better
return;

@@ +137,5 @@
> +        if (Services.prefs.prefHasUserValue(RSYNC_WIFI_ONLY_PREF)) {
> +          wifiOnly = Services.prefs.getBoolPref(RSYNC_WIFI_ONLY_PREF);
> +        }
> +      } catch(e) {
> +        dump(e);

here too.

@@ +196,5 @@
> +    let data;
> +    try {
> +      data = JSON.parse(aData);
> +    } catch(e) {
> +      dump(e);

debug or a better dump message.

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

We are finally going to be using the idle-daily mechanism on B2G as well, so I am closing this as WONTFIX.

Comment 20

[(no longer active)]

FWIW the spec around this has changed completely!  So the work done in bug 1112469 will need to get reverted.  Sorry for all of the wasted time on this!