1188870 - crash in libsomxcore.so@0x1826 on a Samsung Galaxy S3 Mini with JB 4.1.2

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Jan Henning [:JanH]]

This bug was filed from the Socorro interface and is 
report bp-bba2f311-18b8-4c1d-8861-cc3562150728.
=============================================================

Using a Samsung Galaxy S3 Mini, affected builds of Firefox crash when visiting http://podiobooks.com/title/a-coffee-crusade/

Regression range is this:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=192cf71f768f&tochange=06340c449def
which points to the changes made in bug 1148102.

There's also an old bug (bug 812881) for this crash signature, but I don't know in how far it's relevant for the current problem.

Comment 1

[Jean-Yves Avenard [:jya]]

Code's from bug 1148102 isn't active and is turned off by default

The crash is deep within Android's stagefright ; we have no control over this.

Jan, some ideas?

Comment 2

[Jan Henning [:JanH]]

Unfortunately not. This came up while confirming a bug report on mozillazine which turned into bug 1188873.

Comment 3

[Jan Gerber]

That page has one audio element with an mp3[1], dont see how it could be related to webm.
The only place would be the hookup part. 

Possibly: CreateDecoder calls SupportsMimeType now, AndroidDecoderModule::SupportsMimeType would do something like this:

 return static_cast<bool>(mozilla::CreateDecoder(aMimeType));
 -> mozilla::CreateDecoder:
 NS_ENSURE_SUCCESS(MediaCodec::CreateDecoderByType(PromiseFlatCString(aMimeType).get(), &codec), nullptr);

-- 
1 -
<audio id="jp_audio_0" preload="metadata" src="http://media.podiobooks.com/coffeecrusade/PB-CoffeeCrusade-01.mp3"></audio>

Comment 4

[Jan Gerber]

Attachment: Bug-1188870-AndroidDecoderModule-supports-MP3.patch

If indeed the problem is that MediaCodec::CreateDecoderByType crashes, it might be an option to handle audio/mpeg like video/mp4 and return true without trying to create a decoder in AndroidDecoderModule::SupportsMimeType.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640789 [details] [diff] [review]
Bug-1188870-AndroidDecoderModule-supports-MP3.patch

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

sure, but how is that any different to what was before the webm patch ?

Comment 6

[Jean-Yves Avenard [:jya]]

oh, I see... SupportsMimeType is called in CreateDecoder()

However, Right before CreateDecoder() , in the MediaDecoderReader you have     NS_ENSURE_TRUE(IsSupportedAudioMimeType(mInfo.mAudio.mMimeType), false);

which also calls SupportsMimeType() .

So it would be the twice in a row call that would cause the crash ?

Likely another android bug being revealed (like bug 1186965).

Comment 7

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640789 [details] [diff] [review]
Bug-1188870-AndroidDecoderModule-supports-MP3.patch

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

Passing the review to snorp.

Does all android support mp3, like they all support AAC and h264 ? seems reasonable to think so

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8640789 [details] [diff] [review]
Bug-1188870-AndroidDecoderModule-supports-MP3.patch

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

This looks right, but we are supposedly already using AndroidDecoderModule for MP3. I'm confused how it could've ever worked without this patch. Passing review to Eugen.

Comment 9

[Eugen Sawin [:esawin]]

Comment on attachment 8640789 [details] [diff] [review]
Bug-1188870-AndroidDecoderModule-supports-MP3.patch

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

I don't have the device to reproduce the crash, so I can't say whether that's the right fix, but it looks good to me.
Audio playback remains broken, however that seems to a long-standing issue and is handled in bug 1188873.

@snorp: AndroidDecoderModule::SupportsMimeType returns true for "audio/mpeg" because mozilla::CreateDecoder should succeed in this case.

Comment 10

[Szabolcs Hubai (:xabolcs)]

STR from Bug 812881 comment #74 crashes my Firefox Beta for Android 40b10.


Crash report is at https://crash-stats.mozilla.com/report/index/ffb47ed7-5657-404d-b278-8a5702150806

Comment 11

[Jan Henning [:JanH]]

Any particular reason this has stalled? If it needs testing, I'm happy to test whether this patch has actually fixed the crash as long as someone can provide me with a local/try build.

Comment 12

[Jean-Yves Avenard [:jya]]

Here is a try push:

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

not sure how that could fix anything ... but time will tell.

Comment 13

[Jan Henning [:JanH]]

Mystery as it may be, but I'm happy to report that
- latest Incoming build is still broken
- that try build looks fixed, i.e. it doesn't crash neither on that audiobook site, nor upon MP3 playback in general

Comment 14

[Jean-Yves Avenard [:jya]]

All this indicates to me is that the current code is wrong, and things only work with AndroidDecoderModule::CreateDecoder() isn't called to check if the mimetype is supported.

Looking at the android source code, you can see that MediaCodec::CreateByType will return an initialized MediaCodec instance ; in the MediaCodec destructor; there's CHECK_EQ(mState, UNINITIALIZED); which will assert if the MediaCodec isn't in uninitialized stated.

The decoder is initialized asynchronously; as such the code is racy. It's possible that the destructor will be called whil mState isn't UNINITIALIZED and therefore will crash.

The code used to be only used for H264 and AAC for which the call to CreateDecoder is bypassed.

 I'll try to wrap a quick work around.

Comment 15

[Jean-Yves Avenard [:jya]]

Solution was simple actually:

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

Could you give this a try once compilation is done?

Also, could you check that webm player works for you?

This is to test for using HW acceleration works on Android thanks to bug 1190379.

You can find some webm samples there:
http://www.webmfiles.org/demo-files/

I don't know if your S3 mini supports hardware VP8/VP9 ; but at least this will also test that the patch for this bug properly handles it.

So regardless, it will be useful information

Comment 16

[Jan Henning [:JanH]]

Okay, regarding MP3 playback, the second Try build is working fine as well.

Regarding WebM, I've tried playing the Big Buck Bunny trailer, however both the Incoming and your second Try build are crashing after opening the file/pressing the playback button on the embedded player:
bp-2b6a53db-fd1c-40b4-87a6-d33f32150808 for an example from the Try build and
bp-8ffdb78e-1df3-4c3c-b1e5-5aeae2150808 for the Incoming build I've been using

On FF39, playback is still working normally, albeit with a few stutters. Should I file a new bug and get a regression range for those WebM crashes, or do you already have something on file?

Comment 17

[Jean-Yves Avenard [:jya]]

What do you call the incoming build?

webm with HW decoding isn't enabled yet. so just that may be the problem.

That the problem is fixed for MP3 indicates that the problem was indeed that the decoder wasn't released and triggered the assert in stagefright

Comment 18

[Jean-Yves Avenard [:jya]]

On second thought, if it's crashing with a standard nightly build ; then yes please lodge a bug. Likely a regression due to bug 1185792. Would be good to confirm.

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: Properly release decoder before destruction.

The comment in android's source code is rather explicit:
    // Client MUST call release before releasing final reference to this
    // object.

Comment 20

[Jean-Yves Avenard [:jya]]

Comment on attachment 8640789 [details] [diff] [review]
Bug-1188870-AndroidDecoderModule-supports-MP3.patch

This is just a work around the real problem.

Comment 21

[Jan Henning [:JanH]]

I meant the build from mozilla-incoming I had used to make sure that without your patch applied, Firefox was still crashing for MP3 files.

Regarding the crash, will do.

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8645356 [details] [diff] [review]
Properly release decoder before destruction.

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

Good catch

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8b24ff97b6

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8c8b24ff97b6

Comment 25

[Jan Henning [:JanH]]

Verified as working on yesterday's Nightly (build 20150810030205).
Thanks very much for fixing this.

@xabolcs: If this hasn't fixed your crash, and if your crash doesn't have the same regression window, it's probably best to open a new bug for it.