Closed Bug 1371127 Opened 3 years ago Closed 3 years ago

Use GetCurrentVirtualThread for MediaManager mMainThreadCheck

Categories

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

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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).
Attachment #8875555 - Flags: review?(cpearce)
Comment on attachment 8875555 [details] [diff] [review]
patch

This is WebRTC code, so forwarding to Jesup.
Attachment #8875555 - Attachment is patch: true
Attachment #8875555 - Flags: review?(cpearce) → review?(rjesup)
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).
Attachment #8875555 - Flags: review?(rjesup) → review-
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.
(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?
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
(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.
Attached patch patch v2Splinter Review
Here is a new patch that includes a comment requiring the behavior we've discussed.
Attachment #8875555 - Attachment is obsolete: true
Attachment #8875826 - Flags: review?(rjesup)
Attachment #8875826 - Flags: review?(rjesup) → review+
Component: Audio/Video → Audio/Video: Playback
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef88b62fc004
Use GetCurrentVirtualThread() in MediaManager (r=jesup)
https://hg.mozilla.org/mozilla-central/rev/ef88b62fc004
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.