1426867 - Crash in mozilla::CubebUtils::GetCubebContextUnlocked

Status: RESOLVED FIXED

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

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-17e18cfa-9a00-4000-89e2-5a8b50171222.
=============================================================

Top 10 frames of crashing thread:

0 libxul.so mozilla::CubebUtils::GetCubebContextUnlocked dom/media/CubebUtils.cpp:457
1 libxul.so mozilla::CubebUtils::GetCubebContext dom/media/CubebUtils.cpp:294
2 libxul.so mozilla::AudioStream::Init dom/media/AudioStream.cpp:356
3 libxul.so mozilla::media::AudioSink::InitializeAudioStream dom/media/mediasink/AudioSink.cpp:204
4 libxul.so mozilla::media::AudioSink::Init dom/media/mediasink/AudioSink.cpp:95
5 libxul.so mozilla::media::AudioSinkWrapper::Start dom/media/mediasink/AudioSinkWrapper.cpp:196
6 libxul.so mozilla::media::VideoSink::Start dom/media/mediasink/VideoSink.cpp:172
7 libxul.so mozilla::MediaDecoderStateMachine::StartMediaSink dom/media/MediaDecoderStateMachine.cpp:3328
8 libxul.so mozilla::MediaDecoderStateMachine::MaybeStartPlayback dom/media/MediaDecoderStateMachine.cpp:2958
9 libxul.so mozilla::MediaDecoderStateMachine::DecodingState::Step dom/media/MediaDecoderStateMachine.cpp:2339

=============================================================

There are 5 crashes (from the same install) in nightly 59 with buildid 20171222100407. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1405877.

[1] https://hg.mozilla.org/mozilla-central/rev/502337e7c6ae

Comment 1

[Matthew Gregan [:kinetik]]

It looks like we're trying to create a cubeb context before the child has completed the pre-init for the audio remoting.  That's unusual, so this should be an uncommon assert to hit.  We can fix it by forcing the cubeb context init to wait for the pre-init to complete, but I'd like to understand what's triggering this first, if possible.

I'll take a look at this when I'm back from PTO (early January).  In the meantime, this can be worked around by setting the pref media.cubeb.sandbox to false.

Comment 2

[Matthew Gregan [:kinetik]]

There are 5 crash results (4 Linux, 1 macOS) for this signature.  All of them are in "content (file)" process types, suggesting this is triggered via file:// playback.  Testing various local media, I can reproduce this reliably by running "mach run dom/media/test/gizmo.mp4" (some other files don't trigger the assert, possibly due to slightly different timing).

Comment 3

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=772e401ac248d1e2e7253c836a733ad100277a56

Comment 4

[Michael Froman [:mjf]]

I believe I've seen this issue too.  When it happens, I see this message:
Assertion failure: sIPCConnection.IsValid(), at /home/mfroman/mozilla/moz-central/dom/media/CubebUtils.cpp:458

https://dxr.mozilla.org/mozilla-central/source/dom/media/CubebUtils.cpp?q=GetCubebContextUnlocked&redirect_type=direct#458

In particular, I've seen it when using some test html from the fuzzing team on Bug 1425780.  See the trigger.html attachment:
https://bugzilla.mozilla.org/attachment.cgi?id=8937373

When running with basic prefs (not the prefs provided for fuzzing in the bug), I've been able to repro mostly reliably on my linux box.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

[Tracking Requested - why for this release]: crash regression on linux and osx

Comment 6

[Matthew Gregan [:kinetik]]

It's Nightly-only code.

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: wip: wait for init promise to complete

Attaching for posterity - this is what the earlier try push used, but it's broken due to main thread issues.

This removed the static global file descriptor and waited on the Gecko IPC initialization promise to complete.  This is a nicer solution than what we have now, but requires no GetCubebContext() calls are made from the main thread.  Unfortunately, Web Audio does exactly that (e.g. creating an AudioContext queries the max channel count, which creates a cubeb context).  

Long term, we should ban cubeb calls from the main thread, since they could be expensive and it's pretty dumb to block the main thread on a cubeb context initialization (which could involve initializating multiple audio backends, loading DSOs, connecting over sockets, etc.).  I'll file a separate bug for that.

Comment 8

[Matthew Gregan [:kinetik]]

Comment on attachment 8942796 [details] [diff] [review]
wip: wait for init promise to complete

Obsoleting because we can't use this approach today.

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: bug1426867_p1_cleanup_cubebutils_h.patch

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: bug1426867_p2_init_audioipc_earlier.patch

For now, we take the dumb and ugly approach and try to win the initialization race by starting even earlier.  This works for all of the cases I found locally that would trigger the assert before.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f52323c4ef843eaadc49c9a0cf0e11a086a387e

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b41fbd6b35a
Move private functions out of CubebUtils.h.  r=kamidphish
https://hg.mozilla.org/integration/mozilla-inbound/rev/14b5945f84c4
Init AudioIPC even earlier.  r=kamidphish

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7b41fbd6b35a
https://hg.mozilla.org/mozilla-central/rev/14b5945f84c4