1371127 - Use GetCurrentVirtualThread for MediaManager mMainThreadCheck

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

We need this for Quantum DOM, where there will be multiple "main" threads. See this comment for a description of GetCurrentVirtualThread:
http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/xpcom/threads/nsThreadUtils.h#1790-1825

Since this is a thread safety assertion, I think GetCurrentVirtualThread is the right function. Note that it will work even when there is no XPCOM thread manager. It's more at the NSPR level (although it's not actually part of NSPR).

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

This is WebRTC code, so forwarding to Jesup.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

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

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

r- without an API assurance that this is the behavior required of GetCurrentVirtualThread.  Update the comment to state that behavior (and the eventual implementation to match), and r+.

::: dom/media/MediaManager.cpp
@@ +3681,5 @@
>        target = GetMainThreadEventTarget();
>        if (NS_WARN_IF(!target)) {
>          NS_ASSERTION(false, "Mainthread not available; running on current thread");
>          // Ensure this really *was* MainThread (NS_GetCurrentThread won't work)
> +        MOZ_RELEASE_ASSERT(mMainThreadCheck == GetCurrentVirtualThread());

While the comment on this patch states that this will work when there's no XPCOM thread manager (late shutdown), the comment pointed to makes no such assertion, and there's no real code behind this yet (nor any tests or assertions that this behavior is guaranteed in the future).

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Also note that threadpools are typically shut down by that point, so the impl would have to guarantee the virtual thread id of a thread would outlive the pool - which I don't think it can.  The logic here is such that the test would be wrong in any case, unless somehow the thread-that-runs-shutdown is the same virtual ID as every "mainthread" thread.

What you really want here (and what it was testing) is a "is this on the virtual mainthread OR are we in post-nsThreadManager-shutdown?" method.  So I think r- even with the API assurance.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Randell Jesup [:jesup] from comment #3)
> Also note that threadpools are typically shut down by that point, so the
> impl would have to guarantee the virtual thread id of a thread would outlive
> the pool - which I don't think it can.  The logic here is such that the test
> would be wrong in any case, unless somehow the thread-that-runs-shutdown is
> the same virtual ID as every "mainthread" thread.
> 
> What you really want here (and what it was testing) is a "is this on the
> virtual mainthread OR are we in post-nsThreadManager-shutdown?" method.  So
> I think r- even with the API assurance.

I'm not sure I understand this comment. When we have multiple main threads, the implementation of GetCurrentVirtualThread will look like this:

1. if we're on one of the cooperative main threads (which we'll check via TLS), return the original main thread's PRThread
2. otherwise return PR_GetCurrentThread

Even if we have shut down the cooperative thread pool at this point, we'll still be on the original main thread, so earlier results returned from GetCurrentVirtualThread will still be valid.

I'm happy to add an additional comment for GetCurrentVirtualThread, but it sounds like you want more than that?

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

The test you modified is trying to determine if it's running on "MainThread", which in this case includes "running on MainThread after nsThreadManager shutdown".  As coded, I think even with the impl you describe that last part will fail, since it will return PR_CurrentThread, which may not match the earlier (pre-shutdow) virtual thread id it stored

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Randell Jesup [:jesup] from comment #5)
> The test you modified is trying to determine if it's running on
> "MainThread", which in this case includes "running on MainThread after
> nsThreadManager shutdown".  As coded, I think even with the impl you
> describe that last part will fail, since it will return PR_CurrentThread,
> which may not match the earlier (pre-shutdow) virtual thread id it stored

During shutdown, we'll be on the original main thread, and it will return the PRThread for that thread, which is the correct value.

Even if we were somehow still using cooperative threads during shutdown, the function would still return the original main thread's PRThread. Maybe you're worried that the TLS will become invalid or incorrect when we shut down the thread manager? MOZ_THREAD_LOCAL doesn't know anything about shutdown.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

Here is a new patch that includes a comment requiring the behavior we've discussed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef88b62fc004
Use GetCurrentVirtualThread() in MediaManager (r=jesup)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef88b62fc004