1232911 - GIFV files crash Firefox

Status: RESOLVED FIXED

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

Description

[Captain Packrat]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151208100201

Steps to reproduce:

Open a link to a GIFV file, such as https://imgur.com/YilRiRQ (work safe).


Actual results:

The web page is displayed for a split second, then the Firefox crash report screen appears.


Expected results:

The web page should load and the video should play.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

This likely will be device specific. Please let us know the model and the Android version. Getting a crash ID would be a great help as well, https://support.mozilla.org/en-US/kb/mozillacrashreporter

Comment 2

[Captain Packrat]

Samsung Galaxy Note 10.1 (GT-N8013) with Android 4.1.2.

There is no details button on the crash report screen in Firefox for Android, and the about:crashes only lists one report from over a month ago, even though I've submitted dozens of reports since.  I think this is the same issue, however, it's been happening for a couple months.  The report is here:  https://crash-stats.mozilla.com/report/index/5abd28ff-c12a-4948-bbe2-62b802151124

Comment 3

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

It seems there is an OS update for your device that might fix this issue. Can you give that a try?

Comment 4

[Captain Packrat]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> It seems there is an OS update for your device that might fix this issue.
> Can you give that a try?

Several of the N80xx models have updates to 4.4.2, but for some reason Samsung decided to screw everyone who bought the N8013 (which appears to be the USA WiFi-only model), it's still stuck at 4.1.2.  http://samsung-updates.com/device/?id=gt-n8013  The last update we got was a security update (probably for Stagefright) in September.  I have installed this update, nothing else is available through OTA or Kies.

Comment 5

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

The best we can do here is block using the Android hardware decoder for WebM on this device.

To do this, block the Android PDM from returning true for VP8 or VP9 in  AndroidDecoderModule::SupportsMimeType(). You can use VPXDecoder::IsVPX(aMimeType) to tell if aMimeType is VP8 or VP9.

Comment 6

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

Eugen, looks like we need an easy way to block some devices from using MediaCodec for WebM.

Comment 7

[Eugen Sawin [:esawin]]

We can just reuse the abandoned patches from bug 1192539.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1232911-1.1-Allow-to-test-for-specific-VPX-MIME-.patch

We need to test for the specific VPX version in ADM in TranslateMimeType and to be future-proof in the new blacklisting mechanics.

Alternatively, we can add dedicated IsVP8 and IsVP9 functions.

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1232911-2.1-Add-VPX-decoding-support-blocking.-r.patch

Allows us to block devices with broken VPX support.

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1232911-3.1-Block-VPX-support-in-ADM-on-unsuppor.patch

Use the blocking mechanics in ADM.

Comment 11

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

Comment on attachment 8732993 [details] [diff] [review]
0001-Bug-1232911-1.1-Allow-to-test-for-specific-VPX-MIME-.patch

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

I'll let jya make the call about this.

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8732993 [details] [diff] [review]
0001-Bug-1232911-1.1-Allow-to-test-for-specific-VPX-MIME-.patch

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

::: dom/media/platforms/agnostic/VPXDecoder.cpp
@@ +218,2 @@
>  {
> +  return aCodecMask & VPXDecoder::VP8 &&

Please add parenthesis across & and &&, this will cause warnings if compiled with clang

  return ((aCodecMask & VPXDecoder::VP8) &&
         aMimeType.EqualsLiteral("video/webm: codecs=vp8")) ||
         ((aCodecMask & VPXDecoder::VP9) &&
         aMimeType.EqualsLiteral("video/webm: codecs=vp9"));

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1232911-1.2-Allow-to-test-for-specific-VPX-MIME-.patch

Fixed a typo in the MIME type.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1232911-2.2-Add-VPX-decoding-blocking-support.-r.patch

We need to handle the new features' case in GetPrefNameForFeature, however, we cannot provide a pref name for them, otherwise we would try to access prefs off main thread.

With this patch, we would avoid any assertions and OMT prefs access without having to re-implement/change GfxInfo state handling, but I think it's not a nice workaround. I'm open for alternative ideas.

Comment 15

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1232911-3.2-Block-VPX-support-in-ADM-on-unsuppor.patch

Cosmetic fixes.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dfc5468af1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/36e706a8833e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf86e5e944f

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5dfc5468af1c
https://hg.mozilla.org/mozilla-central/rev/36e706a8833e
https://hg.mozilla.org/mozilla-central/rev/cbf86e5e944f