Closed Bug 1181544 Opened 5 years ago Closed 3 years ago

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

Categories

(Firefox OS Graveyard :: Runtime, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ferjm, Unassigned)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → ferjmoreno
Status: NEW → ASSIGNED
Target Milestone: --- → FxOS-S2 (10Jul)
Blocks: 1156092
Attached patch WIP (obsolete) — Splinter Review
Depends on: 1182117
Target Milestone: FxOS-S2 (10Jul) → FxOS-S3 (24Jul)
Summary: Updating a service worker via about:sw in Settings doesn't work if the owner is not running → Service workers updates needs to be done in the appropriate content process
Attached patch v1 (obsolete) — Splinter Review
I still need to write some tests for this, but I could use some feedback in the meantime. Thanks!
Attachment #8631156 - Attachment is obsolete: true
Attachment #8633534 - Flags: feedback?(amarchesini)
Attached patch v1 (obsolete) — Splinter Review
Now without the debug traces.
Attachment #8633534 - Attachment is obsolete: true
Attachment #8633534 - Flags: feedback?(amarchesini)
Attachment #8633535 - Flags: feedback?(amarchesini)
Actually, I don't think we can do much more wrt testing this feature other than what's been done on bug 	1171917
Comment on attachment 8633535 [details] [diff] [review]
v1

Vivien, could you take a look at this patch, please? Thanks!
Attachment #8633535 - Flags: review?(21)
I am adding the tests dependency so we don't land this before the tests are ready.
Depends on: 1171917
Depends on: 1179191
No longer depends on: 1182117
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.
Attachment #8633535 - Flags: feedback?(amarchesini) → feedback+
Attached patch v1Splinter Review
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.
Attachment #8633535 - Attachment is obsolete: true
Attachment #8633535 - Flags: review?(21)
Attachment #8637195 - Flags: review?(21)
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 :|
Attachment #8637195 - Flags: review?(fabrice)
Per comment 8, setting the appropriated dependency here. Thanks!
Depends on: 1182117
No longer depends on: 1179191
Target Milestone: FxOS-S3 (24Jul) → FxOS-S4 (07Aug)
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...
Attachment #8637195 - Flags: review?(21) → review-
Attachment #8637195 - Flags: review?(fabrice)
Target Milestone: FxOS-S4 (07Aug) → FxOS-S7 (18Sep)
Target Milestone: FxOS-S7 (18Sep) → FxOS-S8 (02Oct)
Target Milestone: FxOS-S8 (02Oct) → FxOS-S9 (16Oct)
Assignee: ferjmoreno → nobody
Status: ASSIGNED → NEW
Target Milestone: FxOS-S9 (16Oct) → ---
We are moving to a new architecture that will solve this.  See ServiceWorkers-e10s bug.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.