1163814 - Fennec crash in nsBaseAppShell::RunSyncSectionsInternal(bool, unsigned int)

Status: VERIFIED FIXED

(Core :: Widget, defect)

Description

[(Away)]

This bug was filed from the Socorro interface and is 
report bp-1f60b7fe-d2e5-4510-9556-d98a42150510.
=============================================================

These crashes continue on Fennec only, after the fix for bug 1158761.

Comment 1

[Bobby Holley (:bholley)]

This one looks media-related. I'll look into it or find an owner.

Comment 2

[Bobby Holley (:bholley)]

So chris, it looks like HTMLMediaElement::SelectResourceWrapper can spin the event loop, which is verboten to do from a sync section (because it triggers more sync sections). Any thoughts on the best way to fix this?

Comment 3

[Catalin Suciu [:csuciu]]

Fennec crashes with this signature every time I'm loading http://www.w3schools.com/html/html5_video.asp
https://crash-stats.mozilla.com/report/index/af63e2e4-9e10-4aa3-a671-641672150513

Reproducing on Note 3 (4.4.2), Galaxy S5 (4.4.2), Alcatel OT (4.1.2) on Nightly 41 and Aurora 40.

Comment 4

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

http://hg.mozilla.org/mozilla-central/annotate/77d92f6d7679/dom/media/fmp4/MP4Decoder.cpp#l311
It looks like we just need to do |decoder->Shutdown()| asynchronously.

Comment 5

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

Attachment: 1163814_async_shutdown-v1.patch

Comment 6

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

Hi Catalin,
Can you try if the patch fix the problem for you on Fennec? Thanks.

Comment 7

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

Attachment: 1163814_async_shutdown-v2.patch

Please try this patch.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Bobby Holley (:bholley) from comment #2)
> So chris, it looks like HTMLMediaElement::SelectResourceWrapper can spin the
> event loop, which is verboten to do from a sync section (because it triggers
> more sync sections). Any thoughts on the best way to fix this?

JW's fix seems reasonable to me. Thanks JW!

Comment 9

[u421692]

I've pushed the patch above on a local build, and Fennec dies(without a crash report, but seems that crash reporter doesn't work on my local build). See log: https://pastebin.mozilla.org/8833442
There we're rejects when I've pushed the patch, so I'm not sure I've handled them correctly.
Can you please post a new patch?

Comment 10

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

I have no conflicts when applying the patch to the latest revision of Central. What is your revision number?

Comment 11

[Robert Kaiser]

[Tracking Requested - why for this release]:
This is by far the #1 crash on Android for 40 and 41, with a volume that doesn't really good good when turning on Aurora updates.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Should we consider backing out the bug that caused this regression?

Comment 13

[u421692]

(In reply to JW Wang [:jwwang] from comment #10)
> I have no conflicts when applying the patch to the latest revision of
> Central. What is your revision number?

Sorry about that, it seems to work after downloading new sources. I'm going to build again and retest.

Comment 14

[u421692]

Able to build without any issues with the patch.
Fennec still crashes when trying to load http://www.w3schools.com/html/html5_video.asp
Tested on Samsung Galaxy S5 (Android 4.4.2)
See logs: https://pastebin.mozilla.org/8833454

Comment 15

[jen strickland :jenstrickland]

Tracking enabled for 40 and 41, since it's a high volume / visible crash on Android.

Comment 16

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

FYI, we're holding on re-enabling Fennec Aurora updates until we have a fix for this bug. We're scheduled to re-enable updates tomorrow (May 15).

Comment 17

[Bobby Holley (:bholley)]

I disabled the MOZ_RELEASE_ASSERT on android for aurora only as a stop-gap measure. Mihai, can you confirm that the crash is gone on the aurora branch?

https://hg.mozilla.org/releases/mozilla-aurora/rev/3fbb370b781b

Comment 18

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

(In reply to Mihai Pop from comment #14)
> Able to build without any issues with the patch.
> Fennec still crashes when trying to load
> http://www.w3schools.com/html/html5_video.asp
> Tested on Samsung Galaxy S5 (Android 4.4.2)
> See logs: https://pastebin.mozilla.org/8833454

Can't find stack trace in the logs...

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

We can always add an "#ifdef MOZ_WIDGET_ANDROID \n return true;" inside MP4Decoder::CanCreate*Decoder() as an additional stop-gap.

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Or perhaps a quicker fix is to make the body of MP4Decoder::CanCreate*Decoder() #ifdef XP_WIN, and return true on other platforms. This code is also causing bug 1164480.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: Only check if we can create decoder on Windows

Only check to see if we can create a decoder in MP4Decoder::CanCreate*Decoder() on Windows, since it's causing crashes and problems on Android and MacOSX (bug 1164480).

We only really need this on Windows, I think we don't incorrectly detect whether the decoders are available on other platforms.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 23

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

android.media.MediaCodec requires Android API level >= 16. However our Try includes Android 4.0 API11+ only. There is no way to test this bug on Try...

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

m-c doesn't build fennec for me locally, so I can't test locally either. But I'm sure effectively disabling the code should fix it. Mostly I just want to ensure I don't get bitten by warnings-as-errors (which I did, that's why I did a second try push...)

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2: Only check if we can create decoder on Windows

Build bustage fixed, carry forward r=mattwoodrow.

Try is looking promising.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64c63327cac6

Comment 27

[u421692]

(In reply to Bobby Holley (:bholley) from comment #17)
> I disabled the MOZ_RELEASE_ASSERT on android for aurora only as a stop-gap
> measure. Mihai, can you confirm that the crash is gone on the aurora branch?
> 
> https://hg.mozilla.org/releases/mozilla-aurora/rev/3fbb370b781b

Tested on:
Alcatel One Touch 8008D (Android 4.1.2)
Samsung Galaxy S5 (Android 4.4.2)
On Aurora 40.0a2 (2015-05-15) it doesn't crash when loading http://www.w3schools.com/html/html5_video.asp
Still reproducible on latest Nightly

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3f23693794

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f3f23693794

Comment 31

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f3f23693794

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8606226 [details] [diff] [review]
Patch v2: Only check if we can create decoder on Windows

Approval Request Comment
[Feature/regressing bug #]: bug 1160321 which is required to be uplift for YouTube & EME support

[User impact if declined]: I won't be able to uplift bug 1160321, which means Firefox can attempt to launch EME when it's not going to work. The same thing can also happen on YouTube. With this patch and bug 1160321, we can detect ahead of time, and the site can fallback.

[Describe test coverage new/current, TreeHerder]: We have lots of EME/video mochitests.

[Risks and why]: We added code in bug 1160321 to check that creating an H.264 & AAC decoder works before reporting to Content that we can play H.264 & AAC. This patch just limits us to only do that on Windows, as that's where this is needed, and where we know it works. So I this this is low risk.

[String/UUID change made/needed]: None

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

Must remember to uplift...

Comment 34

[Catalin Suciu [:csuciu]]

Verifying as fixed on latest Nightly.

Comment 35

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

Comment on attachment 8606226 [details] [diff] [review]
Patch v2: Only check if we can create decoder on Windows

Verified patch required for uplift of bug 1160321. Beta+ Aurora+

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Catalin (or Mihai) can you verify this with 39.0b1 build 1? It should be ready for testing tomorrow. Thanks!

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b7ddc9e56c29

Comment 39

[Chris Pearce [:cpearce (Not reading bugmail)]]

Try of Beta 39 push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=653bd3d35f0c

Comment 40

[Catalin Varga [QA][:VarCat]]

Setting the needinfo for the mobile QA team.

Comment 41

[Catalin Suciu [:csuciu]]

Verifying as fixed on Firefox 39.0b1 and Aurora 40.0a2 (2015-05-20)   
Tested on Galaxy Note 3 (4.4.2) and Alcatel OT (4.1.2)

Comment 42

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

(In reply to Mihai Pop from comment #27)
> (In reply to Bobby Holley (:bholley) from comment #17)
> > I disabled the MOZ_RELEASE_ASSERT on android for aurora only as a stop-gap
> > measure. Mihai, can you confirm that the crash is gone on the aurora branch?
> > 
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/3fbb370b781b
> 
> Tested on:
> Alcatel One Touch 8008D (Android 4.1.2)
> Samsung Galaxy S5 (Android 4.4.2)
> On Aurora 40.0a2 (2015-05-15) it doesn't crash when loading
> http://www.w3schools.com/html/html5_video.asp
> Still reproducible on latest Nightly

Bug 1164453 explains why this doesn't work.

https://hg.mozilla.org/mozilla-central/file/cb33535b93f2/dom/media/fmp4/MP4Decoder.cpp#l267

Since CreateTestH264Decoder is the only place to pass a null MediaDataDecoderCallback, we can treat null MediaDataDecoderCallback as an indication to test decoder capability without actual decoding. By doing so, we can revert changes in this bug and bug 1164453 to make MP4Decoder::CanCreateH264Decoder() work more precisely by checking if a decoder with the specific config can be created. Does that make sense?

Comment 43

[Chris Pearce [:cpearce (Not reading bugmail)]]

We only call CreateTestH264Decoder() on Windows now, so this shouldn't be an issue on Fennec anymore. Do we need to do this check on other platforms?

Comment 44

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

Right, it won't be an issue on Fennec anymore. However, my intention is to remove platform-dependent ifdefs so the code is consistent on all platforms which are also less a headache to test.