Closed Bug 1364821 Opened 3 years ago Closed 3 years ago

Ambiguous AbstractThread::GetCurrent() on the main thread

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: jwwang, Assigned: billm)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

We have [1] setting sCurrentThreadTLS on the main thread. We also have [2] doing that too.

So AbstractThread::GetCurrent() when called on the main thread, could return a MessageLoopAbstractThreadWrapper or an EventTargetWrapper depending on the initialization order.

[1] http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/ipc/glue/MessageLoopAbstractThreadWrapper.h#31
[2] http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/xpcom/threads/AbstractThread.cpp#275

In fact, setting sCurrentThreadTLS [3] is suspicious too which should return the AbstractThread the caller is currently running in [4].

[3] http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/xpcom/threads/AbstractThread.cpp#304
[4] http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/xpcom/threads/AbstractThread.h#40

For a task dispatched via nsIThread::Dispatch(), GetCurrent() returns non-null due to the setup of [3] despite the fact the task is not running in any AbstractThread.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #0)
> We have [1] setting sCurrentThreadTLS on the main thread. We also have [2]
> doing that too.
It seems that setting in MessageLoopAbstractThreadWrapper only happened off the main thread if AbstractThread::InitMainThread() always be done earlier:
http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/ipc/glue/MessageChannel.cpp#776-780
http://searchfox.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#861-865
I think we should set sCurrentThreadTLS only when a task is run by an AbstractThread like TaskQueue does.

[1] http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/xpcom/threads/TaskQueue.h#165

Then GetCurrent() will return non-null when the caller is running in the context of an AbstractThread, and null if it is not running in any AbstractThread.

However, there is one problem where some code uses state-mirroring outside the context of AbstractThread and state-mirroring relies on AbstractThread and its tail dispatcher to work.

This is especially true for the media element which will use state-mirroring in response to a play() call from the JS code and the caller is not running in the context of an AbstractThread.

To overcome this problem we have this setup [1] so the caller can pretend it is running in the context of an AbstractThread which however results in the ambiguity mentioned above.

[1] http://searchfox.org/mozilla-central/rev/484d2b7f51b7aed035147bbb4a565061659d9278/xpcom/threads/AbstractThread.cpp#275
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #1)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #0)
> > We have [1] setting sCurrentThreadTLS on the main thread. We also have [2]
> > doing that too.
> It seems that setting in MessageLoopAbstractThreadWrapper only happened off
> the main thread if AbstractThread::InitMainThread() always be done earlier:
> http://searchfox.org/mozilla-central/rev/
> 484d2b7f51b7aed035147bbb4a565061659d9278/ipc/glue/MessageChannel.cpp#776-780
> http://searchfox.org/mozilla-central/source/ipc/glue/MessageChannel.cpp#861-
> 865

That's what I said, it depends on a particular initialization order to work.
To resolve the ambiguity, I propose the concept of 'AbstractThread context':

1. A task dispatched via AbstractThread::Dispatch() will run in its context.
2. All code happen in-between AbstractThread::EnterContext() and AbstractThread::ExitContext() will run the context of the AbstractThread.

2 is used to solve the problem in comment 2 where it is not feasible to do a Dispatch() in order to run some code in the right context. It provides a way to enter the context of an AbstractThread synchronously. It also makes it possible to support tail dispatching on all threads without the help of the appshell-specific stable state.

Then we will be able to remove the setup of sCurrentThreadTLS and GetCurrent() will unambiguously return non-null only when the caller is running in some AbstractThread context.
I'm not sure I understand the distinction between AbstractThread contaxt and non-AbstractThread context.

The MessageLoopAbstractThreadWrapper is created to make using MozPromise easier in non-main-thread IPC context. I think we placed MozPromise in xpcom/ because we want MozPromise to be used not only in media code. In order to do that we should have a sane default for the MozPromise to operate on so different code in gecko can expect consistent order of execution. See also bug 1361607.

If you mean that code not running in a AbstractThread (for whatever it means) should not use GetCurrent(), maybe we should assert that and IPC code should provide a TaskQueue getter method to replace AbstractThread::GetCurrent().
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #5)
> If you mean that code not running in a AbstractThread (for whatever it
> means) should not use GetCurrent(), maybe we should assert that and IPC code
> should provide a TaskQueue getter method to replace
> AbstractThread::GetCurrent().

No. I mean for some code that doesn't run in any AbstractThread context, it can call Enter/ExitContext for GetCurrent() and tail dispatcher to work correctly (and thus fixing the problem in comment 2).
See Also: → 1364841
Depends on: 1365483
Is this what you were thinking of, JW? I don't know this code at all, so it's difficult for me to know all the places where we need an AbstractThread on the main thread. So I just removed this line:
http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/xpcom/threads/AbstractThread.cpp#318
Then I added an AutoEnter call to all the places where we crash. It seems like MediaDecoder is the easiest place to do this.
Attachment #8892676 - Flags: feedback?(jwwang)
Comment on attachment 8892676 [details] [diff] [review]
AbstractThread context patch

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

I have similar patches a while ago which somehow I didn't have time to finish. (Bug 1364821. P1 ~ P4)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7699d28c9c6f0089de07396cb9bc21ae905d488

My patches went as far as removing the requirement of appshell-specific stable state (P3) to make tail dispatching work on non-main threads.

I think for now fixing the issue on the main thread is good enough.
Attachment #8892676 - Flags: feedback?(jwwang) → feedback+
Comment on attachment 8892676 [details] [diff] [review]
AbstractThread context patch

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

It looks like we changed different places. I have a green try run, but I can add more call sites to match your patch if you think it makes sense.
Attachment #8892676 - Flags: review?(jwwang)
Comment on attachment 8892676 [details] [diff] [review]
AbstractThread context patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +3155,5 @@
>  
>  void MediaDecoderStateMachine::SetVideoDecodeMode(VideoDecodeMode aMode)
>  {
> +  MOZ_ASSERT(NS_IsMainThread());
> +  AbstractThread::AutoEnter context(mAbstractMainThread);

This should be moved to MediaDecoder::UpdateVideoDecodeMode() which is the only caller of MediaDecoderStateMachine::SetVideoDecodeMode().

::: xpcom/threads/AbstractThread.h
@@ +128,5 @@
> +  {
> +    AutoEnter(AbstractThread* aThread)
> +    {
> +      mLastCurrentThread = sCurrentThreadTLS.get();
> +      sCurrentThreadTLS.set(aThread);

Not sure if many calls to .set() will hurt the performance. It might be prudent to early return when |sCurrentThreadTLS.get() == aThread|.
Attachment #8892676 - Flags: review?(jwwang) → review+
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e19186dac46
Introduce AbstractThread::AutoEnter and remove main thread's AbstractThread::GetCurrent() (r=jwwang)
https://hg.mozilla.org/mozilla-central/rev/4e19186dac46
https://hg.mozilla.org/mozilla-central/rev/397cdaf2f4a0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee: nobody → wmccloskey
Depends on: 1387702
You need to log in before you can comment on or make changes to this bug.