1164829 - Add a button to stop a Service worker in about:debugging

Status: NEW

(DevTools :: about:debugging, defect, P2)

Description

[Noemí Freire (:noemi)]

As discussed during the Service Workers work week in Madrid, in order to facilitate the way to inspect, debug, handle Service Workers a button to stop a Service Worker could be added in about:serviceworkers

Comment 1

[Jan Keromnes [:janx]]

Given that service workers get terminated anyway after being idle for a short while, I'm not sure how useful such a button would be in about:debugging.

Comment 3

[Jan Keromnes [:janx]]

Increasing priority based on Sole's feedback in bug 1253621 comment 0.

Comment 4

[Jan Keromnes [:janx]]

dev-doc-needed: This additional should be documented in https://developer.mozilla.org/en-US/docs/Tools/about%3Adebugging when it lands.

Comment 5

[Jan Keromnes [:janx]]

Helen, what would be a good way to surface this additional Service Worker feature in about:debugging?

- Add a "Stop" button next to the "Debug" and "Push" buttons when a service worker is running?
- Add a "stop" link somewhere in the details?
- Add a dropdown button for less useful features, and hide "Stop" in it?
- Implement a right-click menu, and add a "Stop" entry to it?
- Or some other, better way?

I don't think it's super-useful, because you can just wait a few seconds for the service worker to be killed, but it seems important to some users.

Comment 6

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Jan Keromnes [:janx] from comment #5)
> Helen, what would be a good way to surface this additional Service Worker
> feature in about:debugging?
> 
> - Add a "Stop" button next to the "Debug" and "Push" buttons when a service
> worker is running?
> - Add a "stop" link somewhere in the details?
> - Add a dropdown button for less useful features, and hide "Stop" in it?
> - Implement a right-click menu, and add a "Stop" entry to it?
> - Or some other, better way?
> 
> I don't think it's super-useful, because you can just wait a few seconds for
> the service worker to be killed, but it seems important to some users.

Let's try adding a "stop" link somewhere in the details and gauge how important the feature is—if we're already getting feedback that users really feel they need it, I think it should at least be somewhere obvious (instead of in a context menu). I'm not personally opposed to putting it up next to the "Debug" and "Push" buttons other than that area would get a bit unwieldy with a third button, right?

Comment 7

[Benoit Chabod [:bigben]]

I've started investigating into this, and I would like to take this bug if you guys are okay with it.
From my point of view, we could implement this feature imitating the "start" button process.
Here's what I've understood (correct me if I'm wrong):

1. Clicking the button launches a "start" request on the worker actor [clients/aboutdebugging/components/worker/panel.js]

2. Using the PPMM, the worker actor loads a script in the child process and then broadcasts a "start" message along with the worker's scope [server/actors/worker.js]

3. Upon the message reception, the script loaded earlier parses the worker registration list (calling ServiceWorkerManager), finds the right one using the scope parameter, and starts it using attachDebugger/detachDebugger (a bit hacky, but it works) [server/service-worker-child.js]

The stop feature could be achieved similarly. The service-worker-child script could contain another message listener for "serviceWorkerRegistration:stop", and it would kill the worker instance using "TerminateWorker()" defined in /dom/workers/ServiceWorkerPrivate (luckily, we can access the ServiceWorkerPrivate object through ServiceWorkerInfo).

Tell me what you think of this approach :)

Comment 8

[Jan Keromnes [:janx]]

(In reply to Benoit Chabod [:bigben] from comment #7)
> I've started investigating into this, and I would like to take this bug if
> you guys are okay with it.

Of course, thank you Benoit!

> 1. Clicking the button launches a "start" request on the worker actor
> [clients/aboutdebugging/components/worker/panel.js]

Correct.

> 2. Using the PPMM, the worker actor loads a script in the child process and
> then broadcasts a "start" message along with the worker's scope
> [server/actors/worker.js]

Correct, but the script is only loaded once per process (the first time we need it), after that just broadcast the "start" message.

Note: We broadcast from the main process to all processes, because we're trying to trigger an `attachDebugger()` call from within the content process where the corresponding worker actually lives (if we called this directly from the main process, we'd start a separate worker instance within that process, which would never be able to control its website and misbehave in very confusing ways).

> 3. Upon the message reception, the script loaded earlier parses the worker
> registration list (calling ServiceWorkerManager), finds the right one using
> the scope parameter, and starts it using attachDebugger/detachDebugger (a
> bit hacky, but it works) [server/service-worker-child.js]

Exactly.

> The stop feature could be achieved similarly. The service-worker-child
> script could contain another message listener for
> "serviceWorkerRegistration:stop", and it would kill the worker instance
> using "TerminateWorker()" defined in /dom/workers/ServiceWorkerPrivate
> (luckily, we can access the ServiceWorkerPrivate object through
> ServiceWorkerInfo).
> 
> Tell me what you think of this approach :)

Actually, I don't think we need to use this broadcasting system for "stop", because we can use the WorkerActor for that directly:

- In about:debugging, the WorkerActors we have already live in the right process (a content process in e10s mode), so calling "Debug" on them just works.
- However, ServiceWorkerRegistrationActors don't live in the right process (except in single-process mode) because they're all in the main process.
- When a service worker is registered but not started, we have a ServiceWorkerRegistrationActor, but there is no WorkerActor available, because there is no running worker private anywhere.
- In the above case, in order to cause a worker private to start running in the right process, we use the broadcasting system above. Once the worker private is started, about:debuggin gets a corresponding WorkerActor, and can use it for everything else.
- If we want to stop a running worker private, we're certain to already have a WorkerActor for it in the right proccess, because it's already running and about:debugging will know about it. So we can just implement a "stop" call on WorkerActor, which should terminate the worker private e.g. by using an appropriate platform call.

Sorry this is a bit subtle, but as I mentioned earlier we had to work around some cases where the platform didn't work properly across multiple processes.

Comment 9

[Benoit Chabod [:bigben]]

Attachment: 1st patch - add a stop button for service workers

Okay guys, here's a first attempt at a patch to provide this feature in about:debugging.
I've had issues with an intermittent segmentation fault that seems to have vanished, so I'm not 100% confident this works properly.
I guess we will need additional tests to make sure that the service worker actually stops when the button is pressed.

As Helen suggested, I've added the "stop" link in the details, near the scope and unregister.
Having a third button near Debug and Push would probably not look good. But maybe we should think of a better way to represent the fact that the service worker is alive or dead.
For example, it could be a play/stop icon similar to a music player. What do you think?

By the way, sometimes the about:debugging view won't refresh automatically when the service worker is killed.
I could force it to update using:

    client.send({
      from: this.actorID,
      type: "serviceWorkerRegistrationListChanged"
    });

But I don't know if this is acceptable.
Jan, don't hesitate to pass the review flag on to someone else as I know you're away this week.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8757304 [details] [diff] [review]
1st patch - add a stop button for service workers

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

Could you split your patch in two?
Move nsIServiceWorkerManager.idl + ServiceWorkerInfo.cpp modifications into a dedicated patch.
And provide a low level test for terminateWorker?
You could take dom/workers/test/serviceworkers/test_serviceworkerinfo.xul as an example.
You can run this test via:
./mach mochitest dom/workers/test/serviceworkers/test_serviceworkerinfo.xul
When creating a new test, you have to register it in dom/workers/test/serviceworkers/chrome.ini

Once you get this patch, you should get the review from :bkelly or someone in the service worker team.

Otherwise, the patch against aboutdebugging looks good to me.
You also need a patch against it, you can tweak or copy devtools/client/aboutdebugging/test/browser_service_workers_start.js

::: devtools/server/actors/worker.js
@@ +156,5 @@
>      request: {},
>      response: RetVal("json")
>    }),
>  
> +  stop: method(function () {

You should add this check first:
   if (this._dbg.type !== Ci.nsIWorkerDebugger.TYPE_SERVICE) {
      return { error: "wrongType" };
    }
As stop only works for service workers.

Comment 11

[Benoit Chabod [:bigben]]

Attachment: Part 1: Add backend to kill a service worker

Sure, I can split the patch in two.
To begin with, here's the low level part modifying our .idl and .cpp service worker implementation.
The main goal here is to make the TerminateWorker() method available in ServiceWorkerInfo object, because it is available on the JS side.

I've added a test in test_serviceworkerinfo.xul, but it's not conclusive.
Indeed, I can't really test if a service worker is "dead" or not, so I just check if the .debugger attribute has been nullified.
Other tests in the same file rely on a system message called "service-worker-shutdown" to ensure that the service worker is dead, but this system message is dispatched in the first lines of "TerminateWorker()" so it doesn't prove much.

I also had to set mDebuggerCount and mTokenCount to 0 in the original TerminateWorker() method, because any calls to GetDebugger would crash otherwise once the service worker has been killed.
I guess it was assumed that all debuggers would be detached when TerminateWorker() is called, but this is not the case anymore.

Ben, can you review this part?
I want to be sure this is the right way to kill a service worker.
In e10s method, it looks like you have to call this method multiple times to really stop the corresponding service worker :/
But this might be due to an error in part 2 (JS)

Comment 12

[Benoit Chabod [:bigben]]

Attachment: Part 2: Add the stop button in about:debugging

And this is the second part of the patch, regarding about:debugging itself.
I still have to write the test, and to close any debugger toolbox attached when the service worker is stopped using this feature, so this isn't ready for review yet.

However, I thought this would be useful for anyone wanting to test part 1 on the JS side.
As I said earlier, there is a weird problem with e10s mode.
If a tab is openned containing a page with an URL included in the service worker's scope and you try to "start" then "stop" the service worker, you have to press the stop link exactly twice to kill the service worker. Only one click is necessary if this tab isn't opened... It's as if there were two actors spawned for a single worker.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8759205 [details] [diff] [review]
Part 1: Add backend to kill a service worker

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

Sorry, but this doesn't seem quite right.  We can't just null out the counters since it will mess up future state changes.

::: dom/workers/ServiceWorkerPrivate.cpp
@@ +1745,5 @@
>    AssertIsOnMainThread();
>  
>    mIdleWorkerTimer->Cancel();
>    mKeepAliveToken = nullptr;
> +  mDebuggerCount = 0;

Can you just call serviceWorkerInfo->detachDebugger() before TerminateWorker() instead?  You could make serviceWorkerInfo->terminateWorker() throw if a debugger is attached.

@@ +1746,5 @@
>  
>    mIdleWorkerTimer->Cancel();
>    mKeepAliveToken = nullptr;
> +  mDebuggerCount = 0;
> +  mTokenCount = 0;

I don't think this is right.  There can still be KeepAliveToken objects alive that will decrement mTokenCount when they go away.  That would then underflow mTokenCount.

::: dom/workers/test/serviceworkers/test_serviceworkerinfo.xul
@@ +96,5 @@
> +          info("Attach a debugger to the service worker so that it restarts " +
> +               "and check that calling stop kills it instantly");
> +          activeWorker.attachDebugger();
> +          activeWorker.terminateWorker();
> +          ok(activeWorker.debugger === null);

I think you should expose a .running attribute instead.  This would get piped down to the ServiceWorkerPrivate and return true if mWorkerPrivate is not nullptr.  Does that make sense?

Comment 14

[Benoit Chabod [:bigben]]

Attachment: Part 1: Add backend to kill a service worker, v2

You're right, this was a bit messy!
I thought it would be the debuggers and the tokens responsability to detect that the worker was killed, and to prevent incorrect state changes, but it was a bad idea.

Here's a new version of the patch. TerminateWorker() now throws a NS_ERROR_UNEXPECTED if it's called while a debugger is still attached.
I've added a .running attribute available in ServiceWorkerPrivate and ServiceWorkerInfo, and I've created two tests for these changes.
I guess we could change the whole test suite to use this new attribute, but I don't know if it would be useful.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8759638 [details] [diff] [review]
Part 1: Add backend to kill a service worker, v2

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

Looks good, thanks!  r=me with comments addressed.

Please make sure you test the case where someone has a debugger window open for the SW script and then the "stop" button is pushed.  What is the desired behavior there?  Should the "stop" button be disabled or automatically close the debugger window?  I think devtools code needs to figure that part out.

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ +38,5 @@
>    void attachDebugger();
>  
>    void detachDebugger();
> +
> +  void terminateWorker();

Please add a comment that this can only be called if there are no attached debuggers.

::: dom/workers/ServiceWorkerPrivate.h
@@ +137,5 @@
>    void
>    Activated();
>  
> +  bool
> +  isRunning();

Please make this a const method.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8759209 [details] [diff] [review]
Part 2: Add the stop button in about:debugging

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

::: devtools/server/actors/worker.js
@@ +170,5 @@
> +    }
> +    worker.terminateWorker();
> +    this.conn.send({
> +      from: this.actorID,
> +      type: "serviceWorkerRegistrationListChanged"

This is a workaround. A weird one, which shouldn't be needed.
It looks like one onListChanged isn't called correctly.
It should be the one from WorkerActorList which relates to the start/stop state.
ServiceWorkerActorList is more about it being registered or not.

So Ben's comment about detaching the debugger makes sense. It may be the reason why WorkerActorList state is confused. May be the worker debugger is never correctly detached and so onUnregister is never called, nor onListChanged (which ends up emitted this 'serviceWorkerRegistrationListChanged' message)

Comment 18

[Julian Descottes [:jdescottes]]

Benoit, any plans to work on this or should we unassign?

Comment 19

[Benoit Chabod [:bigben]]

Sorry Julian, I'm too busy and probably won't be working on this anytime soon.
I hope someone will be able to finish my patch / start from scratch and add this in the future.

Comment 20

[Julian Descottes [:jdescottes]]

Thanks for the quick answer Benoit, no worries :)

Comment 21

[Dhruvi Butti]

Hey, I found this bug interesting and want to work on this. Can you assign this to me?
Thank you,
Dhruvi

Comment 22

[Sutom Jeu]

With over 11 years of massage experience and over 10,000 massages performed, my skills and techniques are unmatched. My experience allows me to truly customize every massage session to meet the client’s specific need. Whether you just need to relax or need pain relief, my massage sessions produce results. I have helped numerous clients who suffer from lower back pain, neck pain, headaches, TMJ, sciatica, and many other ailments. My reputation speaks for itself with over 490 5-Star Google Reviews.

https://nextlevelmassages.com/