Closed Bug 1163814 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Widget, defect)

Unspecified
Android
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox39 --- verified
firefox40 + verified
firefox41 + verified

People

(Reporter: away, Assigned: cpearce)

References

Details

(Keywords: crash, reproducible)

Crash Data

Attachments

(2 files, 2 obsolete files)

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.
This one looks media-related. I'll look into it or find an owner.
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(bobbyholley) → needinfo?(cpearce)
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.
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.
Hi Catalin,
Can you try if the patch fix the problem for you on Fennec? Thanks.
Flags: needinfo?(catalin.suciu)
Please try this patch.
Attachment #8605160 - Attachment is obsolete: true
(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!
Flags: needinfo?(cpearce)
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?
Flags: needinfo?(catalin.suciu) → needinfo?(jwwang)
I have no conflicts when applying the patch to the latest revision of Central. What is your revision number?
Flags: needinfo?(jwwang) → needinfo?(mihai.g.pop)
[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.
Should we consider backing out the bug that caused this regression?
(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.
Flags: needinfo?(mihai.g.pop)
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
Tracking enabled for 40 and 41, since it's a high volume / visible crash on Android.
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).
Flags: needinfo?(jwwang)
Flags: needinfo?(cpearce)
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
Flags: needinfo?(mihai.g.pop)
(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...
We can always add an "#ifdef MOZ_WIDGET_ANDROID \n return true;" inside MP4Decoder::CanCreate*Decoder() as an additional stop-gap.
Flags: needinfo?(cpearce)
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.
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.
Attachment #8606114 - Flags: review?(matt.woodrow)
Attachment #8606114 - Flags: review?(matt.woodrow) → review+
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...
Flags: needinfo?(jwwang)
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...)
Build bustage fixed, carry forward r=mattwoodrow.

Try is looking promising.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64c63327cac6
Assignee: nobody → cpearce
Attachment #8606114 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8606226 - Flags: review+
(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
Flags: needinfo?(mihai.g.pop)
https://hg.mozilla.org/mozilla-central/rev/0f3f23693794
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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
Attachment #8606226 - Flags: approval-mozilla-beta?
Attachment #8606226 - Flags: approval-mozilla-aurora?
Must remember to uplift...
Flags: needinfo?(cpearce)
Verifying as fixed on latest Nightly.
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+
Attachment #8606226 - Flags: approval-mozilla-beta?
Attachment #8606226 - Flags: approval-mozilla-beta+
Attachment #8606226 - Flags: approval-mozilla-aurora?
Attachment #8606226 - Flags: approval-mozilla-aurora+
Catalin (or Mihai) can you verify this with 39.0b1 build 1? It should be ready for testing tomorrow. Thanks!
Flags: qe-verify+
Flags: needinfo?(catalin.varga)
Setting the needinfo for the mobile QA team.
Flags: needinfo?(catalin.varga) → needinfo?(catalin.suciu)
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)
Status: RESOLVED → VERIFIED
Flags: needinfo?(catalin.suciu)
(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?
Flags: needinfo?(cpearce)
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?
Flags: needinfo?(cpearce)
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.
Flags: qe-verify+ → qe-verify-
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.