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.
Created attachment 8449391 [details] [diff] [review] Patch needed to fix linking of the AVOID_NSPR build
Attachment #8449391 - Flags: review?(benjamin)
Created attachment 8449393 [details] [diff] [review] Make NS_IsMainThread use its own TLS
Created attachment 8450259 [details] [diff] [review] Make NS_IsMainThread use its own TLS
Attachment #8450259 - Attachment is patch: true
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c571df9a85de remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7829c78348a4
Assignee: nobody → bjacob
Backed out per bug 774388 comment 58.
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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.
(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.
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.
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.
(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.
Ah, that's more concerning then. Benjamin, do we still think that NS_LogInit is the right place to SetMainThread?
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.
You need to log in before you can comment on or make changes to this bug.