1740899 - Investigate deeply nested "nsThread::Shutdown: IPC Launch #1" event loop stacks

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[Jens Stutte [:jstutte]]

Like 5d4233b5-109a-4ccd-8fa1-e45150211111

The nested variant for now is happening only on Mac, it seems.

Comment 1

[Jens Stutte [:jstutte]]

This might just be another consequence of something going wrong with the annotation? See bug 1279293 comment 184. Also here I do not see any SpinEventLoopUntil on the stack.

Comment 2

[Jens Stutte [:jstutte]]

(In reply to Jens Stutte [:jstutte] from comment #1)

This might just be another consequence of something going wrong with the annotation? See bug 1279293 comment 184. Also here I do not see any SpinEventLoopUntil on the stack.

According to bug 1741131 comment 5 we think that the misalignment is caused by a time gap between synchronizing the annotations from the child to the parent process and the effective crash. This means that a deep stack like this looks still suspicious and we should continue the investigation, because at some point in time the nesting level we see here was real.

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: Stack trace of the main process

Comment 4

[Jens Stutte [:jstutte]]

The attachment Stack trace of the main process shows a sequence of:

1. Dispatch a nsThreadPool::Shutdown as runnable function to the main thread
2. Call nsThread::Shutdown for the first thread in the pool
3. Spin the event loop while waiting for that thread to shut down
4. Dispatch another nsThreadPool::Shutdown as runnable function to the main thread
5. Goto 2.

We clearly have no re-entrance check for a thread pool shutdown, so it seems easy to mitigate as such.

What puzzles me is, that we should have a re-entrance check inside nsThread::ShutdownInternal for each single thread. Still it seems from the thread name on XPCOMSpinEventLoopStack, that we run the event loop multiple times for the same thread.

Comment 5

[Jens Stutte [:jstutte]]

Attachment: Bug 1740899: Make nsThreadPool::Shutdown(WithTimeout) return an error on re-entrance. r?#xpcom-reviewers

Comment 6

[Jens Stutte [:jstutte]]

(In reply to Jens Stutte [:jstutte] from comment #4)

What puzzles me is, that we should have a re-entrance check inside nsThread::ShutdownInternal for each single thread. Still it seems from the thread name on XPCOMSpinEventLoopStack, that we run the event loop multiple times for the same thread.

A possible constellation that would explain the stack would be a thread pool with n Threads that all carry the same name. What would happen then is:

1. Dispatch a nsThreadPool::Shutdown as runnable function to the main thread
2. Call nsThread::Shutdown for the first thread in the pool
3. Spin the event loop while waiting for that thread to shut down
4. Dispatch another nsThreadPool::Shutdown as runnable function to the main thread
5. Call nsThread::Shutdown for the first thread in the pool, this is rejected
6. Call nsThread::Shutdown for the second thread in the pool
7. Spin the event loop while waiting for the second thread to shut down
8. Dispatch another nsThreadPool::Shutdown as runnable function to the main thread
9. Goto 4. and get to the next not-yet-shutdown thread

Still puzzling why this would happen only on Mac OS.

Comment 7

[Jens Stutte [:jstutte]]

BTW, such a scenario would also lead to a pseudo-parallel shutdown of the threads in a thread pool, which we wanted to avoid apparently by looping them sequentially. So the re-entrance check for nsThreadPool::Shutdown seems important to me in any case.

Comment 8

[Jens Stutte [:jstutte]]

I'll try to wrap up nika's input from a chat on matrix:

• In general, the nsThreadPool exit function is actually guarded against multiple shutdown entrances, because the mThreads list is cleared on each call, so we won't try to shut down the same thread multiple times. Still we want to provide explicit feedback to the caller on re-entrance instead of silently absorbing the call.
• In specific, the "IPC Launch" thread pool is only ever used on MacOS. On the other systems we are using a single, dedicated thread. This explains, why we see this behavior only on MacOS.
• Even more given that a SharedThreadPool isn't kept alive in global state we probably actually create a new one for every time we launch a child process, which should be optimized. We might just want to consider using the dedicated thread for "IPC Launch" also on MacOS.
• Each of those "IPC Launch" thread pools would contain (at least) one thread named "IPC Launch #1". If we end up creating new instances of them during late shutdown we might end up with a stack like we've seen here without being this necessarily a hang given that the various SpinEventLoopUntil would refer to different instances and thus state variables.
• We see the annotation for "IPC Launch" unexpectedly on the child process despite we know that it lives only in the parent process. Bug 1741131 might need some additional consideration, the doubt would be that we actually see the parent process' one here.

Comment 9

[Jens Stutte [:jstutte]]

There might be nothing more to investigate here, but that does not depend on the patch and I am not yet sure...

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72843551cbb7
Make nsThreadPool::Shutdown(WithTimeout) return an error on re-entrance. r=xpcom-reviewers,nika

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/72843551cbb7

Comment 12

[Jens Stutte [:jstutte]]

(In reply to Jens Stutte [:jstutte] from comment #8)

• Each of those "IPC Launch" thread pools would contain (at least) one thread named "IPC Launch #1". If we end up creating new instances of them during late shutdown we might end up with a stack like we've seen here without being this necessarily a hang given that the various SpinEventLoopUntil would refer to different instances and thus state variables.

Note to myself: It is actually not the "different instances and thus state variables" that make this work. It is the guarantee that the ack events that control that state will all be fired by every single thread shutdown and elaborated on the main thread. Even a multiple times nested event loop whose (equal) predicates use the same state would unwind correctly (with the sole condition that the state must be kept alive since then, best on the stack), IIUC.

(In reply to Jens Stutte [:jstutte] from comment #8)

• In specific, the "IPC Launch" thread pool is only ever used on MacOS. On the other systems we are using a single, dedicated thread. This explains, why we see this behavior only on MacOS.
• Even more given that a SharedThreadPool isn't kept alive in global state we probably actually create a new one for every time we launch a child process, which should be optimized. We might just want to consider using the dedicated thread for "IPC Launch" also on MacOS.

I filed bug 1742219 for this. And bug 1741131 does not block this investigation any more, too, so I'll close this bug.