1128091 - Add mfbt/Thread.h for easy "is on same thread as before" checks

Status: ASSIGNED

(Core :: MFBT, defect)

Description

[Markus Stange [:mstange]]

Attachment: patch

I wanted to add an assert of the style
MOZ_ASSERT(mOwningThread == NS_GetCurrentThread());
to PseudoStack.h and became very sad when it wouldn't compile in some parts of the tree that don't have MOZILLA_INTERNAL_API defined. (The specific file that blew up was media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp.)

This patch adds mfbt/Thread.h so you can do MOZ_ASSERT(mOwningThread == mozilla::ThisThread::Id()). It is modeled after std::this_thread::id().

Comment 1

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #0)
> I wanted to add an assert of the style
> MOZ_ASSERT(mOwningThread == NS_GetCurrentThread());
> to PseudoStack.h and became very sad when it wouldn't compile in some parts
> of the tree that don't have MOZILLA_INTERNAL_API defined. (The specific file
> that blew up was
> media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp.)

I realize now that this is a bad example as far as justification for Thread.h goes, because I can just fix my problem by shuffling code between some .h and .cpp files. But I think the patch has value anyway.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8557367 [details] [diff] [review]
patch

Review of attachment 8557367 [details] [diff] [review]:
-----------------------------------------------------------------

So, sadly, there's already a bunch of work done on this in bug 991710, that floundered for lack of time on my part.  And you haven't addressed all the complexity people wanted for this.

Specifically people wanted universally unique thread ids, which GetCurrentThreadId() does not give (nor, I believe, is pthread_self() guaranteed to give).  The approach that seems best to me is just to use a ThreadLocal<uintptr_t> to store each thread's id, then initialize it from an Atomic<uintptr_t> that increments until it's rolled around.  Unfortunately once you overflow you lose uniqueness guarantees.  Maybe we should just assume that's impossible?  And crash if it happens?  I tried a more involved approach in the patch there, that I guess is kind of overkill, for a 32-bit-only problem.

Anyway.  I'll copy my patch over to this bug, then you can finish it up.  I guess we should probably do ThreadId for now and deal with having a Thread class and Thread::id for later.  And the oddities of having an always-64-bit counter variable, we can just not deal with, because on 32-bit I think it probably just requires a lock (which just isn't worth it).

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Over-architected alternative that doesn't have tombstone-value issues

Probably you can just remove a bunch of nonsense complexity in this, then resubmit it and I (or someone else, depending how much we care about self-review, tho older versions of that patch were reviewed by others) can quickly stamp it.