AbstractThread IsCurrentThreadIn() implementations make a lot of assumptions

NEW
Unassigned

Status

()

Core
XPCOM
P3
normal
a year ago
3 months ago

People

(Reporter: bkelly, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

a year ago
Consider:

  MOZ_ASSERT(NS_IsMainThread());

  bool in = AbstractThread::MainThread()->IsCurrentThreadIn();
  MOZ_ASSERT(in);

  nsCOMPtr<nsIThread> mt = do_GetMainThread();
  RefPtr<AbstractThread> other =
    AbstractThread::CreateXPCOMThreadWrapper(mt, true);

  // This asserts in XPCOMThreadWrapper::IsCurrentThreadIn()!
  in = other->IsCurrentThreadIn();

  MOZ_ASSERT(in);

This code should work in my opinion, but it does not in DEBUG builds.  The XPCOMThreadWrapper::IsCurrentThreadIn() has this assertion:

  bool in = PR_GetCurrentThread() == thread;
  MOZ_ASSERT(in == (GetCurrent() == this));

But this assumes the IsCurrentThreadIn() is only ever called from that particular instance of XPCOMThreadWrapper.  Its clearly possible to call it on the AbstractThread handle from other entry points to the xpcom thread.

I think we should probably change this code to:

  bool in = PR_GetCurrentThread() == thread &&
            GetCurrent() == this;

Both in the XPCOMThreadWrapper and in TaskQueue if/when it starts supporting general nsIEventTargets.
My thoughts:

For NS_DispatchToMainThread(runnable1),
runnables should see |AbstractThread::MainThread()->IsCurrentThreadIn() == false|.

For any |AbstractThread* at|,
at->IsCurrentThreadIn() should return true iff the runnable is executed as a consequence of at->Dispatch(runnable).

Since runnable1 is not executed as as a consequence of AbstractThread::MainThread()->Dispatch(runnable1), it should see |AbstractThread::MainThread()->IsCurrentThreadIn() == false|.

It also means:
  RefPtr<AbstractThread> at1 = AbstractThread::CreateXPCOMThreadWrapper(mt, true);
  RefPtr<AbstractThread> at2 = AbstractThread::CreateXPCOMThreadWrapper(mt, true);
are 2 different 'abstract' threads in concept even though they share the same underlying nsIThread.
(Reporter)

Comment 2

a year ago
Ok, I think the proposed code change in comment 0 would give us the semantics in comment 1.  Basically incorporate the `GetCurrent() == this` in the conditional instead of as a separate assertion.

Updated

3 months ago
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.