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

RESOLVED FIXED in mozilla33

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Other Branch
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
(Assignee)

Comment 1

4 years ago
Created attachment 8449391 [details] [diff] [review]
Patch needed to fix linking of the AVOID_NSPR build
Attachment #8449391 - Flags: review?(benjamin)
(Assignee)

Comment 2

4 years ago
Created attachment 8449393 [details] [diff] [review]
Make NS_IsMainThread use its own TLS
Attachment #8449393 - Flags: review?(benjamin)

Updated

4 years ago
Attachment #8449391 - Flags: review?(benjamin) → review+
(Assignee)

Comment 3

4 years ago
Created attachment 8450259 [details] [diff] [review]
Make NS_IsMainThread use its own TLS
Attachment #8449393 - Attachment is obsolete: true
Attachment #8449393 - Flags: review?(benjamin)
Attachment #8450259 - Flags: review?(benjamin)
(Assignee)

Updated

4 years ago
Attachment #8450259 - Attachment is patch: true

Updated

4 years ago
Attachment #8450259 - Flags: review?(benjamin) → review+
(Assignee)

Comment 6

4 years ago
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
https://hg.mozilla.org/mozilla-central/rev/db75178f2e17
https://hg.mozilla.org/mozilla-central/rev/d0addbc7cc96
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

Updated

4 years ago
Blocks: 1042708

Comment 8

4 years ago
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

4 years ago
(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.

Comment 12

4 years ago
(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?
Flags: needinfo?(benjamin)

Comment 14

4 years ago
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.
Flags: needinfo?(benjamin)
Blocks: 1139361
You need to log in before you can comment on or make changes to this bug.