1347007 - Profiler registration creates too many nsThreads

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

I found this while debugging bug 1208957 and while fixing this doesn't fix the intermittent orange, I think we should fix this anyway. Currently [1], when we start an nsThread-based thread, we set the thread's name, register it with the profiler, and then try to register it with the thread manager.

The problem is that registering a thread with the profiler creates a ThreadInfo object, which forces creation of a *new* nsThread (with the stack: profiler_register_thread -> RegisterCurrentThread -> ThreadInfo::ThreadInfo -> NS_GetCurrentThread). This creates two competing nsThreads for the same PRThread.

By registering the new thread with the thread manager before registering it with the profiler, we can avoid this.

[1] http://searchfox.org/mozilla-central/rev/a5c2b278897272497e14a8481513fee34bbc7e2c/xpcom/threads/nsThread.cpp#463,466,470

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bug 1347007 - Register with the profiler after doing so for the ThreadManager.

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

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ecd69f8995c29a85bad59ec734a62f471aed02b

contains this change.

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8846900 [details]
Bug 1347007 - Register with the profiler after doing so for the ThreadManager.

https://reviewboard.mozilla.org/r/119888/#review121794

::: xpcom/threads/nsThread.cpp:473
(Diff revision 1)
>    nsThreadManager::get().RegisterCurrentThread(*self);
>  
>    mozilla::IOInterposer::RegisterCurrentThread();
>  
> +  if (!initData->name.IsEmpty()) {
> +    profiler_register_thread(initData->name.BeginReading(), &stackTop);

Ugh. So, AIUI, the problem is a mixing of layers. XPCOM threads are layered on top of NSPR threads, and this code is part of the XPCOM implementation, so it's using NSPR primitives (e.g. PR_GetCurrentThread()), but the profiler uses the XPCOM layer which is only half set up and problems ensue.

Can you add a comment here, something like "This must come after the call to nsThreadManager::RegisterCurrentThread(), because that call is needed to properly set up this thread as an nsThread, which profiler_register_thread() requires. See bug 1347007."

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8846900 [details]
Bug 1347007 - Register with the profiler after doing so for the ThreadManager.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119888/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c62935c570f
Register with the profiler after doing so for the ThreadManager. r=njn

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c62935c570f

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8846900 [details]
Bug 1347007 - Register with the profiler after doing so for the ThreadManager.

Approval Request Comment
[Feature/Bug causing the regression]: n/a
[User impact if declined]: Unknown
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Stepping through the new code shows it doing the right thing. I don't know of any functional changes this causes.
[String changes made/needed]: n/a

Comment 8

[Gerry Chang [:gchang]]

Hi :mrbkap, 
From the request information, I can't tell why we need this in Aurora54. There is no specified feature/bug causing the regression. Could you give us more details about the user impact if we don't take this in Aurora?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

To be honest, I don't know what user impact there is as a result of this bug. It is probably minimal.

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8846900 [details]
Bug 1347007 - Register with the profiler after doing so for the ThreadManager.

Given there is no significant user impact, I prefer to let this ride the train on 55 and won't fix in 54. Aurora54-.