1436267 - GraphImpl()->CurrentDriver() == aPreviousDriver assertion failure in SetGraphTime()

Status: VERIFIED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Description

[Karl Tomlinson (:karlt)]

https://crash-stats.mozilla.com/report/index/2689e0c0-5e2d-4999-9294-663810180205
is an NT example of a CurrentDriver() == aPreviousDriver assertion failure in
SetGraphTime() from FallbackToSystemClockDriver() on cubeb error in
AudioCallbackDriver::Stop().  There is also an MSG nsThread waiting for an
event.

If failing to stop the cubeb stream, we don't want to start a fallback thread
(because the intention is to stop the thread).

Usually, mShouldFallbackIfError has been reset to false in DataCallback(),
before Stop() is called.  However, if fallback to a system clock driver due to
cubeb error has already occurred, then mShouldFallbackIfError does not get
reset, and Stop() is still called.

https://crash-stats.mozilla.com/report/index/67bdf274-ea6f-44db-aff8-919860180204
is a similar crash on Linux, though I don't see a second MSG nsThread.

Comment 1

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68754f9032b0edefcab881684751617e4a4d0e32

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: bug 1436267 fallback from audio callback to system clock driver on error only once

Usually, mShouldFallbackIfError has been reset to false in DataCallback()
before Stop() is called.  However, if fallback to a system clock driver due to
cubeb error had already occurred, then mShouldFallbackIfError would not have
been reset, and Stop() is still called.  With mShouldFallbackIfError still
true, a cubeb error in stop would have created another fallback thread.

I expect that resetting mShouldFallbackIfError in Stop() would also be an
effective alternative solution, but resetting on StateCallback() happens
earlier, which would be an advantage if any additional errors could possibly
be reported to StateCallback().

Review commit: https://reviewboard.mozilla.org/r/218354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218354/

Comment 3

[Paul Adenot (:padenot)]

Comment on attachment 8948929 [details]
bug 1436267 fallback from audio callback to system clock driver on error only once

https://reviewboard.mozilla.org/r/218354/#review224164

Comment 4

[Paul Adenot (:padenot)]

Comment on attachment 8948929 [details]
bug 1436267 fallback from audio callback to system clock driver on error only once

https://reviewboard.mozilla.org/r/218354/#review224166

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c96fdc3e0d63
fallback from audio callback to system clock driver on error only once r=padenot

Comment 6

[Stefan Hindli [:stefan_hindli]]

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

Comment 7

[Calixte Denizet (:calixte)]

No more crashes in nightly 60 since the patch landed.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8948929 [details]
bug 1436267 fallback from audio callback to system clock driver on error only once

Approval Request Comment
[Feature/Bug causing the regression]:
https://hg.mozilla.org/mozilla-central/rev/53f055298fa9#l1.165
[User impact if declined]:
crashes with webrtc, web audio
[Is this code covered by automated tests?]:
Not really.  There are many tests, but none cover this error path.
[Has the fix been verified in Nightly?]:
There have been no more crashes reported on Nightly, but volume was low there.
Volume is slightly higher on aurora.
On beta, this assertion is not in the build, and so crashes have different
signatures.
[Needs manual test from QE? If yes, steps to reproduce]:
No.
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
It is a small change.  The consequences of any flaw in the logic would be
failure to attempt recovery from error in system audio libraries.
[String changes made/needed]:
None.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8948929 [details]
bug 1436267 fallback from audio callback to system clock driver on error only once

crash fix, verified on Nightly, Beta59+

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e8b4957bf5f