1072780 - MediaStreamGraph cleanup from refactor

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

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

Bug 848954 refactored MediaStreamGraph and introduced GraphDriver; we need to land a number of cleanups for intermittents and race conditions.  CurrentDriver() from off-MSG-thread is a common problem.

Also found that mInCallback was getting set and cleared immediately due to how the DebugOnly was set up.

First patch here should underly bug 1072775 since it depends on InCallback()

Comment 1

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

Attachment: patch 1 - clean up CurrentDriver() use off-MSG-thread; fix InCallback()

Comment 2

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

Attachment: patch 3 - Use atomics for EnsureNextIteration to close races around CurrentDriver

patch 2 is bug 1072775 currently - this is all WIP

Comment 3

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

Comment on attachment 8495053 [details] [diff] [review]
patch 3 - Use atomics for EnsureNextIteration to close races around CurrentDriver

roc - feedback appreciated at trying to streamline and make EnsureNextIteration safe without heavy locks (now that drivers can change under us and also sleep).

Implements roughly this:

[01:26]	jesup	considered an Atomic<bool> for EnsureNextIteration to avoid the need for CurrentDriver(), but the need to do a WakeUp() if the driver is asleep made that unappealing for now

[01:30]	jesup	Though I imagine something like "(atomic) mOneMoreIteration = true; if ((atomic) mIsAsleep) { MonitorAutoLock lock(mMonitor); CurrentDriver()->WakeUp; }" might work. Any races remaining are just un-needed WakeUps

Comment 4

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

https://tbpl.mozilla.org/?tree=Try&rev=c709d5e64095 with both plus bug 1072775 and bug 1033066

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8495053 [details] [diff] [review]
patch 3 - Use atomics for EnsureNextIteration to close races around CurrentDriver

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

Why can't we use the graph monitor here?

Comment 6

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

> Why can't we use the graph monitor here?

We certainly can; the previous patch lock the monitor around accesses to CurrentDriver() from other threads - but there are a lot (and even some SetCurrentDrivers() from other threads in the 'Revive()' case), so it gets messy and increases the chance that MSG threads (or system audio callback threads!) will end up blocked on a monitor held by a thread that won't be scheduled for a bit.  Atomics introduce memory barriers but don't block runnability for the MSG/callback.

mNeedAnotherIteration seems like a good use of an Atomic: lots of setters to true; once reader/setter-to-false, and a writer-reader race is ok (we just do an extra iteration, since we set false before starting an iteration, at the end of WaitForNextIteration() with this patch.)  Very rarely do we need to do the heavier-weight WakeUp().

Comment 7

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

Attachment: patch 4 - Fix up Revive() to not trigger assertions, and also to avoid Init() (blocking) on MainThread

Comment 8

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

https://tbpl.mozilla.org/?tree=Try&rev=0edb7bc464de
Looks pretty green

Comment 9

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

Attachment: patch 1 - clean up CurrentDriver() use off-MSG-thread; fix InCallback()

Comment 10

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

Attachment: patch 3 - Fix up Revive() to not trigger assertions, and also to avoid Init() (blocking) on MainThread

Comment 11

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

Attachment: patch 4 - Use atomics for EnsureNextIteration to close races around CurrentDriver

Comment 12

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

Comment on attachment 8495529 [details] [diff] [review]
patch 1 - clean up CurrentDriver() use off-MSG-thread; fix InCallback()

The primary thing in this patch is support for controlling the use of CurrentDriver() from non-MSG threads, especially for a common pattern of "do something to a stream, graph->CurrentDriver()->EnsureNextIteration();"

The danger was that CurrentDriver() wasn't locked, could change on you, and was a TSAN violation to use like this.

Also some other bits of cleanup

Comment 13

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

Comment on attachment 8495534 [details] [diff] [review]
patch 4 - Use atomics for EnsureNextIteration to close races around CurrentDriver

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

This one is optional, but I think is an improvement

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8495533 [details] [diff] [review]
patch 3 - Fix up Revive() to not trigger assertions, and also to avoid Init() (blocking) on MainThread

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

::: content/media/MediaStreamGraphImpl.h
@@ +440,4 @@
>     */
>    void SetCurrentDriver(GraphDriver* aDriver) {
> +    MOZ_ASSERT(mDriver->OnThread() ||
> +               (mMonitor.AssertCurrentThreadOwns(), true));

This is a bit obscure. Please rewrite this as
  if (!mDriver->OnThread()) {
    mMonitor.AssertCurrentThreadOwns();
  }

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8495534 [details] [diff] [review]
patch 4 - Use atomics for EnsureNextIteration to close races around CurrentDriver

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

I'm not crazy about this but I'm not against it either.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/649d337d331c
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f42231c8684
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2138c7f8569

Note: to uplift these, we probably need bug 1072775 which they landed with.

Comment 17

[Daniel Holbert [:dholbert]]

Backed out:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8faa128dff
for fatal assertion failures in Linux64 tests like:
Assertion failure: lock->locked && pthread_equal(lock->owner, pthread_self()), at /builds/slave/m-in-l64-d-0000000000000000000/build/nsprpub/pr/src/pthreads/ptsynch.c:226

e.g.
https://tbpl.mozilla.org/php/getParsedLog.php?id=49067104&tree=Mozilla-Inbound

Comment 18

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

Got the boolean backwards when applying the review comment:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0860a1ab3506
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c741cf2e57
https://hg.mozilla.org/integration/mozilla-inbound/rev/765e4c810ce0

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/649d337d331c
https://hg.mozilla.org/mozilla-central/rev/3f42231c8684
https://hg.mozilla.org/mozilla-central/rev/f2138c7f8569
https://hg.mozilla.org/mozilla-central/rev/0860a1ab3506
https://hg.mozilla.org/mozilla-central/rev/34c741cf2e57
https://hg.mozilla.org/mozilla-central/rev/765e4c810ce0

Comment 21

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

Comment on attachment 8495529 [details] [diff] [review]
patch 1 - clean up CurrentDriver() use off-MSG-thread; fix InCallback()

(for all patches, and they need to land with bug 1072775):

Approval Request Comment
[Feature/regressing bug #]: bug 848954

[User impact if declined]: crashes, test oranges, possible sec issues

[Describe test coverage new/current, TBPL]: some known oranges are likely due to this.  Adds considerable thread-safety assertions, which caught a bug immediately on landing (which someone had been working and had a fix for, but took them far longer to find after it caused an odd crash)

[Risks and why]: fairly low.  Cleans up locking around a number of pointers; of course in theory it may have missed something, but it added asserts about safety/locking.  Main risk would be missing a case where the assert is too draconian (had one of those on the initial landing) and causing an intermittent orange.

[String/UUID change made/needed]: none.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8495529 [details] [diff] [review]
patch 1 - clean up CurrentDriver() use off-MSG-thread; fix InCallback()

Aurora+

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8495533 [details] [diff] [review]
patch 3 - Fix up Revive() to not trigger assertions, and also to avoid Init() (blocking) on MainThread

Aurora+

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8495534 [details] [diff] [review]
patch 4 - Use atomics for EnsureNextIteration to close races around CurrentDriver

Aurora+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6fc7dace7ec9
https://hg.mozilla.org/releases/mozilla-aurora/rev/e3397237012d
https://hg.mozilla.org/releases/mozilla-aurora/rev/5ef997137570