The issue also applies to the method GetCurrentThreadEventTarget()

GetCurrentThreadSerialEventTarget() will always return the currently nsThread regardless of which SerialEventTarget the current runnable is using

This is particularly problematic as we are moving to TaskQueue such as returned by NS_CreateBackgroundTaskQueue in place of NS_NewNamedThread .

Say you have the code:

  RefPtr<TaskQueue> tq1 =
      new TaskQueue(GetMediaThreadPool(MediaThreadType::PLAYBACK), false);
  Unused << tq1->Dispatch(NS_NewRunnableFunction(
      "TestTaskQueue::TestCurrentSerialEventTarget::TestBody", [tq1]() {
        nsCOMPtr<nsISerialEventTarget> thread =
            GetCurrentThreadSerialEventTarget();
        EXPECT_EQ(thread, tq1);  <---- boom
      }));
  tq1->BeginShutdown();
  tq1->AwaitShutdownAndIdle();

You would expect GetCurrentThreadSerialEventTarget() to return the address of tq1 and not the actual nsThread in the ThreadPool the TaskQueue is actually using.

Often code will use the following pattern:

MethodReturningMozPromise()->Then(GetCurrentThreadSerialEventTarget(), {...});

If the rest of the code is running in a TaskQueue; the assumption that a new task dispatched will be run after all the pending tasks is now broken.

A lot of class constructors (particularly in code using IPC) do things like:

class A {
  nsCOMPtr<nsISerialEventTarget> mThread; 
public:
  A() : mThread(GetCurrentThreadSerialEventTarget())
}

If class A was constructed from a runnable running in a TaskQueue ; there will be some implied assumption related to task ordering ; which is now broken because A will use the single nsThread the code was running on at the time.

The documentation of GetCurrentThreadEventTarget
https://searchfox.org/mozilla-central/rev/0688ffdef223dac527c2fcdb25560118c4e4df51/xpcom/threads/nsThreadUtils.h#1787
says:
"// These functions should be used in preference
// to the nsIThread-based NS_Get{Current,Main}Thread functions since they will
// return more useful answers in the case of threads sharing an event loop."

Except that is not correct, it will only ever return the actual nsThread in use as the two implementations are:

  nsCOMPtr<nsIThread> thread;
  nsresult rv = NS_GetCurrentThread(getter_AddRefs(thread));
  if (NS_FAILED(rv)) {
    return nullptr;
  }

  return thread->EventTarget();
}

nsISerialEventTarget* GetCurrentThreadSerialEventTarget() {
  nsCOMPtr<nsIThread> thread;
  nsresult rv = NS_GetCurrentThread(getter_AddRefs(thread));
  if (NS_FAILED(rv)) {
    return nullptr;
  }

  return thread->SerialEventTarget();
}

In bug 1637433, I replaced some nsThread with a nsISerialEventTarget returned by NS_CreateBackgroundTaskQueue and got some unexpected failures, those were the direct results of newly introduced races with the runnables dispatched.

GetCurrentThreadSerialEventTarget() and GetCurrentThreadEventTarget() should be renamed to
GetCurrentSerialEventTarget and GetCurrentEventTarget() respectively.

In the future, we may want to extend GetCurrentThreadSerialEventTarget to return the actual nsISerialEventTarget used to dispatch the task.

Before P1, GetCurrentThreadSerialEventTarget would have always returned the same data as NS_GetCurrentThread, making the comment incorrect Now it will properly return the running TaskQueue if any.

This change of name more clearly exposes what they are doing, as we aren't always dealing with threads directly; but a nsISerialEventTarget

Depends on D80353

This will allow to remove AbstractThread::Current() as GetCurrentSerialEventTarget TLS value will be set whenever a task dispatched on the XPCOMThreadWrapper is run.

Depends on D80354

nsTimerEvent::GetName calls nsTimerImpl::GetName; which is thread safe.

The mTimer member itself would be suspicious; however the RELEASE_ASSERT itself access mTimer at all time.

When wrapped with SerialEventTargetGuardRunnableWrapper in the follow-up change; we access the name in order to propagate it later; and is called in TimerThread::PostTimerEvent during the Dispatch, and this is done with the monitor locked.

Depends on D80355

We introduce SerialEventTargetGuardRunnableWrapper object which will wrap a Runnable and set the CurrentSerialEventTarget TLS for the duration of the runnable's run and so that it returns the right value when called.

We use it for DocGoup and message_loop's event target wrapper.

Depends on D80356

Depends on D80357

partially landing because otherwise I get constant rebase failures due to the broad method name change.

https://hg.mozilla.org/mozilla-central/rev/00687d06a8a6
https://hg.mozilla.org/mozilla-central/rev/48eaa84c206d
https://hg.mozilla.org/mozilla-central/rev/fbd4c4309a75
https://hg.mozilla.org/mozilla-central/rev/36ea162317d7

Will take P5 in a new bug

Comment on attachment 9157924 [details]
Bug 1637500 - P5. Have GetCurrentSerialEventTarget returns the actual target in use. r?froydnj,farre

Revision D80357 was moved to bug 1647958. Setting attachment 9157924 [details] to obsolete.