Closed Bug 1228382 Opened 4 years ago Closed 4 years ago

Keep service worker alive when opening a toolbox against them

Categories

(DevTools :: about:debugging, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached patch patch v1 (obsolete) — Splinter Review
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...
Attachment #8692585 - Flags: feedback?(janx)
Attachment #8692585 - Flags: feedback?(ejpbruel)
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 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.
Attachment #8692585 - Flags: feedback?(bkelly)
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.
Attachment #8692585 - Flags: feedback?(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 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".
Attachment #8692585 - Flags: feedback?(janx)
Attachment #8692585 - Flags: feedback?(bkelly)
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?
Attached patch patch v2 (obsolete) — Splinter Review
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?
Attachment #8693730 - Flags: feedback?(bkelly)
Attachment #8692585 - Attachment is obsolete: true
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);
Attachment #8693730 - Flags: feedback?(bkelly) → feedback+
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!
Attachment #8693730 - Flags: feedback?(ejpbruel)
Depends on: 1225473
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 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.
Attachment #8693730 - Flags: feedback?(ejpbruel) → feedback+
Attached patch platform patch v1 (obsolete) — Splinter Review
Here is the patch split in two.
First part, the platform one which just add the necessary API
to match nsIServiceWorkerInfo and nsIWorkerDebugger.
Attachment #8693730 - Attachment is obsolete: true
Attached patch about debugging patch - v1 (obsolete) — Splinter Review
And the second part, modifying about:debugging to freeze the worker
when we start debugging them.

Both patches include test at each level.
Comment on attachment 8694851 [details] [diff] [review]
platform patch v1

Try is green, time for reviews!
Attachment #8694851 - Flags: review?(ejpbruel)
Attachment #8694852 - Flags: review?(janx)
Attachment #8694852 - Flags: review?(ejpbruel)
Assignee: nobody → poirot.alex
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 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.
Attachment #8694851 - Flags: review?(ejpbruel) → review+
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.
Attachment #8694852 - Flags: review?(ejpbruel) → review+
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.
Attachment #8694852 - Flags: review?(janx) → review+
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
(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.
With uuid changed. I didn't knew we had checks done during push to hg!
Attachment #8698395 - Flags: review+
Attachment #8694851 - Attachment is obsolete: true
Attachment #8694852 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/153bbd21f70e
https://hg.mozilla.org/mozilla-central/rev/038664873a7b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
[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
[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
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.