Closed Bug 1498697 Opened 2 years ago Closed 2 years ago

Opening the Browser Toolbox on macOS creates a subprocess_worker_unix.js worker that keeps polling an fd even after the toolbox is closed

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: mconley, Assigned: Gijs)

References

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file)

STR:

1) Restart the browser
2) Go to about:debugging, click on Workers on the left, and scroll down to Other Workers
3) No subprocess_worker_unix.js should be there
4) Start up the Browser Toolbox
5) Note that the subprocess_worker_unix.js is now there
6) Shut down the Browser Toolbox
7) Note that the subprocess_worker_unix.js sticks around
8) Also note that periodically, about:performance notes that the subprocess_worker_unix.js consumes some CPU cycles every few seconds. I believe this is because it's polling a file descriptor.

We should shut this thing down when we don't need it, shouldn't we?
Whiteboard: [fxperf]
Looks like :aswan knows this code.

From inspection, it looks like we keep calling poll() here: https://searchfox.org/mozilla-central/source/toolkit/modules/subprocess/subprocess_worker_unix.js#562-567 which uses the POLL_TIMEOUT (const hardcoded to 5 seconds), even after the browser toolbox calls proc.kill() in https://searchfox.org/mozilla-central/rev/c9272ef398954288525e37196eada1e5a93d93bf/devtools/client/framework/ToolboxProcess.jsm#358 . Seems we only set `running` to false if someone calls io.shutdown(), which only happens if there's an error polling the file descriptor, which seems to not be happening here?

This should be straightforward to address, and not polling will help energy impact for folks using either the browser toolbox or, AFAICT, native messaging from add-ons (which I assume will have the same issue, though I didn't check this... perhaps browser toolbox is "doing things wrong" in terms of using Subprocess.jsm, but it was all written by the same people, looks like, and the subprocess.jsm doesn't seem to provide examples in terms of how to dispose of the process once you're done with it, so...)
Component: General → Async Tooling
Flags: needinfo?(aswan)
Product: DevTools → Toolkit
Whiteboard: [fxperf] → [fxperf:p2]
I haven't looked at this code in some time, but from a quick refresh, I think Gijs is right that this should be straightforward to address within the "io" object defined in subprocess_worker_unix.js.  Kris originally wrote this code, if there's some non-obvious reason that won't work, I have no doubt he will tell us...

For what its worth, I believe proc.kill() is the proper way to end a process created from Subprocess.call().  Docs are here:
https://firefox-source-docs.mozilla.org/toolkit/modules/subprocess/toolkit_modules/subprocess/index.html
Flags: needinfo?(aswan)
Is a similar patch needed for toolkit/modules/subprocess/subprocess_worker_win.js?
(In reply to Florian Quèze [:florian] from comment #4)
> Is a similar patch needed for
> toolkit/modules/subprocess/subprocess_worker_win.js?

Hm, yeah. I also realized that we use the running flag on shutdown to destroy the signal thingy for the worker, and because we reuse the worker to spawn multiple processes, I decided to use a separate bool for the polling stuff.

That said, I'm also seeing quite a bit of activity on this process while the browser toolbox is up, with the browser at rest. I dunno if that's related to the same thing, but if it is, I'm confused why the polling is so expensive, and if it is indeed so expensive, if we really need to do it...
(In reply to :Gijs (he/him) from comment #5)

> That said, I'm also seeing quite a bit of activity on this process while the
> browser toolbox is up, with the browser at rest. I dunno if that's related
> to the same thing, but if it is, I'm confused why the polling is so
> expensive, and if it is indeed so expensive, if we really need to do it...

about:performance showing this worker as high energy impact is a bug of the performance counters. I tried to explain in bug 1497794 comment 1.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/70fd3d40a484
stop polling when there's nothing to poll, restart when processes are re-added, r=kmag
(In reply to :Gijs (he/him) from comment #1)
> This should be straightforward to address, and not polling will help energy
> impact for folks using either the browser toolbox

For what it's worth, I doubt this will have a measurable impact on energy usage. Blocking on poll() is more or less free, and when there's nothing to poll, we just trampoline back to the event queue once every 5 seconds so we have a chance to run GC and such.

> or, AFAICT, native messaging from add-ons

I doubt this will affect native messaging much, either. Most extensions should be using stateful native messaging, which means the process they start should generally stay alive for the entire browser session. For extensions using stateless messaging, it might make a small difference.
https://hg.mozilla.org/mozilla-central/rev/70fd3d40a484
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Assignee: nobody → gijskruitbosch+bugs
You need to log in before you can comment on or make changes to this bug.