1181544 - Service workers updates needs to be done in the appropriate content process

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Runtime, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

When we request an update for a sw from the Settings app, it only gets updated when the application owning that sw is running.

This needs to be fixed also for periodic sw updates on b2g.

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: WIP

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

I still need to write some tests for this, but I could use some feedback in the meantime. Thanks!

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

Now without the debug traces.

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Actually, I don't think we can do much more wrt testing this feature other than what's been done on bug 	1171917

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8633535 [details] [diff] [review]
v1

Vivien, could you take a look at this patch, please? Thanks!

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

I am adding the tests dependency so we don't land this before the tests are ready.

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8633535 [details] [diff] [review]
v1

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

::: b2g/chrome/content/shell.js
@@ +1224,5 @@
> +      dump(e);
> +      return;
> +    }
> +
> +    if (!serviceWorkerUpdateData ||

Would be nice to have a validator method that returns a boolean if this object exists and it's valid. You do this check many times in this patch.

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: v1

With this patch we create the child process hosting the service worker owner to launch the update.

Unfortunately, this needs to be disabled until bug 1182117 is done :(, and so we still need to do the updates from the parent process. I commented the part where the child process creation is made and kept the old update request done from the parent, along with a comment explaining the reason. Once bug 1182117 I'll move to the child process update part.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8637195 [details] [diff] [review]
v1

Fabrice, I know that you are on PTO until this Friday, but maybe you'll get to this before Vivien.

We really need more peers for the B2G runtime :|

Comment 10

[Noemí Freire (:noemi)]

Per comment 8, setting the appropriated dependency here. Thanks!

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8637195 [details] [diff] [review]
v1

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

::: b2g/chrome/content/serviceWorkerUpdater.js
@@ +26,5 @@
> +  let data = aMessage.data;
> +  if (!data ||
> +      !data.originAttributes ||
> +      !data.scope) {
> +    dump("Service Worker Update: missing information\n");

Use the console service instead of a raw dump.

::: b2g/chrome/content/shell.js
@@ +1229,5 @@
> +    if (!serviceWorkerUpdateData ||
> +        !serviceWorkerUpdateData.originAttributes ||
> +        !serviceWorkerUpdateData.scope) {
> +      return;
> +    }

You are already in a method with ServiceWorker in the name. No need to prefix updateData with ServiceWorker. It just makes the code more verbose and force you to line break all over the place.

@@ +1264,5 @@
> +      mm.addMessageListener("service-worker-update-done", onDone);
> +    } catch (e) {
> +      dump("Could not load serviceWorkerUpdater.js frame script " + e + "\n");
> +    }
> +  }, "service-worker-update", false /* ownsWeak */);

Ok. That's the part I don't want. I want to delegate all window creations to the system app. Also, if a window is already opened on the system app side., it just seems a waste to create a new TabParent/TabChild just to call your frame script.

So instead, you want to forward a message to Gaia, telling to call iframe.update() (or a similar API). Gaia can then look into its list of opened window. If any is opened for the target informations, it will call iframe.update on it, which will call a method of the mozbrowserAPI.
If none is opened, it will open a new iframe mozbrowser for it and then call iframe.update afterward.

Similarly the call to ask Gaia to open a window for ServiceWorker related task could be useful for the Push Notification APIs which also use nsIServiceWorkerManager but I dunno how it works if there is no open frame for it.

::: b2g/components/ServiceWorkers.jsm
@@ +51,5 @@
>  }
>  
> +let ServiceWorkerUpdateData = function(aOriginAttributes, aScope) {
> +  this.originAttributes = aOriginAttributes;
> +  this.scope = aScope;

Can you add a this.manifestURL = null; property too ?
You are changing that later, will be cool this property explicit.

@@ +52,5 @@
>  
> +let ServiceWorkerUpdateData = function(aOriginAttributes, aScope) {
> +  this.originAttributes = aOriginAttributes;
> +  this.scope = aScope;
> +};

Tbh I also wondering if you really need this to be a function that you build with new instead of a simple js object you pass to the update method. You are not doing anything special with this function.

It will make more sense if you update the manifestURL property in this function when it is build if a function is really what you want.

@@ +85,5 @@
>        error: aError
>      });
>    },
>  
> +  update: function(aServiceWorkerUpdateData) {

aServiceWorkerUpdateData/aUpdateData

We are already into ServiceWorker lands, no need to prefix the variable with. It just makes the code more verbose without additional benefits.

@@ +89,5 @@
> +  update: function(aServiceWorkerUpdateData) {
> +    if (!aServiceWorkerUpdateData ||
> +        !aServiceWorkerUpdateData.originAttributes ||
> +        !aServiceWorkerUpdateData.scope) {
> +      dump("Missing service worker update information\n");

Use the console service if you really want to warn about that.

@@ +102,5 @@
> +    //     service worker being updated is not opened.
> +    gServiceWorkerManager.propagateSoftUpdate(
> +      aServiceWorkerUpdateData.originAttributes,
> +      aServiceWorkerUpdateData.scope
> +    );

I wondering why do you have a UpdateData object that contains both properties when propagateSoftUpdates does not ? Maybe there was a reason for splitting both into separate parameters ?

I tend to think that we should align our parameters over all the methods. But I have to admit that I prefer the single parameter as you are doing...

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

We are moving to a new architecture that will solve this.  See ServiceWorkers-e10s bug.