1038091 - Assertion failure: frames >= mBaseOffset, at content/media/AudioStream.cpp:107

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Douglas Crosher [:dougc]]

Testing an asm.js demo game on Flame build from m-c, hits this crash.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 3446.3534]
0xb57d683c in GetPosition (frames=496, this=0xaef43f90) at ../../../src/content/media/AudioStream.cpp:107
107	    MOZ_ASSERT(frames >= mBaseOffset);
(gdb) back
#0  0xb57d683c in GetPosition (frames=496, this=0xaef43f90) at ../../../src/content/media/AudioStream.cpp:107
#1  mozilla::AudioClock::GetPositionUnlocked (this=0xb11dfb80) at ../../../src/content/media/AudioStream.cpp:1167
#2  0xb57d694a in mozilla::AudioStream::GetPosition (this=0xb11dfb40) at ../../../src/content/media/AudioStream.cpp:852
#3  0xb57dffb2 in mozilla::MediaDecoderStateMachine::GetAudioClock (this=0xaf396400) at ../../../src/content/media/MediaDecoderStateMachine.cpp:2502
#4  0xb57e052c in GetClock (this=0xaf396400) at ../../../src/content/media/MediaDecoderStateMachine.cpp:2536
#5  mozilla::MediaDecoderStateMachine::GetClock (this=0xaf396400) at ../../../src/content/media/MediaDecoderStateMachine.cpp:2521
#6  0xb57f573c in mozilla::MediaDecoderStateMachine::AdvanceFrame (this=0xaf396400) at ../../../src/content/media/MediaDecoderStateMachine.cpp:2571
#7  0xb57f787c in mozilla::MediaDecoderStateMachine::RunStateMachine (this=0xaf396400) at ../../../src/content/media/MediaDecoderStateMachine.cpp:2333
#8  0xb57e2836 in mozilla::MediaDecoderStateMachineScheduler::TimeoutExpired (this=0xaf1d7670, aTimerId=7) at ../../../src/content/media/MediaDecoderStateMachineScheduler.cpp:160
#9  0xb4be2f96 in nsTimerImpl::Fire (this=0xb1e18b50) at ../../../src/xpcom/threads/nsTimerImpl.cpp:621
#10 0xb4be310e in nsTimerEvent::Run (this=0xb23a10b0) at ../../../src/xpcom/threads/nsTimerImpl.cpp:711
#11 0xb4be15ee in nsThreadPool::Run (this=0xb1d5df40) at ../../../src/xpcom/threads/nsThreadPool.cpp:220
#12 0xb4be0afe in ProcessNextEvent (aResult=0xabd1fe27, aMayWait=true, this=0xaee1c300) at ../../../src/xpcom/threads/nsThread.cpp:766
#13 nsThread::ProcessNextEvent (this=0xaee1c300, aMayWait=<optimized out>, aResult=0xabd1fe27) at ../../../src/xpcom/threads/nsThread.cpp:685
#14 0xb4b9a2a4 in NS_ProcessNextEvent (aThread=0xaee1c300, aMayWait=<optimized out>) at /home/src/b2g/src/xpcom/glue/nsThreadUtils.cpp:256
#15 0xb4d96514 in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0xaeee94c0, aDelegate=0xb22e5240) at ../../../src/ipc/glue/MessagePump.cpp:355
#16 0xb4d84c9e in MessageLoop::RunInternal (this=0xb22e5240) at ../../../src/ipc/chromium/src/base/message_loop.cc:229
#17 0xb4d84cb6 in RunHandler (this=0xb22e5240) at ../../../src/ipc/chromium/src/base/message_loop.cc:222
#18 MessageLoop::Run (this=0xb22e5240) at ../../../src/ipc/chromium/src/base/message_loop.cc:196
#19 0xb4be1a2c in nsThread::ThreadFunc (aArg=0xaee1c300) at ../../../src/xpcom/threads/nsThread.cpp:346
#20 0xb4791348 in _pt_root (arg=0xaee83b00) at ../../../../../src/nsprpub/pr/src/pthreads/ptthread.c:212
#21 0xb6f28ba4 in __thread_entry (func=0xb47912a1 <_pt_root>, arg=0xaee83b00, tls=0xabd1ff00) at bionic/libc/bionic/pthread_create.cpp:92
#22 0xb6f28d20 in pthread_create (thread_out=0xbee0d75c, attr=<optimized out>, start_routine=0x78, arg=0xaee83b00) at bionic/libc/bionic/pthread_create.cpp:201

Comment 1

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

What is the address of the web game?

Comment 2

[Douglas Crosher [:dougc]]

(In reply to JW Wang [:jwwang] from comment #1)
> What is the address of the web game?

The follow public demo reproduces the crash:
http://kripken.github.io/misc-js-benchmarks/banana/benchmark.html

The above will not run far enough on the Flame to reproduce the crash, but the following modified version does: http://www.scieneer.com/files/b4/benchmark.html

Comment 3

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

I can't repro it on the desktop browser. I will give it a try on Flame or some other devices.

Comment 4

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

Attachment: 1038091_fix_cubeb_opensl_drain_positioin.patch

SL_PLAYSTATE_STOPPED will reset the player position and result in the position returned by cubeb_stream_get_position() going backward. Pass SL_PLAYSTATE_PAUSED instead to avoid resetting playback position.

Also add an assertion which checks if the position returned by cubeb_stream_get_position() is going backward.

Try: https://tbpl.mozilla.org/?tree=Try&rev=0511e5def41c
The assertion didn't happen with this patch.

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8496560 [details] [diff] [review]
1038091_fix_cubeb_opensl_drain_positioin.patch

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

::: content/media/AudioStream.cpp
@@ +860,5 @@
> +  if (position >= mLastGoodPosition) {
> +    mLastGoodPosition = position;
> +  } else {
> +    MOZ_ASSERT(false, "cubeb position shouldn't go backward");
> +  }

Since MOZ_ASSERT is fatal, this can be simplified to:

    MOZ_ASSERT(position >= mLastGoodPosition, "cubeb position shouldn't go backward");
    mLastGoodPosition = position;

Comment 6

[Matthew Gregan [:kinetik]]

Do we need to fix cubeb_stream_stop as well?  That's not supposed to reset the playback position.

Comment 7

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

(In reply to Matthew Gregan [:kinetik] from comment #6)
> Do we need to fix cubeb_stream_stop as well?  That's not supposed to reset
> the playback position.

Do you mean opensl_stream_stop [1]? Since SL_PLAYSTATE_PAUSED is used, we should be fine with that.

[1] http://hg.mozilla.org/mozilla-central/file/5e704397529b/media/libcubeb/src/cubeb_opensl.c#l682

Comment 8

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

Comment on attachment 8496560 [details] [diff] [review]
1038091_fix_cubeb_opensl_drain_positioin.patch

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

::: content/media/AudioStream.cpp
@@ +860,5 @@
> +  if (position >= mLastGoodPosition) {
> +    mLastGoodPosition = position;
> +  } else {
> +    MOZ_ASSERT(false, "cubeb position shouldn't go backward");
> +  }

This error handling/recovery keeps us in good shape in release build even if cubeb_stream_get_position() is regressed later and buy us more time for debugging while keeping release build in good shape.

However, since this bullet-proof style kinda defeats the purpose of MOZ_ASSERT and is not as fatal as memory leaks or crash which we want to prevent in release build, I don't have a strong reason to justify the usage.

Comment 9

[Matthew Gregan [:kinetik]]

(In reply to JW Wang [:jwwang] from comment #7)
> Do you mean opensl_stream_stop [1]? Since SL_PLAYSTATE_PAUSED is used, we
> should be fine with that.

Ah, so it is.  Excellent.

(In reply to JW Wang [:jwwang] from comment #8)
> This error handling/recovery keeps us in good shape in release build even if
> cubeb_stream_get_position() is regressed later and buy us more time for
> debugging while keeping release build in good shape.
> 
> However, since this bullet-proof style kinda defeats the purpose of
> MOZ_ASSERT and is not as fatal as memory leaks or crash which we want to
> prevent in release build, I don't have a strong reason to justify the usage.

Fair enough, I have used this technique too.  How about:

    MOZ_ASSERT(position >= mLastGoodPosition, "cubeb position shouldn't go backward");
    if (position >= mLastGoodPosition) {
      mLastGoodPosition = position;
    }

then?  It's mostly just that I prefer the assert statement contains the test directly where possible.

Comment 10

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

(In reply to Matthew Gregan [:kinetik] from comment #9)
How about:
> 
>     MOZ_ASSERT(position >= mLastGoodPosition, "cubeb position shouldn't go
> backward");
>     if (position >= mLastGoodPosition) {
>       mLastGoodPosition = position;
>     }
> 
> then?  It's mostly just that I prefer the assert statement contains the test
> directly where possible.

Great. This style is more concise.

Comment 11

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

Attachment: 1038091_fix_cubeb_opensl_drain_positioin-v2.patch

Fix nits suggested by kinetik.

Try: https://tbpl.mozilla.org/?tree=Try&rev=9758ba5c41d5
     https://tbpl.mozilla.org/?tree=Try&rev=0511e5def41c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67b4334af9b1

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67b4334af9b1