1365205 - Ski Safari WebGL game crashes in android.media.MediaCodec$CodecException: Error 0xfffffff3 at android.media.MediaCodec.releaseOutputBuffer(Native Method)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect, P1)

Description

[Jukka Jylänki]

This bug was filed from the Socorro interface and is 
report bp-e2ac97e5-bfc5-41b6-9ba0-52f970170515.
=============================================================

Playing Ski Safari WebGL game on Facebook on a Huawei P10 Plus, the browser occassionally crashes with this callstack.

https://www.facebook.com/skisafarigame/

Comment 1

[Marcia Knous [:marcia]]

I hit this crash scrolling a web page on CNET.com - https://crash-stats.mozilla.com/report/index/362aaf8c-b488-45c2-8172-757a00170620

Comment 2

[Marcia Knous [:marcia]]

This is currently the top crash on Android Nightly in the last 7 days.

Comment 3

[Marcia Knous [:marcia]]

:jolin - Do we need to reproduce this, or do you need other information to help move this along? Thanks.

Comment 4

[John Lin [:jhlin][:jolin]]

Hi Marcia, could you reproduce this and get the logcat dump? The Java stack trace in the report is from decoder process instead of browser process, and the logcat shows nothing from browser process. My guess is that browser crashed during decoder calling back to it and brought the decoder down with it, but need log from browser for investigation. Thanks.

Comment 5

[Marcia Knous [:marcia]]

ni on Ioana to see if we can get someone to reproduce this crash. Currently the top crash on beta as well as nightly.

Comment 6

[Benjamin Chen [:bechen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a7a9d1bf542&selectedJob=113570504

Maybe we can reproduce it at try server, dom/media/test/test_buffered.html

Comment 7

[John Lin [:jhlin][:jolin]]

Logcat shows there was a flush right before the crash. It looks like a race condition that buffer callback emitted before flush executed after the flush is completed and all buffers are invalid.

07-16 22:57:27.972 26448 27116 I OMXClient: MuxOMX ctor
07-16 22:57:27.982 26448 27116 I ACodec  : [OMX.google.aac.decoder] Now Loaded
07-16 22:57:27.984 26448 27116 I MediaCodec: MediaCodec will operate in async mode
07-16 22:57:27.992 26448 27116 I ACodec  : [OMX.google.aac.decoder] Now Loaded->Idle
07-16 22:57:28.000 26448 27116 I ACodec  : [OMX.google.aac.decoder] Now Idle->Executing
07-16 22:57:28.002 26448 27116 I ACodec  : [OMX.google.aac.decoder] Now Executing
07-16 22:57:28.009 26448 27116 I ACodec  : [OMX.google.aac.decoder] Now handling output port settings change
07-16 22:57:28.021 26448 27116 I ACodec  : [OMX.google.aac.decoder] Now Executing
07-16 22:57:28.034 26448 27116 I ACodec  : [OMX.google.aac.decoder] signalFlush
07-16 22:57:28.034 26448 27116 I ACodec  : [OMX.google.aac.decoder] ExecutingState flushing now (codec owns 0/4 input, 3/4 output).
07-16 22:57:28.034 26448 27116 I ACodec  : [OMX.google.aac.decoder] Now Flushing
07-16 22:57:28.035 26448 27116 I ACodec  : [OMX.google.aac.decoder] FlushingState onOMXEvent(0,1,0)
07-16 22:57:28.035 26448 27116 I ACodec  : [OMX.google.aac.decoder] FlushingState onOMXEvent(0,1,1)
07-16 22:57:28.035 26448 27116 I ACodec  : [OMX.google.aac.decoder] Now Executing
07-16 22:57:28.036 26448 26448 E MediaCodec: getBufferAndFormat - invalid operation (the index 2 is not owned by client)
07-16 22:57:28.037 26448 26448 W System.err: java.lang.IllegalStateException
07-16 22:57:28.052 26448 26448 W System.err: 	at android.media.MediaCodec.getBuffer(Native Method)

Comment 8

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1365205 - part 1: check for buffer validness before processing it.

flush() invalidates all buffers so buffer callbacks emitted earlier than that should not proceed when they're executed after.

Review commit: https://reviewboard.mozilla.org/r/157638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157638/

Comment 9

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1365205 - part 2: execute callback directly when already on the right looper/thread.

Review commit: https://reviewboard.mozilla.org/r/157640/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157640/

Comment 10

[Eugen Sawin [:esawin]]

Comment on attachment 8886909 [details]
Bug 1365205 - part 1: check for buffer validness before processing it.

https://reviewboard.mozilla.org/r/157638/#review162864

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:136
(Diff revision 1)
>  
>          }
>  
> +        private boolean isValidBuffer(final int index) {
> +            try {
> +                mCodec.getInputBuffer(index);

Is there no better way to check for validity and should we not check for a null-return?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:268
(Diff revision 1)
>              }
>          }
>  
> +        private boolean isValidBuffer(final int index) {
> +            try {
> +                mCodec.getOutputBuffer(index);

See previous comment.

Comment 11

[Eugen Sawin [:esawin]]

Comment on attachment 8886910 [details]
Bug 1365205 - part 2: execute callback directly when already on the right looper/thread.

https://reviewboard.mozilla.org/r/157640/#review162866

Comment 12

[John Lin [:jhlin][:jolin]]

Comment on attachment 8886909 [details]
Bug 1365205 - part 1: check for buffer validness before processing it.

https://reviewboard.mozilla.org/r/157638/#review162864

> Is there no better way to check for validity and should we not check for a null-return?

Thanks a lot for the review. I'll add nullness check in next version.

Comment 13

[John Lin [:jhlin][:jolin]]

Comment on attachment 8886909 [details]
Bug 1365205 - part 1: check for buffer validness before processing it.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157638/diff/1-2/

Comment 14

[John Lin [:jhlin][:jolin]]

Comment on attachment 8886910 [details]
Bug 1365205 - part 2: execute callback directly when already on the right looper/thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157640/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73b2bebfcc18
part 1: check for buffer validness before processing it. r=esawin
https://hg.mozilla.org/integration/autoland/rev/9286e4085ee0
part 2: execute callback directly when already on the right looper/thread. r=esawin

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/73b2bebfcc18
https://hg.mozilla.org/mozilla-central/rev/9286e4085ee0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 18

[John Lin [:jhlin][:jolin]]

Comment on attachment 8886909 [details]
Bug 1365205 - part 1: check for buffer validness before processing it.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1361679
[User impact if declined]: On Android L or later devices, sometimes browser crashes when watching video
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]:it detects invalid buffers before processing them
[String changes made/needed]:none

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8886909 [details]
Bug 1365205 - part 1: check for buffer validness before processing it.

fix fennec crash, regression in 55, should be in 55.0b12

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3b374998e442
https://hg.mozilla.org/releases/mozilla-beta/rev/823d66f7bf98