1453003 - Occasional process crash with OSX crash report

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Dave Townsend [:mossop]]

Attachment: crash report

A couple of times I've restarted Firefox to apply an update and after the app restarts I see an OSX crash report. The main app stays up so it suggests to me that one of the content processes is crashing.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

The crash report shows that we're crashing in Thread 19, and here's that threads stack:

0   XUL                           	0x00000001117ad86c profiler_unregister_thread() + 588
1   XUL                           	0x000000010e37eaca nsProcess::Monitor(void*) + 266
2   libnss3.dylib                 	0x000000010e05a0cb _pt_root + 203
3   libsystem_pthread.dylib       	0x00007fff603c16c1 _pthread_body + 340
4   libsystem_pthread.dylib       	0x00007fff603c156d _pthread_start + 377
5   libsystem_pthread.dylib       	0x00007fff603c0c5d thread_start + 13

Comment 2

[Markus Stange [:mstange]]

It's crashing on this line: MOZ_RELEASE_ASSERT(CorePS::Exists());
https://hg.mozilla.org/mozilla-central/file/30d72755b1749953d438199456f1524ce84ab5e5/tools/profiler/core/platform.cpp#l3148

And the main thread is running TLS destructors, so we've already left the main() function, so it makes sense that the profiler state CorePS has already been destroyed.

This means that the nsProcess::Monitor thread is staying alive a lot longer than I thought.

I think we have two options:
 1. Ignore calls to process_unregister_thread() if the profiler has already been shut down.
 2. Or don't profile the nsProcess::Monitor thread. I think it doesn't do anything that's interesting for performance.

Nick, do you have any opinions on this?

I don't know why this crash wasn't intercepted by the Firefox crash reporter.

Comment 3

[Nicholas Nethercote [inactive]]

> I think we have two options:
>  1. Ignore calls to process_unregister_thread() if the profiler has already
> been shut down.
>  2. Or don't profile the nsProcess::Monitor thread. I think it doesn't do
> anything that's interesting for performance.

I think we should do 1. It's a general-purpose solution that will work if there are any other similar threads, and it doesn't require an ad hoc judgment about the profile-worthiness of the nsProcess::Monitor thread.

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1453003 - Ignore calls to profiler_unregister_thread() that happen after the main thread has shut down.

Review commit: https://reviewboard.mozilla.org/r/236322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236322/

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8967610 [details]
Bug 1453003 - Ignore calls to profiler_unregister_thread() that happen after the main thread has shut down.

https://reviewboard.mozilla.org/r/236322/#review242100

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd7869d5d03b
Ignore calls to profiler_unregister_thread() that happen after the main thread has shut down. r=njn

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b922c2a5667
Ignore calls to profiler_unregister_thread() that happen after the main thread has shut down. r=njn

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6afc861a45f
Backed out changeset fd7869d5d03b for browser-chrome failures at browser/components/preferences/in-content/tests/browser_fluent.js on a CLOSED TREE

Comment 9

[Cristina Coroiu [:ccoroiu]]

Sorry, wrong backout, I reland it

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1b922c2a5667