1243639 - crash in libGLES_mali.so@0x107c8 on Mali-400 MP

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

This bug was filed from the Socorro interface and is 
report bp-c440aa28-43d2-4000-98ab-e60dc2160127.
=============================================================

Mfg.    Model           And. API    CPU ABI      #      
samsung SM-T560         19 (REL)  armeabi-v7a  10507  63.291%
samsung SM-T561         19 (REL)  armeabi-v7a   3807  22.932%
samsung SM-J110H        19 (REL)  armeabi-v7a   1042   6.277%
HUAWEI HUAWEI Y336-U02  19 (REL)  armeabi-v7a    726   4.373%
samsung SM-T561M        19 (REL)  armeabi-v7a    281   1.693%

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Sotaro, anything you can do to help?

Comment 2

[Sotaro Ikeda [:sotaro]]

From the log, crash happened within libGLES_mali.so. It seems not clear why the crash happened. Last gecko's function call was EGLImageTextureSource::BindTexture(). It caused the crash. It actually does not allocate GL resource. It just bind EGLImage to GL texture.

Comment 3

[Sotaro Ikeda [:sotaro]]

One possibility is that EGLImage was not valid anymore when EGLImageTextureSource::BindTexture() was called.

Comment 4

[Sotaro Ikeda [:sotaro]]

EGLImageTextureSource is created by EGLImageTextureHost. EGLImageTextureHost's peers are EGLImageTextureData or SharedSurface_EGLImage.

Comment 5

[Sotaro Ikeda [:sotaro]]

SharedSurface_EGLImage is created for SkiaGL and WebGL.
EGLImageTextureData is created for EGLImageImage. EGLImageImage is created by nsPluginInstanceOwner.

Comment 6

[Sotaro Ikeda [:sotaro]]

snorp: Do you know if plugin is still enabled and used on Fennec? If it is used, how can test it?

Comment 7

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

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> snorp: Do you know if plugin is still enabled and used on Fennec? If it is
> used, how can test it?

Yeah. We support Flash if it's installed. You can get it from here: http://download.macromedia.com/pub/flashplayer/installers/archive/android/11.1.115.81/install_flash_player_ics.apk

Comment 8

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

Hmm. The EGLImageImage code in nsPluginInstanceOwner should only be used on Honeycomb, which we no longer support. It looks like people have installed the wrong plugin on a ICS+ device so it's not trying to use the SurfaceTexture API. I'm not sure if there is a good way we can detect this, but it might be enough to just return an error when a plugin tries to request the Honeycomb API. I can take this and write that patch.

Comment 9

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

Actually, Dylan can you do this? It should mostly just be removing some code.

Make this[0] request from the plugin return an error and remove ANPOpenGL.cpp[1]. Then you can remove nsNPAPIPluginInstance::LockContentTexture[2] and the associated machinery.

[0] https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp?case=true&from=nsNPAPIPlugin.cpp#2150
[1] https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/android/ANPOpenGL.cpp
[2] https://dxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPluginInstance.cpp?from=nsNPAPIPluginInstance.cpp#947

Comment 10

[Dylan Roeh (:droeh) (he/him)]

Attachment: Proposed patch

Comment 11

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

Comment on attachment 8725255 [details] [diff] [review]
Proposed patch

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

lgtm, what kind of testing did you do? You should:

1) Make sure the ICS version of Flash works on a ICS+ device
2) Make sure the Gingerbread version of Flash does not crash on a ICS+ device.

I fear that 2) may not be true. In that case we may need to figure out something else.

Comment 12

[Dylan Roeh (:droeh) (he/him)]

Yup, I tested both ICS and pre-ICS flash on my Nexus 6 and got the desired behavior.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdee9d5be90

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

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

Is this fix verified? If so, is this upliftable to 46?

Comment 16

[Kevin Brosnan [Ex-Mozilla]]

There are no reported crashes on 47.0a2 or 48.0a1 so it is hard to say if this is resolved on Nightly. Landing on beta should be conclusive as there are a few hundred crashes on that release. http://mozilla.github.io/bug-signatures-status/#/bug/1243639

Comment 17

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8725255 [details] [diff] [review]
Proposed patch

Approval Request Comment
[Feature/regressing bug #]: 721741
[User impact if declined]: Potential crashes if the wrong version of flash is installed on ICS+ devices.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b229bb6fdbb
[Risks and why]: Should be low-risk, the patch removes code that is only used for the Gingerbread version of flash.
[String/UUID change made/needed]:

Comment 18

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

Comment on attachment 8725255 [details] [diff] [review]
Proposed patch

This landed on 47 (which is now aurora) so no need to request uplift to that channel.

Comment 19

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

Comment on attachment 8725255 [details] [diff] [review]
Proposed patch

Crash fix, hooray! This should land tonight or tomorrow but would not affect Fennec until the beta 6 build next Monday.

Comment 20

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

It would be good to verify this fix some time at the end of next week, if this makes it into beta 6.

Comment 21

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

https://hg.mozilla.org/releases/mozilla-beta/rev/6c4280f3850b

Comment 22

[u279076]

(In reply to Wes Kocher (:KWierso) from comment #21)
> https://hg.mozilla.org/releases/mozilla-beta/rev/6c4280f3850b

This remains a topcrash in Fennec 47 with 21,165 crashes reported over the last week. Should this get reopened or a new bug filed?

Comment 23

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

New bug, please.

Comment 24

[miralobontiu]

Hello,

Is qe-verify+ flag still valid?

Thank you!

Comment 25

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

Hi miralobontiu, for this bug, it probably doesn't need verification at this point.  But, since it is based on crash reports, you can click through on the crash signature field, which will show if there are any current reports with the same crash signature: https://crash-stats.mozilla.com/signature/?signature=libGLES_mali.so%400x107c8   There are a few, but you might notice they are from versions 47 beta 6 and earlier.    Because there aren't any crashes with current versions, we know this bug is fixed, and it was probably from the patch in this bug.