1724337 - [meta] Improve browser shutdown handling in ProcessWatcher

Status: NEW

(Core :: IPC, enhancement)

Description

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

ProcessWatcher, the interface for handling child processes that should be terminating, has separate implementations for Windows and Unix, but the basic idea is:

• if the force parameter is false, we'll wait indefinitely and block parent process shutdown; this is used for builds that need processes to exit normally for memory leak detection
• if the force parameter is true, the process is forcefully terminated after a short (2s) timeout; this is used on release builds (and in ContentParent::KillHard in all cases)

But there are a some problems, and discussion on Matrix turned up some more:

1. In the force == true case, if the parent exits while the timeout is pending, we don't try to kill the process or otherwise ensure that it isn't hanging.
2. The hook used for “on shutdown”, MessageLoop::WillDestroyCurrentMessageLoop on the IPC I/O thread, generally doesn't happen on release builds: we terminate the parent process before that stage of shutdown. It's been suggested to use the xpcom-shutdown-threads observable instead, but that's async, which leads to:
3. Blocking shutdown should use the nsIAsyncShutdown framework instead of actually blocking a thread.
4. The timeouts use PendingTask via PostDelayedTask, which is deprecated; nsITimer should be used instead. (We also have DelayedRunnable which is built on nsITimer and appears to be the backend for PostDelayedTask if used on an XPCOM thread; this needs more investigation.)
5. Related to the previous item, trying to post delayed tasks after XPCOM has shut down timer processing is unhelpful (and could possibly cause hangs in some cases?); moving the “on shutdown” point earlier by fixing item #2 would ensure that can't happen.

Item #1 is more of a defect than an enhancement, and I think it could be fixed without pulling in all of items #2/#3 by using a sufficiently early observer notification to kill any pending forced processes and leaving the shutdown blocking as it currently is. But ideally that whole cluster of issues should be fixed. Item #4 is somewhat separate and could possibly be left to whoever tries to get rid of the PendingTask machinery.

Maybe this would make more sense as a meta-bug, but mainly I wanted to get these things written down somewhere persistent.

Comment 1

[Jens Stutte [:jstutte]]

Yes, I think we need this as meta bug. We see a series of intermittents where we apparently hang in WillDestroyCurrentMessageLoop. There are some sparse analysis here and there and I'd like to bring them all together here.