1032266 - ALSA crash in AudioStream::Shutdown when seeking during playback

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jan Beich]

1. Open http://hpr.dogphilosophy.net/test/opus.opus
2. Seek to any position a few times while the file is playing
3. It should crash with an assert() on alsa_stream_destroy

This happens even in linuxulator with Nightly binary form ftp.mozilla.org.

Assertion failed: (stm && (stm->state == INACTIVE || stm->state == ERROR)), function alsa_stream_destroy, file media/libcubeb/src/cubeb_alsa.c, line 870.

Program received signal SIGABRT, Aborted.
[Switching to Thread 81e60ec00 (LWP 100717 Media Audio)]
thr_kill () at thr_kill.S:3
3       thr_kill.S: No such file or directory.
(gdb) bt
#0  thr_kill () at thr_kill.S:3
#1  0x00000008012d5579 in abort () at lib/libc/stdlib/abort.c:65
#2  0x00000008012b60b1 in __assert (func=0x8074a9809 <.L__func__.alsa_stream_destroy> "alsa_stream_destroy", file=0xfffff80061a58000 <Address 0xfffff80061a58000 out of bounds>, line=1638236160, failedexpr=<optimized out>) at lib/libc/gen/assert.c:54
#3  0x0000000806316f80 in alsa_stream_destroy (stm=0x821b67580) at media/libcubeb/src/cubeb_alsa.c:870
#4  0x0000000806315903 in cubeb_stream_destroy (stream=0x821b67580) at media/libcubeb/src/cubeb.c:219
#5  0x000000080519c155 in nsAutoRefTraits<cubeb_stream>::Release (aStream=0x821b67580) at content/media/AudioStream.h:25
#6  0x000000080519c090 in nsAutoRefBase<cubeb_stream>::SafeRelease (this=0x821c08e60) at xpcom/base/nsAutoRef.h:667
#7  0x000000080519433f in nsAutoRef<cubeb_stream>::reset (this=0x821c08e60) at xpcom/base/nsAutoRef.h:197
#8  0x000000080517d0b0 in mozilla::AudioStream::Shutdown (this=0x821c08d00) at content/media/AudioStream.cpp:844
#9  0x000000080517c901 in mozilla::AudioSink::Cleanup (this=0x823332580) at content/media/AudioSink.cpp:225
#10 0x000000080517bb48 in mozilla::AudioSink::AudioLoop (this=0x823332580) at content/media/AudioSink.cpp:183
#11 0x000000080519cd0a in nsRunnableMethodImpl<void (mozilla::AudioSink::*)(), void, true>::Run (this=0x82332f820) at xpcom/glue/nsThreadUtils.h:391
#12 0x000000080262c092 in nsThread::ProcessNextEvent (this=0x8221ad800, aMayWait=false, aResult=0x7ffffe7cfd2e) at xpcom/threads/nsThread.cpp:762
#13 0x00000008025214b7 in NS_ProcessNextEvent (aThread=0x8221ad800, aMayWait=false) at xpcom/glue/nsThreadUtils.cpp:284
#14 0x0000000802c50f23 in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0x823bd46c0, aDelegate=0x8233c9740) at ipc/glue/MessagePump.cpp:337
#15 0x0000000802ba1576 in MessageLoop::RunInternal (this=0x8233c9740) at ipc/chromium/src/base/message_loop.cc:229
#16 0x0000000802ba14a5 in MessageLoop::RunHandler (this=0x8233c9740) at ipc/chromium/src/base/message_loop.cc:222
#17 0x0000000802ba147d in MessageLoop::Run (this=0x8233c9740) at ipc/chromium/src/base/message_loop.cc:196
#18 0x000000080262a766 in nsThread::ThreadFunc (aArg=0x8221ad800) at xpcom/threads/nsThread.cpp:342
#19 0x000000080ef1fed2 in _pt_root () from /usr/local/lib/libplds4.so.1
#20 0x0000000800f6d555 in thread_start (curthread=0x81e60ec00) at lib/libthr/thread/thr_create.c:284
#21 0x00007ffffe5d0000 in ?? ()
Cannot access memory at address 0x7ffffe7d0000

Comment 1

[Jan Beich]

Happens on Ubuntu as well if you move libpulse.so.0 out of the way.

Comment 2

[Jan Beich]

Let's try changing OS so crash-stats see the bug.

Comment 3

[Paul Adenot (:padenot)]

Attachment: patch

This was originally in bu 848954, but I've been asked to land it separately as it solves issues in other bugs. It has been r+ed by kinetik there.

Comment 4

[Paul Adenot (:padenot)]

Tree is closed, so setting checkin-needed because I'm going to bed and this is blocking jaoo's work.

This has been reviewed in 848954 (part 10), the only change is the commit message.

Comment 5

[Jan Beich]

Comment on attachment 8453869 [details] [diff] [review]
patch

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

::: media/libcubeb/src/cubeb_alsa.c
@@ +868,5 @@
>    cubeb * ctx;
>  
> +  assert(stm && (stm->state == INACTIVE ||
> +                 stm->state == ERROR ||
> +                 stm->state == DRAINING));

No difference at least on FreeBSD. The assert is still triggered when |stm->state == RUNNING|.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d18e878f91d5

Comment 7

[Paul Adenot (:padenot)]

(In reply to Jan Beich from comment #5)
> Comment on attachment 8453869 [details] [diff] [review]
> patch
> 
> Review of attachment 8453869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libcubeb/src/cubeb_alsa.c
> @@ +868,5 @@
> >    cubeb * ctx;
> >  
> > +  assert(stm && (stm->state == INACTIVE ||
> > +                 stm->state == ERROR ||
> > +                 stm->state == DRAINING));
> 
> No difference at least on FreeBSD. The assert is still triggered when
> |stm->state == RUNNING|.

Interesting, this was not part of the summary, I'd have opened another bug otherwise. This needed to go in anyways. Maybe I can try to repro if I have a minute, this is likely to be a regression from the AudioSink split-off. Taking for investigation, because I think kinetik is super busy.

Comment 8

[Maire Reavy [:mreavy]]

NOTE: Jan's failure has a different root cause than the one padenot is fixing for bug 1016277.   Padenot created a new bug for the report in Comment 5:  it's bug 1037423.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Maire Reavy [:mreavy]]

Since we opened a new bug for the other issue (Bug 1037423), I'm marking this resolved fixed.

Paul -- can you write an aurora uplift approval for your patch to get it into v2.0?

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Just curious. How did shutdown happen while draining? Shouldn't we shut down after draining?

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8453869 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: N/A, this has been an issues since cubeb landed ages ago, but way we called the code made it impossible to happen
[User impact if declined]: crash on assert when using Loop (the cubeb library has assert enabled even on release).
[Describe test coverage new/current, TBPL]: this is covered by existing tests and showed up when running tests for bug 1016277 on try (the test on the platform that fails, the b2g emulator, has been disabled for now).
[Risks and why]: this is needed for loop, and has gone though a month worth of try in bug 848954 (where this patch was originally from, part 10).
[String/UUID change made/needed]: none

Comment 13

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

Comment on attachment 8453869 [details] [diff] [review]
patch

Aurora+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7360cef9de1