1228382 - Keep service worker alive when opening a toolbox against them

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect)

Description

[Alexandre Poirot [:ochameau]]

When you attach a toolbox to a service worker,
the worker is going to be terminated unless you manager to set and hit a breakpoint. In any other scenario, the worker is going to be killed and the toolbox is going to stay visible and act like a zombie.

Ideally, once we open a toolbox, the worker is going to be kept alive.
We should also ensure that the toolbox is closed or explicitely disabled once the worker is gone.
It may also help debugging them by increasing the kill timeout in order to attach to a live one.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch v1

Here is a wip patch.
It adds principal and serviceWorkerID attribute to nsIWorkerDebugger,
that to be able to call the new getRegistrationByPrincipal(principal, scope)
which allow us to call RegistrationInfo.forceKeepAlive()
that triggers some code in ServiceWorkerPrivate to keep it alive.

There is most likely things to tweak in ServiceWorkerPrivate in order 
to release the worker after that. But it seems to work at first sight,
but haven't been able to test it extensively as my toolbox are suffering
from the invisible regression and I have to rebuild...

Comment 2

[Alexandre Poirot [:ochameau]]

I'm not sure about the getRegistrationByPrincipal. There might be better ways to match a nsIWorkerDebugger with its related nsIServiceWorkerRegistrationInfo. I'm wondering if it would be better to match with the ID? Or have a getRegistrationForDebugger?
The main issue is that existing getRegistration don't work here as the nsIWorkerDebugger here have undefined `window`.
I was lazy and didn't implemented the lazy+promise implementation, but we can use promise for this as well.

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8692585 [details] [diff] [review]
patch v1

Ben, I think I can ask you about Service workers right?
Could you take a look at the mForceKeepAlive hack within dom/workers/ServiceWorkerPrivate.cpp
Is that sane practice?
Would you do it differently? Or wouldn't even trying to keep a SW alive??
If that's somewhat sane, I can work on trying to tweak that to release it once devtools detach from the worker.

Comment 4

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8692585 [details] [diff] [review]
patch v1

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

I already implemented the platform parts you need for this in bug 1219255. The patch in that bug keeps the service worker alive, but also creates a WorkerPrivate for it if one doesn't already exist. I've implemented this as an attach/detach mechanism because in theory we allow multiple clients to attach to the same worker, and we need to be able to stop keeping a service worker alive when no more clients are attached.

I would recommend you use the API in bug 1219255 and implement the server parts on top of that. However, note that since we also want to be able to debug ServiceWorkers for which a WorkerPrivate does not exist (and for which we therefore have no WorkerActor), the *correct* place to implement this is on ServiceWorkerActor, which wraps a ServiceWorkerInfo instead.

I already implemented all this in bug 1226285. Of course, I realise that that bug is still blocked on the gc issues we've been seeing with bug 1220741, so what you're trying to do seems like a good temporary solution. However, I think the medium term plan should be to replace this patch with the work from bug 1220741.

Comment 5

[Eddy Bruel [:ejpbruel]]

Could you please flag me for review on the final patch? I'd like to make sure that what you're doing does not interfere with the work I've been doing.

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8692585 [details] [diff] [review]
patch v1

Oh great! I didn't knew about bug 1219255.
I asked you about how you would freeze the worker and thought you didn't already submitted bug/patch for it.

I'll rebase this on top of your patch.
The part of the patch to related a given worker to a giver service worker info is still usefull to debug "real life workers".

Comment 7

[Jan Keromnes [:janx]]

Very cool, thanks for digging into this with Eddy!

I couldn't apply your patch locally, but I'm probably not right person to review this anyway.

One question: How do you "unForceKeepAlive"? Once you've "forceKeepAlive()ed" a private, does it stay around indefinitely?

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Rebased on top of bug 1219255.
AttachDebugger/DetachDebugger works great!

I just have some issues with the new test.
Opening a service worker toolbox leaks...
The debugger panel seems to be leaked and introduce
the toolbox, target and client to be also leaked.

This patch still includes some platform tweaks to relates
a given WorkerDebugger to a ServiceWorkerInfo.
So that we can debug "real life" service workers.
I still don't know if that's the best API we can come up with.
May be nsIWorkerDebugger should expose the related nsIServiceWorkerInfo.
As nsIWorkerDebugger already exposes the related nsIServiceWorkerInfo...

Also, I've seen some strange behavior that introduces races.
nsIServiceWorkerRegistrationInfo seems to be updated with some delay.
We get the nIWorkerDebugger instance early, and only later on (not in the same cycle),
the nsIServiceWorkerRegistrationInfo is updated and gets {waiting,installing,active}Worker
field updated and my new getWorkerByID method successfully return the nsIServiceWorkerInfo.

Ben, What do you think about all the stuff I introduced to relate
nsIWorkerDebugger and nsIServiceWorkerInfo?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8693730 [details] [diff] [review]
patch v2

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

Looks ok to me, but to be honest I do not know the worker debugger side of things at all.  These getters seem ok, though.

::: devtools/server/actors/worker.js
@@ +69,5 @@
>      if (!this._isAttached) {
> +      if (this._dbg.type == Ci.nsIWorkerDebugger.TYPE_SERVICE) {
> +        let info = swm.getRegistrationByPrincipal(this._dbg.principal, this._dbg.url);
> +        let worker = info.getWorkerByID(this._dbg.serviceWorkerID);
> +        worker.attachDebugger();

Do you want to throw here if worker is null?  If the work is exiting, I'm not sure if you can end up with a stale serviceWorkerID here or not.

@@ +153,5 @@
>  
> +    if (this._dbg.type == Ci.nsIWorkerDebugger.TYPE_SERVICE) {
> +      let info = swm.getRegistrationByPrincipal(this._dbg.principal, this._dbg.url);
> +      let worker = info.getWorkerByID(this._dbg.serviceWorkerID);
> +      worker.detachDebugger();

Also seems you could factor this out into a single function called from both places.

::: dom/workers/ServiceWorkerManager.cpp
@@ +489,5 @@
>  
>  NS_IMETHODIMP
> +ServiceWorkerRegistrationInfo::GetWorkerByID(uint64_t aID, nsIServiceWorkerInfo **aResult)
> +{
> +  AssertIsOnMainThread();

MOZ_ASSERT(aResult);

@@ +3867,5 @@
> +ServiceWorkerManager::GetRegistrationByPrincipal(nsIPrincipal* aPrincipal,
> +                                                 const nsAString& aScope,
> +                                                 nsIServiceWorkerRegistrationInfo** aInfo)
> +{
> +  MOZ_ASSERT(aPrincipal);

MOZ_ASSERT(aInfo);

::: dom/workers/WorkerPrivate.cpp
@@ +3712,5 @@
>  
>  NS_IMETHODIMP
> +WorkerDebugger::GetPrincipal(nsIPrincipal** aResult)
> +{
> +  AssertIsOnMainThread();

MOZ_ASSERT(aResult);

@@ +3727,5 @@
> +
> +NS_IMETHODIMP
> +WorkerDebugger::GetServiceWorkerID(uint32_t* aResult)
> +{
> +  AssertIsOnMainThread();

MOZ_ASSERT(aResult);

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8693730 [details] [diff] [review]
patch v2

Eddy, What do you think about this new one?
It doesn't replace all what you are doing but instead allow to debug "real life workers", i.e. the one that are automatically spawn by the platform.
I'll split this patch in two between the platform side that allows to relates nsIWorkerDebugger and nsIServiceWorkerInfo. This is the first patch I would like some feedback on.
And a second one that is going to focus to call attachDebugger/detachDebugger for service workers. I've seen various of your patches, and I'm not sure there is much various in making two classes of WorkerActor matching nsIWorkerDebugger. It sounds easier to introduce some specifics to service workers at some critical spots. But there is a lot of value for new actors you are introducing for registrations!

Comment 11

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4083d07ce73

Comment 12

[Alexandre Poirot [:ochameau]]

I figured out the leaks I reported in comment 8.
This isn't related to this patch. about:debugging wasn't cleaning itself up correctly.
I submitted patch in bug 1225473 to fix that.

Comment 13

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8693730 [details] [diff] [review]
patch v2

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

I have no fundamental problems with this patch as it stands, provided we both agree that this is a temporary hack, and should be removed once the actual service worker debugger API lands. Please add a comment to that effect somewhere. Other than that, I have nothing to add to Ben's comments.

I'd like the opportunity to give this patch a proper review once it is finished. It looks pretty good as it is, so I expect to give it an r+ with comments addressed.

Comment 14

[Alexandre Poirot [:ochameau]]

Attachment: platform patch v1

Here is the patch split in two.
First part, the platform one which just add the necessary API
to match nsIServiceWorkerInfo and nsIWorkerDebugger.

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: about debugging patch - v1

And the second part, modifying about:debugging to freeze the worker
when we start debugging them.

Both patches include test at each level.

Comment 16

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ecd59b0932f1

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 8694851 [details] [diff] [review]
platform patch v1

Try is green, time for reviews!

Comment 18

[Jan Keromnes [:janx]]

Very sorry to delay this review, but I'm under water with bug 1224725 and associated meetings. I hope to get to this some time tomorrow.

Comment 19

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8694851 [details] [diff] [review]
platform patch v1

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

Please add a comment somewhere that explains why this API is here, and that we'd like to replace it in the long term.

Other than that, this patch looks good to go.

Comment 20

[Eddy Bruel [:ejpbruel]]

Comment on attachment 8694852 [details] [diff] [review]
about debugging patch - v1

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

r+ with comments addressed.

::: devtools/server/actors/worker.js
@@ +59,5 @@
>        return { error: "closed" };
>      }
>  
>      if (!this._isAttached) {
> +      if (this._dbg.type == Ci.nsIWorkerDebugger.TYPE_SERVICE) {

Please add a comment here that explains why we are doing this and that we'd like to replace this in the long run.

@@ +142,5 @@
> +    // If the worker is already destroyed, nsIWorkerDebugger.type throws
> +    let type;
> +    try {
> +      type = this._dbg.type;
> +    } catch(e) {}

You should be able to use the closed property for this. It returns true if the worker is already destroyed.

Comment 21

[Jan Keromnes [:janx]]

Comment on attachment 8694852 [details] [diff] [review]
about debugging patch - v1

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

Thanks for the additional test, it passes!

However, I still see these issues:
- When opening a toolbox on an "Other Worker" (e.g. SessionWorker.js) twice, the debugger breaks the second time (I was hoping that detaching the client would fix it)
- I tried your patch on https://mdn.github.io/sw-test/, and saw this error message[0] in the Toolbox's Console (only once).
- The service worker was still killed after a (rather long) while, even though I had an active Toolbox on it.

[0] connectionClosed: 'startListeners' request packet to 'server1.conn10.worker5/console2' can't be sent as the connection is closed.

::: devtools/client/aboutdebugging/test/browser.ini
@@ +10,5 @@
>  
>  [browser_addons_install.js]
>  [browser_service_workers.js]
> +[browser_service_workers_timeout.js]
> +skip-if = debug # leak the worker debugger panel

Please address this, or file a follow up bug and mention it here.

Comment 22

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe2271cd859c

Comment 23

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e86618c3a065

Comment 24

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/153bbd21f70e38ac3d5a795564ee303c76538afb
Bug 1228382 - Expose an API to relate nsIWorkerDebugger to its nsIServiceWorkerInfo instance. r=ejpbruel

https://hg.mozilla.org/integration/fx-team/rev/038664873a7bb62ef3fc904511dc063a28e601a4
Bug 1228382 - Keep service worker alive when attaching to them. r=janx,ejpbruel

Comment 25

[Alexandre Poirot [:ochameau]]

(In reply to Eddy Bruel [:ejpbruel] from comment #20)
> @@ +142,5 @@
> > +    // If the worker is already destroyed, nsIWorkerDebugger.type throws
> > +    let type;
> > +    try {
> > +      type = this._dbg.type;
> > +    } catch(e) {}
> 
> You should be able to use the closed property for this. It returns true if
> the worker is already destroyed.

It is not enough. closed appear to be false and it throws, so I kept the try/catch thing.

Comment 26

[Alexandre Poirot [:ochameau]]

Attachment: platform patch v2

With uuid changed. I didn't knew we had checks done during push to hg!

Comment 27

[Alexandre Poirot [:ochameau]]

Attachment: about debugging patch v2

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/153bbd21f70e
https://hg.mozilla.org/mozilla-central/rev/038664873a7b

Comment 29

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent 		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected

Comment 30

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent   		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected