1450070 - Application panel: add button to force Update

Status: NEW

(DevTools :: Application Panel, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

Follow up to Bug 1445197.

The Application panel will show a button to unregister a service worker. We should also have a button to force an update of a service worker. See mockups at https://projects.invisionapp.com/share/ZQGIRLJ2KBU#/screens/287275492_Artboard

Comment 1

[Julian Descottes [:jdescottes]]

I am trying to add an "Update" button in the application panel, that should trigger the update of a service worker registration (ie https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/update).

I tried using the same approach as in about:serviceworkers here, but the update button available there triggers a "soft update" (https://searchfox.org/mozilla-central/source/dom/interfaces/base/nsIServiceWorkerManager.idl#141) which is not followed by a new installation of the service worker. Is there another way to trigger a "real" update on a service worker registration? Or should we add new APIs to ServiceWorkerManager or ServiceWorkerRegistrationInfo?

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

I think we probably need to expose a skipWaiting() on the nsIServiceWorkerInfo.  That would allow devtools to trigger an update and then set the skipWaiting() on the installing worker.  This would then fully activate.  Once the activation completes you could refresh the tab.

Currently skipWaiting() is only available within the ServiceWorkerGlobalScope on the sw thread.

Comment 3

[Patrick Brosset <:pbro>]

Mass move to the new application panel component.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1450070 - WIP: Add update method to ServiceWorker actor

Review commit: https://reviewboard.mozilla.org/r/244870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244870/

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1450070 - WIP: Add update button to application panel UI

Review commit: https://reviewboard.mozilla.org/r/244872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244872/

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1450070 - WIP: Add test for application panel update button

Review commit: https://reviewboard.mozilla.org/r/244874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244874/

Comment 7

[Julian Descottes [:jdescottes]]

I will pick this back up when the blocking bugs have landed, but the current state is that we have:
- new actor method that "simulates" an update by doing a propagateSoftUpdate/attachDebugger/detachDebugger
- new button to trigger the update
- end to end test

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: bug1450070-skip-waiting.patch

Based on your suggestions, I tried to expose skipWaiting in the patch attached here. 

I don't think this is exactly what I need here, because the current issue is that after calling `propagateSoftUpdate`, installation is not triggered. 

I don't necessarily want to force skipWaiting as part of the update anyway, we should rather surface it as a dedicated control, for instance a "skip waiting" button or link displayed only when the current service worker is waiting.

I need an API that would do the exact same as calling "update()" on a service worker registration. Right now doing propagateSoftUpdate then attachDebugger then detachDebugger seems to "emulate" that (except it tries to start the worker even if there was no update). But that is really hacky. Should I try to implement a "propagateUpdate" that would perform the update of the service worker registration?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

You should not be calling propagateSoftUpdate, IMO.  We probably want you to be able to trigger an update similar to what navigations trigger, which is ServiceWorkerRegistrationInfo::MaybeScheduleUpdate().  Of course, that uses a 1 second delay that may not be what you want.

Honestly I think it would be easier to implement all this after the e10s work is complete.  I'm worry you are going to implement something that needs to be re-done shortly.  For example, all the "propagate" stuff is going to go away.

Also, what process are you accessing the nsIServiceWorker* interface is it the parent process?  These interfaces are going to go away in the content processes soon.

Comment 10

[Julian Descottes [:jdescottes]]

Thanks for the feedback. 

> We probably want you to be able to trigger an update similar to what navigations trigger, 
> which is ServiceWorkerRegistrationInfo::MaybeScheduleUpdate().  Of course, that uses a 1 
> second delay that may not be what you want.

A one second delay is not a big issue for now, we could run with that.

> Honestly I think it would be easier to implement all this after the e10s work is complete.
> I'm worry you are going to implement something that needs to be re-done shortly.  

Sure, let's block this on Bug 1231208 then. I will re-upload cleaned up versions of the UI and test patches, but will leave the actor patch for later.

> For example, all the "propagate" stuff is going to go away.

We already rely on propagate* to unregister service workers for now: 
https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/devtools/server/actors/worker.js#387

That's one spot that needs to be updated when the refactor lands.

> Also, what process are you accessing the nsIServiceWorker* interface is it the parent process?
> These interfaces are going to go away in the content processes soon.

Normally from the parent process, but to start service workers (with attach/detachDebugger), we also broadcast a message to all processes: https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/devtools/server/actors/worker.js#359-367

That will also need to be updated.

Comment 11

[Julian Descottes [:jdescottes]]

unassigning since we are not working on this at the moment.

Comment 12

[:Harald Kirschner :digitarald]

(Switched blocking to parent intercept being enabled)

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Relevant context if revisiting this:

• We removed registration resurrection in bug 1557244 so (from the content global) a call to unregister() on a registration followed by an equivalent call to ServiceWorkerContainer.register() (with the same scope) will definitely cause the install process to happen.
• Because of security-related enhancements being made in bug 1544232 surrounding concerns about having ServiceWorkers running in the background without an owning tab, I don't know that we want to add any kind of methods to our XPCOM interfaces that would potentially try to initiate an update outside the context of an existing Client. Especially since I think presumably devtools would be operating in the context of a specific client. So I think it is interesting considering just kicking off such an update from the relevant content global (via xrays), especially if the goal is just for the user to be able to debug the whole evaluating / installing / waiting / active chain.
  • That said we could always expose ChromeOnly helpers on the WebIDL.
  • If the high level goal is to help debug "why is my ServiceWorker not updating?", that would be something different. In general, ServiceWorkers check for updates arguably too much by spec, so if someone's SW is not updating the problem will involve something like caching or throwing errors or other things, so I'm not sure forcing an update is actually beneficial in that case. We would potentially want more instrumentation in that case, though.