Crash in mozilla::gl::GLBlitHelper::BlitImage

REOPENED
Assigned to

Status

()

Firefox for Android
Audio/Video
P1
critical
REOPENED
21 days ago
6 hours ago

People

(Reporter: calixte, Assigned: snorp)

Tracking

(Blocks: 1 bug, {crash, regression, topcrash})

Trunk
Unspecified
Android
crash, regression, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+, firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58blocking fixed, firefox59 affected)

Details

(Whiteboard: [clouseau], crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

21 days ago
This bug was filed from the Socorro interface and is 
report bp-1331cca0-6270-431f-aa1c-dc02e0171101.
=============================================================

There are 61 crashes in nightly 58 with buildid 20171031234943. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1395497.

[1] https://hg.mozilla.org/mozilla-central/rev/b6aad6761527
Flags: needinfo?(snorp)
Up to 141 crashes now for the 10-31 Nightly.
Assignee: nobody → snorp
(Reporter)

Comment 2

18 days ago
The signature is ranked #1 in nightly 58 top-crashers for FennecAndroid (1738 crashes for 784 installs).
Keywords: topcrash
Huge crash, marking it as blocking.
tracking-firefox58: --- → blocking
Pretty sure this will be fixed by bug 1413230, which is just waiting for review.
Flags: needinfo?(snorp)
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> Pretty sure this will be fixed by bug 1413230, which is just waiting for
> review.

The fix for that bug landed several days ago, but I am still seeing crashes in this signature in 20171108111014.
Flags: needinfo?(snorp)
(In reply to Marcia Knous [:marcia - use ni] from comment #5)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> > Pretty sure this will be fixed by bug 1413230, which is just waiting for
> > review.
> 
> The fix for that bug landed several days ago, but I am still seeing crashes
> in this signature in 20171108111014.

Indeed. I'll look into it further.
Flags: needinfo?(snorp)
Whoops, found the problem.

Jeff, I need an adult. Shader fails to compile because we don't have GL_OES_EGL_image_external_essl3. Can we easily fall back to ES 2.0?

|[0][GFX1]: DrawBlitProg link failed:
progLog: Link Error: Fragment shader was not successfully compiled.

vsLog: Success.

fsLog: Compile failed.
ERROR: 0:3: Extension GL_OES_EGL_image_external_essl3 not supported
ERROR: 1 compilation errors. No code generated.
tracking-fennec: --- → +
Flags: needinfo?(jgilbert)
Priority: -- → P1
Comment hidden (mozreview-request)
I'm have this problem too. The  last  version of nightly for Android  crash on "x" nsfw  video sites.
369 occurrences in Nightly 20171110100131, which is enormously high -- making recenty Nightly builds probably more than 20x crashier than normal.

Please fix ASAP!

Comment 11

8 days ago
mozreview-review
Comment on attachment 8926968 [details]
Bug 1413500 - Disable video surface readback (for page thumbnail) on Android

https://reviewboard.mozilla.org/r/198188/#review204314

I don't think this does what you want it to do.

In the notes to GL_OES_EGL_image_external_essl3 it states that it's not allowed to support GL_OES_EGL_image_external_essl3 but not GL_OES_EGL_image_external. I think we should always use ESSL100 instead of ESSL300 because of this. 

Also, we should probably make this gfxCriticalNote, not gfxCriticalError, so that it doesn't crash because of this. (assuming we have a fallback?)
Attachment #8926968 - Flags: review?(jgilbert) → review-
status-firefox59: --- → affected
:snorp - Would it be possible to back this out while we continue working on a fix? Since this is in 58 we are worried about the beta audience.
Flags: needinfo?(snorp)
(In reply to Marcia Knous [:marcia - use ni] from comment #12)
> :snorp - Would it be possible to back this out while we continue working on
> a fix? Since this is in 58 we are worried about the beta audience.

I think we're gonna get a fix soon. If that doesn't happen we can certainly back it out.
Flags: needinfo?(snorp)
The backout required a bit of rebasing, but it looks good on Try so far.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=611e3676c8937ef5050426bb62dec68aa953fce5

I'd say that if we have a fix in time for Monday's b5 gtb, great. Otherwise, let's backout instead. We shouldn't ship another beta with this crash still not addressed.
Comment hidden (mozreview-request)

Comment 16

6 days ago
mozreview-review
Comment on attachment 8926968 [details]
Bug 1413500 - Disable video surface readback (for page thumbnail) on Android

https://reviewboard.mozilla.org/r/198188/#review205546

My concern here is that it's viable for an implementation to support OES_EGL_image_external without OES_EGL_image_external_essl3.
Now, it's not likely that an implementation with only image_external would deliberately support it in ES3 essl100 shaders but not implicitly have image_external_essl3 behavior.
But there's no reason to use essl3 here at all. We should only check EGL_image_external, and then supply essl100 shaders.
Attachment #8926968 - Flags: review?(jgilbert) → review-
(Assignee)

Comment 17

5 days ago
mozreview-review
Comment on attachment 8926968 [details]
Bug 1413500 - Disable video surface readback (for page thumbnail) on Android

https://reviewboard.mozilla.org/r/198188/#review205560
(Assignee)

Comment 18

5 days ago
mozreview-review-reply
Comment on attachment 8926968 [details]
Bug 1413500 - Disable video surface readback (for page thumbnail) on Android

https://reviewboard.mozilla.org/r/198188/#review205546

I tried that initially, but specifying "#version 100 es" didn't work on a ES3 context (which is what we get most of the time even if it's not WebGL2). I'll poke around some more.
Comment hidden (mozreview-request)

Comment 20

4 days ago
mozreview-review
Comment on attachment 8926968 [details]
Bug 1413500 - Disable video surface readback (for page thumbnail) on Android

https://reviewboard.mozilla.org/r/198188/#review206120
Attachment #8926968 - Flags: review?(jgilbert) → review+

Comment 21

4 days ago
mozreview-review-reply
Comment on attachment 8926968 [details]
Bug 1413500 - Disable video surface readback (for page thumbnail) on Android

https://reviewboard.mozilla.org/r/198188/#review205546

Ah yeah, essl100 is actually `#version 100`, even though essl300 is `#version 300 es`. (Historical reasons, but fwiw, there is no glsl100, so it all works out)
Flags: needinfo?(jgilbert)

Comment 22

4 days ago
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fec62780f47
Use GLSL 100 for blitting on ES r=jgilbert
https://hg.mozilla.org/mozilla-central/rev/2fec62780f47
Status: NEW → RESOLVED
Last Resolved: 4 days ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
1 failures in 762 pushes (0.001 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 1

Platform breakdown:
* android-4-3-armv7-api16: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1413500&startday=2017-11-13&endday=2017-11-19&tree=all
Hi :snorp,
Can you help request Beta approval for this?
Flags: needinfo?(snorp)
Hi Aryx,
Can you merge the patch to Beta directly?
Flags: needinfo?(aryx.bugmail)
https://hg.mozilla.org/releases/mozilla-beta/rev/8f6a7ac9d726
status-firefox58: affected → fixed
Looks like this is still broken.
Status: RESOLVED → REOPENED
Flags: needinfo?(snorp)
Resolution: FIXED → ---
Hi :snorp,
We might need to back out bug 1395497 and this bug from beta?
Flags: needinfo?(snorp)
Yes we should. And we should ship another Fennec beta ASAP with that backout included. Note that comment 14 has an already-rebased patch that is green on Try.
Hi Aryx,
Can you help backout bug 1395497 & this bug from beta?
Backed out from beta on request from gchang and RyanVM:

https://hg.mozilla.org/releases/mozilla-beta/rev/96ccac75a2f75ebaaf0bff6f99e8ec35fc8b97ec
status-firefox58: fixed → affected
Flags: needinfo?(aryx.bugmail)
fixed on 58 by backing out bug 1395497
status-firefox58: affected → fixed
status-firefox59: fixed → affected
Target Milestone: Firefox 59 → ---
Comment hidden (mozreview-request)
The latest patch should stop the bleeding, and fixes a problem we'd want to fix anyway.
Flags: needinfo?(snorp)
Jeff, the latest crashes all have the following shader compilation errors which I don't understand. Any ideas?

|[0][GFX1]: DrawBlitProg link failed:
progLog: L0101 All attached shaders must be compiled prior to linking
vsLog: 
fsLog: 3:2: L0001: Typename expected, found 'samplerExternalOES'
 (t=379.697)

"#extension GL_OES_EGL_image_external : require" is supposed to provide this. If that extension is missing (it's not, Android requires it in devices where SurfaceTexture is supported), surely the compiler would've complained about that line?
Flags: needinfo?(jgilbert)
Can we just buy an affected device? I don't think we can keep trying to fix it at a distance.

We also should really make this not fatal, instead using gfxCriticalNote.

I don't see how the compiler can spec-compliantly accept `#extension GL_OES_EGL_image_external : require` but not `samplerExternalOES`. I think we really need to experiment with an affected device.
Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.