1545133 - Assertion failure: mStreams.Contains(aStream), at /builds/worker/workspace/build/src/dom/media/MediaStreamGraph.cpp:3535

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev bbca68b2af26.

Assertion failure: mStreams.Contains(aStream), at /builds/worker/workspace/build/src/dom/media/MediaStreamGraph.cpp:3535

rax = 0x000056450a1f4e20 rdx = 0x00007f42b1b8f680
rcx = 0x0000000000000b40 rbx = 0x00007f42a2c11400
rsi = 0x00007f42bcc1c8b0 rdi = 0x00007f42bcc1b680
rbp = 0x00007f42a26b90f0 rsp = 0x00007f42a26b90d0
r8 = 0x00007f42bcc1c8b0 r9 = 0x00007f42a26ba700
r10 = 0x0000000000000000 r11 = 0x0000000000000000
r12 = 0x00007f42a26b90d8 r13 = 0x00007f42a282bfb0
r14 = 0x0000000000000006 r15 = 0x0000000000000000
rip = 0x00007f42add23bc8
OS|Linux|0.0.0 Linux 4.18.0-17-generic #18~18.04.1-Ubuntu SMP Fri Mar 15 15:27:12 UTC 2019 x86_64
CPU|amd64|family 6 model 94 stepping 3|1
GPU|||
Crash|SIGSEGV /SEGV_MAPERR|0x0|35
35|0|libxul.so|mozilla::MediaStreamGraphImpl::IncrementSuspendCount(mozilla::MediaStream*)|hg:hg.mozilla.org/mozilla-central:dom/media/MediaStreamGraph.cpp:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|3541|0x40
35|1|libxul.so|mozilla::MediaStreamGraphImpl::SuspendOrResumeStreams(mozilla::dom::AudioContextOperation, nsTArray<mozilla::MediaStream*> const&)|hg:hg.mozilla.org/mozilla-central:dom/media/MediaStreamGraph.cpp:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|3575|0x5
35|2|libxul.so|mozilla::MediaStreamGraphImpl::ApplyAudioContextOperationImpl(mozilla::MediaStream*, nsTArray<mozilla::MediaStream*> const&, mozilla::dom::AudioContextOperation, void*, mozilla::dom::AudioContextOperationFlags)|hg:hg.mozilla.org/mozilla-central:dom/media/MediaStreamGraph.cpp:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|3629|0x5
35|3|libxul.so|mozilla::MediaStreamGraphImpl::RunMessagesInQueue()|hg:hg.mozilla.org/mozilla-central:dom/media/MediaStreamGraph.cpp:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|1169|0x11
35|4|libxul.so|mozilla::MediaStreamGraphImpl::OneIterationImpl(long)|hg:hg.mozilla.org/mozilla-central:dom/media/MediaStreamGraph.cpp:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|1396|0x8
35|5|libxul.so|mozilla::AudioCallbackDriver::DataCallback(float const*, float*, long)|hg:hg.mozilla.org/mozilla-central:dom/media/GraphDriver.cpp:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|902|0xc
35|6|libxul.so|<futures::future::lazy::Lazy<F, R> as futures::future::Future>::poll|hg:hg.mozilla.org/mozilla-central:third_party/rust/futures/src/future/lazy.rs:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|82|0x73
35|7|libxul.so|std::panicking::try::do_call|hg:hg.mozilla.org/mozilla-central:third_party/rust/futures/src/future/catch_unwind.rs:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|49|0x5
35|8|libxul.so|__rust_maybe_catch_panic|||0x9
35|9|libxul.so|<futures_cpupool::MySender<F, core::result::Result<<F as futures::future::Future>::Item, <F as futures::future::Future>::Error>> as futures::future::Future>::poll|hg:hg.mozilla.org/mozilla-central:third_party/rust/futures-cpupool/src/lib.rs:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|325|0x96
35|10|libxul.so|futures::task_impl::std::set|hg:hg.mozilla.org/mozilla-central:third_party/rust/futures/src/future/mod.rs:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|113|0xa
35|11|libxul.so|futures::task_impl::std::Run::run|hg:hg.mozilla.org/mozilla-central:third_party/rust/futures/src/task_impl/std/mod.rs:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|450|0x68
35|12|libxul.so|futures_cpupool::Inner::work|hg:hg.mozilla.org/mozilla-central:third_party/rust/futures-cpupool/src/lib.rs:bbca68b2af262ffbbf2e3a2d2e77a16c999f479a|257|0x42
35|13|libxul.so|std::sys_common::backtrace::__rust_begin_short_backtrace|git:github.com/rust-lang/rust:src/libstd/sys_common/backtrace.rs:91856ed52c58aa5ba66a015354d1cc69e9779bdf|135|0x35
35|14|libxul.so|std::panicking::try::do_call|git:github.com/rust-lang/rust:src/libstd/thread/mod.rs:91856ed52c58aa5ba66a015354d1cc69e9779bdf|469|0x21
35|15|libxul.so|__rust_maybe_catch_panic|||0x9
35|16|libxul.so|<F as alloc::boxed::FnBox<A>>::call_box|git:github.com/rust-lang/rust:src/liballoc/boxed.rs:91856ed52c58aa5ba66a015354d1cc69e9779bdf|749|0xa4
35|17|libxul.so|thread_start|git:github.com/rust-lang/rust:src/libstd/sys/unix/thread.rs:91856ed52c58aa5ba66a015354d1cc69e9779bdf|81|0x84
35|18|libpthread-2.27.so||||0x76db
35|19|libc-2.27.so|clone|||0x3f

Comment 1

[Michael Froman [:mjf]]

Andreas, do you have any thoughts on what is happening here?

Comment 2

[Andreas Pehrson [:pehrsons]]

Not immediately, but if I can reproduce.

Comment 3

[Andreas Pehrson [:pehrsons]]

This seems to be triggered by the next CC after running the testcase.

Comment 4

[Andreas Pehrson [:pehrsons]]

NB that autoplay blocking must be disabled for the testcase to work.

Comment 5

[Andreas Pehrson [:pehrsons]]

What's leading to this assertion failure is the sequence below (all for the same MediaStream):

MediaStream::mSuspendedCount = 1;
MSGImpl::DecrementSuspendCount() (mSuspendedCount = 0)
MSGImpl::IncrementSuspendCount() (mSuspendedCount = 1)
MSGImpl::DecrementSuspendCount() (mSuspendedCount = 0)
MSGImpl::DecrementSuspendCount() (mSuspendedCount = -1) (!!!)
MSGImpl::IncrementSuspendCount() (mSuspendedCount = 0)
MSGImpl::DecrementSuspendCount() (mSuspendedCount = -1)
MSGImpl::IncrementSuspendCount() (BOOM)

Before the suspended count goes from -1 to 0, IncrementSuspendCount() checks whether we were not suspended, and if so assumes we now became suspended. In this case this is wrong, and this is the first bug.

Because it assumed we became suspended, the stream was moved from mStreams to mSuspendedStreams. On the next DecrementSuspendCount nothing happens because the suspend state didn't change. However, on the following IncrementSuspendCount it is again assumed that the stream became suspended, but this time it cannot be moved from mStreams to mSuspendedStreams because it is already in mSuspendedStreams; it's a double-suspend, and the move has already happened.

In the sequence above, the double-DecrementSuspendCount that I marked with (!!!) is the second bug. This seems however fixed on m-c since this was filed. I'll focus on landing the test and fixing the first bug as a protection against future bugs that do something similar to the second bug I found here.

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1545133 - Add crashtest. r?padenot

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1545133 - Avoid trying to double-suspend a MediaStream if external code makes the suspendCount negative. r?padenot

We don't currently know any ways in which external code can make the
suspendCount negative, but this will keep us safe should a future bug enable it.

Depends on D28805

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/158a9ef03153
Add crashtest. r=padenot
https://hg.mozilla.org/integration/autoland/rev/c0e459d13624
Avoid trying to double-suspend a MediaStream if external code makes the suspendCount negative. r=padenot

Comment 9

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/158a9ef03153
https://hg.mozilla.org/mozilla-central/rev/c0e459d13624