1288446 - Crash in libGLESv2_POWERVR_SGX540_120.so@0x4d88

Status: RESOLVED FIXED

(Firefox for Android :: Toolbar, defect, P2)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-b1a54e09-f3da-4736-abe1-b984e2160720.
=============================================================

Seen while reviewing crash stats: http://bit.ly/29PSDVT and also shows up on the 7-19 explosive report.

Crashes are present primarily on 47 but also affect the latest 48 beta.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

47.0 had 19401 of these crashes since July 1, 48.0 is on track to match or exceed that, and 49.0 beta is the same. It's #18 on the topcrasher list so if there's something we can try to fix this we should probably try it.

snorp/jnicol, any thoughts?

Comment 2

[Jamie Nicol [:jnicol]]

The stacktraces seem pretty badly corrupted, but quite a few seem to indicate a problem when Skia is uploading texture data.

We have a Samsung Galaxy Tab 2 7.0 P3110 (3% of crashes) in London. If I can get it to charge I'll try to investigate, but it unfortunately uses a special cable which looks like it might be broken. If that's the case I'll order a new device through service now.

Comment 3

[Jamie Nicol [:jnicol]]

This was indeed by the Skia m49 upgrade (bug 1246756). The specific change is in GrGLGpu::createRenderTargetObjects(). The call to glCheckFramebufferStatus() after glFramebufferTexture2D() is now only called once rather than for every render target we create.

I have worked out that the crash occurs when:
1. Texture A is rendered from (e.g. glDrawElements with it bound to a texture unit and accessed from the shader)
2. A new skia canvas is created. So a Framebuffer is created and bound, and Texture B is created and attached to the framebuffer. But we no longer call glCheckFramebufferStatus.
3. Texture A is uploaded to with glTexSubImage2D.

So it appears there's a driver bug to do with the contentious read and write of texture A. I have absolutely no idea why a call to glCheckFramebufferStatus avoids the crash, but it does.

I don't want to write invasive code for a bug that only affects a small number of devices, so I think the best course of action is to simply always call glCheckFramebufferStatus after glFramebufferTexture2D on affected devices.

Comment 4

[Jamie Nicol [:jnicol]]

Attachment: Bug 1288446 - Call glCheckFramebufferStatus after glFramebufferTexture2D to avoid driver crash;

Some PowerVR SGX540 devices running Android 4.0 were crashing when
uploading texture data with glTexSubImage2D. This was unearthed by a
Skia upgrade which removed what should have been an unnecessary OpenGL
call. This reinstates the OpenGL call which prevents the crash, but at a
GLContext level rather than in Skia.

Review commit: https://reviewboard.mozilla.org/r/77652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77652/

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Does this have any performance impact? Are we just avoiding the problem by causing a flush?

Comment 6

[Jamie Nicol [:jnicol]]

I think it could have a performance impact on some devices. Presumably that's why Skia added the optimisation of not calling it every time. However, causing glFlush (or glFinish) doesn't prevent this crash, so I don't think that's what's going on here.

We also already call this ourselves fairly frequently when binding FBOs. And this brings up back to calling it pretty much the same amount as we used to, so while it could be a regression from very recent releases it wouldn't be a longer term one.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8789413 [details]
Bug 1288446 - Call glCheckFramebufferStatus after glFramebufferTexture2D to avoid driver crash;

https://reviewboard.mozilla.org/r/77652/#review77800

Comment 8

[Jamie Nicol [:jnicol]]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0a451335c43
Call glCheckFramebufferStatus after glFramebufferTexture2D to avoid driver crash; r=jrmuizel

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0a451335c43

Comment 11

[Jamie Nicol [:jnicol]]

Comment on attachment 8789413 [details]
Bug 1288446 - Call glCheckFramebufferStatus after glFramebufferTexture2D to avoid driver crash;

Let's get this in 50 and 51.

Approval Request Comment
[Feature/regressing bug #]: bug 1246756
[User impact if declined]: Frequent crashes on affected devices on pages using canvas
[Describe test coverage new/current, TreeHerder]: Try run in comment 8
[Risks and why]: Low. Only changes behaviour on affected devices. Reinstates removed OpenGL function call.
[String/UUID change made/needed]: N/A

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8789413 [details]
Bug 1288446 - Call glCheckFramebufferStatus after glFramebufferTexture2D to avoid driver crash;

The crash volume of this one is very low but since it's still early 50 beta cycle, let's take it. Aurora51+, Beta50+

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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