Assertion failure: false (A non-finished SourceMediaStream wasn't fed enough data by NotifyPull), at /builds/worker/workspace/build/src/dom/media/MediaStreamGraph.cpp:1254

RESOLVED FIXED in Firefox 68

Status

()

defect
P2
critical
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: jkratzer, Assigned: pehrsons)

Tracking

(Blocks 1 bug, {crash, regression, testcase})

unspecified
mozilla68
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 wontfix, firefox67 wontfix, firefox68 fixed)

Details

(Whiteboard: [fuzzblocker], crash signature)

Attachments

(2 attachments)

Reporter

Description

4 months ago
Posted file testcase.html

Assertion failure: false (A non-finished SourceMediaStream wasn't fed enough data by NotifyPull), at /builds/worker/workspace/build/src/dom/media/MediaStreamGraph.cpp:1254

Testcase found while fuzzing mozilla-central rev 52b73d447c52. Testcase must be served via a local webserver in order to reproduce.

==7337==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x7f05fda2a577 bp 0x7f05dfe0dd90 sp 0x7f05dfe0dae0 T38)
==7337==The signal is caused by a WRITE memory access.
==7337==Hint: address points to the zero page.
#0 0x7f05fda2a576 in mozilla::MediaStreamGraphImpl::UpdateGraph(long) /builds/worker/workspace/build/src/dom/media/MediaStreamGraph.cpp:1252:13
#1 0x7f05fda2d8b2 in mozilla::MediaStreamGraphImpl::OneIteration(long) /builds/worker/workspace/build/src/dom/media/MediaStreamGraph.cpp:1395:3
#2 0x7f05fd6f374a in mozilla::ThreadedDriver::RunThread() /builds/worker/workspace/build/src/dom/media/GraphDriver.cpp:306:41
#3 0x7f05fd7251a2 in mozilla::MediaStreamGraphInitThreadRunnable::Run() /builds/worker/workspace/build/src/dom/media/GraphDriver.cpp:204:14
#4 0x7f05f515e1c6 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1162:14
#5 0x7f05f5165f8d in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:474:10
#6 0x7f05f6410431 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:303:20
#7 0x7f05f62fb83e in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#8 0x7f05f62fb83e in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#9 0x7f05f62fb83e in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#10 0x7f05f5156663 in nsThread::ThreadFunc(void*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:449:11
#11 0x7f061a2dd666 in _pt_root /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:201:5
#12 0x7f0619f216da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
#13 0x7f0618eff88e in clone /build/glibc-OTsEL5/glibc-2.27/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/worker/workspace/build/src/dom/media/MediaStreamGraph.cpp:1252:13 in mozilla::MediaStreamGraphImpl::UpdateGraph(long)
Thread T38 (MediaStreamGrph) created by T36 (AudioIPC1) here:
#0 0x556f2a8306bd in __interceptor_pthread_create /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:210:3
#1 0x7f061a2da395 in _PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:433:14
#2 0x7f061a2d9f7e in PR_CreateThread /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:518:12
#3 0x7f05f51595b9 in nsThread::Init(nsTSubstring<char> const&) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:655:8
#4 0x7f05f5164c45 in nsThreadManager::NewNamedThread(nsTSubstring<char> const&, unsigned int, nsIThread**) /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp:410:12
#5 0x7f05f5169b44 in NS_NewNamedThread(nsTSubstring<char> const&, nsIThread**, nsIRunnable*, unsigned int) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:127:57
#6 0x7f05fd6f255c in NS_NewNamedThread<16> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:71:10
#7 0x7f05fd6f255c in mozilla::ThreadedDriver::Start() /builds/worker/workspace/build/src/dom/media/GraphDriver.cpp:222
#8 0x7f05fd6f1695 in mozilla::GraphDriver::SwitchToNextDriver() /builds/worker/workspace/build/src/dom/media/GraphDriver.cpp:103:17
#9 0x7f05fd6fbc94 in mozilla::AudioCallbackDriver::DataCallback(float const*, float*, long) /builds/worker/workspace/build/src/dom/media/GraphDriver.cpp:942:5
#10 0x7f06062f7a98 in $LT$audioipc_client..stream..CallbackServer$u20$as$u20$audioipc..rpc..server..Server$GT$::process::$u7b$$u7b$closure$u7d$$u7d$::h28dd4e3894b83022 /builds/worker/workspace/build/src/media/audioipc/client/src/stream.rs:110:24
#11 0x7f06062f7a98 in _$LT$futures..future..lazy..Lazy$LT$F$C$$u20$R$GT$$GT$::get::h104328d61243c4d6 /builds/worker/workspace/build/src/third_party/rust/futures/src/future/lazy.rs:64
#12 0x7f06062f7a98 in $LT$futures..future..lazy..Lazy$LT$F$C$$u20$R$GT$$u20$as$u20$futures..future..Future$GT$::poll::hd23a29becfa61583 /builds/worker/workspace/build/src/third_party/rust/futures/src/future/lazy.rs:82
#13 0x7f06062f7a98 in futures::future::catch_unwind::
$LT$impl$u20$futures..future..Future$u20$for$u20$std..panic..AssertUnwindSafe$LT$F$GT$$GT$::poll::h0850ffa648a463c4 /builds/worker/workspace/build/src/third_party/rust/futures/src/future/catch_unwind.rs:49
#14 0x7f06062f7a98 in $LT$futures..future..catch_unwind..CatchUnwind$LT$F$GT$$u20$as$u20$futures..future..Future$GT$::poll::$u7b$$u7b$closure$u7d$$u7d$::h6c033e1ce6ef5b31 /builds/worker/workspace/build/src/third_party/rust/futures/src/future/catch_unwind.rs:32
#15 0x7f06062f7a98 in std::panicking::try::do_call::h3a4127862ee7f453 /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libstd/panicking.rs:310
#16 0x7f06062f7a98 in __rust_maybe_catch_panic /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libpanic_abort/lib.rs:39
#17 0x7f06062f7a98 in std::panicking::try::h09f95521e16c9c6b /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libstd/panicking.rs:289
#18 0x7f06062f7a98 in std::panic::catch_unwind::h7b9bd9b48aa68a0a /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libstd/panic.rs:398
#19 0x7f06062f7a98 in _$LT$futures..future..catch_unwind..CatchUnwind$LT$F$GT$$u20$as$u20$futures..future..Future$GT$::poll::h32cba86a96c825b5 /builds/worker/workspace/build/src/third_party/rust/futures/src/future/catch_unwind.rs:32
#20 0x7f06062f7a98 in _$LT$futures_cpupool..MySender$LT$F$C$$u20$core..result..Result$LT$$LT$F$u20$as$u20$futures..future..Future$GT$..Item$C$$u20$$LT$F$u20$as$u20$futures..future..Future$GT$..Error$GT$$GT$$u20$as$u20$futures..future..Future$GT$::poll::ha949fea50d0b6a22 /builds/worker/workspace/build/src/third_party/rust/futures-cpupool/src/lib.rs:325

Thread T36 (AudioIPC1) created by T0 (file:// Content) here:
#0 0x556f2a8306bd in __interceptor_pthread_create /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:210:3
#1 0x7f060690e5a6 in std::sys::unix::thread::Thread::new::h6179c0ba07009a42 /rustc/9fda7c2237db910e41d6a712e9a2139b352e558b/src/libstd/sys/unix/thread.rs:78:18

==7337==ABORTING

Flags: in-testsuite?

Andreas could you please have a look if this is related to recent refactoring, or a long standing issue?

Flags: needinfo?(apehrson)
Priority: -- → P2
Assignee

Comment 2

4 months ago

It's probably from recent fixes. I'll take it.

Assignee: nobody → apehrson
Flags: needinfo?(apehrson)
Assignee

Updated

4 months ago
Status: NEW → ASSIGNED
Assignee

Comment 3

4 months ago

This needs media.navigator.permission.disabled set to true to reproduce.

Assignee

Comment 4

4 months ago

It's a pulled track that doesn't have a listener attached. In this case I keep hitting it for the fake audio track, but that's probably irrelevant and could happen on any gUM-track. Highly likely that it's from a recent change.

Assignee

Comment 5

4 months ago

For controlling these streams and tracks we send messages from main thread.

Here we have a race because initialization looks like this (thread in parens) (two paths):
GUMRunnable (main) -> InitializeAsync() (media) -> SetPullingEnabled(true) (main)
GUMRunnable (main) -> Activate() (main) -> AddTrackListener() (no thread hop)

While the shutdown sequence triggering this looks like (two paths):
OnNavigation() (main) -> Stop() (media) -> SetPullingEnabled(false) (main)
OnNavigation() (main) -> Remove() (main) -> RemoveTrackListener()/SetPullingEnabled(false) (main) (no thread hop)

So the order of these events when we crash is (times are from my run's log):

  • GUMRunnable
  • Activate (10:37:09.691115)
    • AddTrackListener
  • OnNavigation (10:37:09.708155)
  • Remove (10:37:09.709060)
    • RemoveTrackListener
    • SetPullingEnabled(false) (futile)
  • InitializeAsync (10:37:12.244697)
  • SetPullingEnabled(true)
  • Boom
Assignee

Comment 6

4 months ago

I think the best way forward here is to give control of the track listeners to MediaEngineSources -- and only the ones that need it.

It would be nice to wait for bug 1423253 since it removes the need for pulling (i.e., the need for the listener altogether) from all sources except the microphone one. Perhaps we don't need to pull the microphone either, but it does help us to assert that we're adding the expected amount of data.

Depends on: 1423253
Assignee

Comment 7

3 months ago

This moves the responsibility of forwarding NotifyPull() from the graph thread
to MediaEngineSources out of MediaManager and into the sources themselves. This
is better aligned with how the sources work, since not all sources need pulling.
This also clarifies lifetime management of these listeners in relation to when
pulling is enabled for a track, since the sources are already handling enabling
pulling themselves.

Assignee

Updated

3 months ago
Blocks: 1494675

Comment 8

3 months ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1f79ed91a20b
Remove SourceTrackListener from MediaManager. r=padenot

Comment 9

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Is it possible to land this testcase as a crashtest?

Flags: needinfo?(apehrson)
Assignee

Comment 11

2 months ago

I tried but this testcase is one of those that need to spin at location.reload() for several seconds before hitting the crash, and I have never managed to convert one of those to a crash test. I think I even tried to put the testcase in an iframe so it could reload itself and have the parent window cancel it after a timeout, but either it failed to repro or I didn't get that cancellation to work.

Flags: needinfo?(apehrson)
Flags: in-testsuite? → in-testsuite-
Assignee

Comment 12

2 months ago

I found this diagnostic assert on crash-stats existing in the wild. That crash is the non-debug version. Shows for 67 and earlier only.

Crash Signature: [@ mozilla::MediaStreamGraphImpl::UpdateGraph]
You need to log in before you can comment on or make changes to this bug.