1033358 - Make NS_IsMainThread use its own separate TLS, so it is working from early init to the end of shutdown, regardless of nsThreadManager

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Currently NS_IsMainThread either relies on nsThreadManager, or uses a TLS, gTLSThreadID, that is initialized and torn down together with nsThreadManager. Consequently, NS_IsMainThread tends to incorrectly return false when called before initialization or after shutdown of nsThreadManager. We have use cases for relying on NS_IsMainThread very early and very late, as it is often useful to assert that we are on the main thread, see bug 1028383.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Patch needed to fix linking of the AVOID_NSPR build

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Make NS_IsMainThread use its own TLS

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Make NS_IsMainThread use its own TLS

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c571df9a85de
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7829c78348a4

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Backed out per bug 774388 comment 58.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Thanks for handling the backout of this push. I think there's no way that the two patches on this one bug could be responsible for the failures, so I relanded them:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/db75178f2e17
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d0addbc7cc96

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/db75178f2e17
https://hg.mozilla.org/mozilla-central/rev/d0addbc7cc96

Comment 8

[Jim Mathies [:jimm]]

We should add some commenting around ScopedLogging in nsAppRunner indicating this is where the main thread gets set. Better still, this shouldn't happen in an innocuous call like NS_LogInit, we should call NS_SetMainThread directly from nsAppRunner.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #8)
> We should add some commenting around ScopedLogging in nsAppRunner indicating
> this is where the main thread gets set. Better still, this shouldn't happen
> in an innocuous call like NS_LogInit, we should call NS_SetMainThread
> directly from nsAppRunner.

I just noticed this is called even earlier in nsBrowserApp, again with little to no fan fare about what it does.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

The problem was that we would then also need to call that from XRE_InitChildProcess at least; that suggested that going that route could end in a big whack-a-mole party; NS_LogInit at least was guaranteed to be always called. :bsmedberg would know better than me though.

I do agree that this is worth logging.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Oh, I see, you did hit a case where NS_LogInit wasn't called. But then, wasn't a bunch of other logging stuff also not working?

Anyway, just calling this in nsBrowserApp would not be enough by itself because of XRE_InitChildProcess.

Comment 12

[Jim Mathies [:jimm]]

(In reply to Benoit Jacob [:bjacob] from comment #11)
> Oh, I see, you did hit a case where NS_LogInit wasn't called. But then,
> wasn't a bunch of other logging stuff also not working?
> 
> Anyway, just calling this in nsBrowserApp would not be enough by itself
> because of XRE_InitChildProcess.

I hit the case where NS_LogInit was called on a non-main thread. The main thread that enters main() in nsBrowserApp is not the gecko main thread with metro. The main thread is created later.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Ah, that's more concerning then. Benjamin, do we still think that NS_LogInit is the right place to SetMainThread?

Comment 14

[Benjamin Smedberg]

Yes it is. The metro code needs to be fixed not to call NS_LogInit from a different thread, because NS_LogInit is not a threadsafe function.