1209612 - crash in mozilla::gl::GLLibraryEGL::InitExtensionsFromDisplay Android API 10

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

This bug was filed from the Socorro interface and is 
report bp-a6241c57-d48f-4f1e-acec-2c3442150928.
=============================================================

samsung	GT-I9070
samsung	GT-I8160
samsung	GT-I8530
samsung	GT-I8160L
samsung	YP-G70
samsung	GT-I8160P
samsung	GT-I9070P
Motorola XT760
SAMSUNG	SAMSUNG-SGH-I897

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

For the other signature the following API 10 devices

samsung	GT-I9000
samsung	GT-P1000
samsung	SCH-I110
samsung	SAMSUNG-SGH-I997
samsung	SHW-M110S
samsung	SCH-S720C
samsung	GT-P1000L
samsung	SCH-I500
samsung	SCH-I510
Samsung	SPH-D700

Comment 2

[u279076]

It looks like the top device is the Samsung GT-I9070 @ 47%:
http://www.gsmarena.com/samsung_i9070_galaxy_s_advance-4469.php

FWIW, this is also the top crash on that device @ 30%.

Comment 3

[u279076]

Unfortunately the GFX team does not have one of these devices in our inventory. Roland said he can do some testing on his Samsung Gingerbread devices but that's probably just a shot in the dark.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Recent regression, tracking.

Margaret,

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Margaret, you might be interested by this bug (#3 crash)

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

This is a platform issue. The affected devices are variants of the first generation Galaxy S phone. Acquiring devices will take searching ebay or maybe your local craigslist they have been out of stores for ages.

Comment 7

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

We should also be able to look at the code and figure something out; this is a startup crash, perhaps a race condition with things not being initialized in time for us to use them?

Comment 8

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

I'm guessing a regression from bug 1182547.

Comment 9

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

Attachment: Watch for nullptr. r=snorp

A bit speculative, but it stands to reason some may have issue with the null display.

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8671918 [details] [diff] [review]
Watch for nullptr. r=snorp

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

We seem to be deliberately doing this, so we don't want to skip this. The caller says:

    //Initialize client extensions
    InitExtensionsFromDisplay(EGL_NO_DISPLAY);

Comment 11

[Kelsey Gilbert [:jgilbert]]

It could also just be us not calling eglInitialize. Regardless, we can do this the right way.

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Use-the-default-display-for-checking-for-ANGLE.patch

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Jeff, are you planning to land the patch? If yes, are you planning to request the uplift to aurora / beta?
Today is the last day for 42. Thanks

Comment 14

[Kelsey Gilbert [:jgilbert]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #13)
> Jeff, are you planning to land the patch? If yes, are you planning to
> request the uplift to aurora / beta?
> Today is the last day for 42. Thanks

We definitely need to land it on trunk first.

Comment 15

[Kelsey Gilbert [:jgilbert]]

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

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Jeff, Milan, what is the plan with this bug? Thanks
Today is the go to build for mobile beta 10.

Comment 17

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

The patch is causing Windows drivers to crash, so we're definitely not going to land it today.  Jeff is looking at it, I'll leave needinfo on.

Comment 18

[Kevin Brosnan [Ex-Mozilla]]

This seems to be a large part of a noticeable regression in the crash rate in release users of Firefox for Android. It is the number 1 top crash accounting for 13.3% of crashes. The volume of this crash is equal to the next 3 or 4 crashes.

Comment 19

[Kevin Brosnan [Ex-Mozilla]]

[Tracking Requested - why for this release]: comment 18

Comment 20

[Kelsey Gilbert [:jgilbert]]

I'll fix this tomorrow.

Comment 21

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

Tracking since it's a huge topcrash.

Comment 22

[Kelsey Gilbert [:jgilbert]]

I think I have a solution for now.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3df8ae69c6ef

Comment 23

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Don-t-call-eglQueryString-null-on-Android-for-now.patch

Comment 24

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

The review is coming, but :jgilbert, can you get us patch(es) that apply to 44, 43 and 42 (in case they're different.)

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8686415 [details] [diff] [review]
0002-Don-t-call-eglQueryString-null-on-Android-for-now.patch

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

Can you walk me through what you're all changing? The summary of this patch makes it seem like it should be much smaller than it is.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Try is clean.
I'll get the uplift patches.

Comment 27

[Kelsey Gilbert [:jgilbert]]

I'm going to make a more minimal patch for uplift.

Comment 28

[Kelsey Gilbert [:jgilbert]]

Attachment: (for uplift) 0001-Only-QueryString-with-null-if-supported.patch

Comment 29

[Kelsey Gilbert [:jgilbert]]

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

Comment 30

[Kelsey Gilbert [:jgilbert]]

The uplift patch applies cleanly to both beta and release. I had to re-clone aurora, but it probably applies there as well.

Comment 31

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8686768 [details] [diff] [review]
(for uplift) 0001-Only-QueryString-with-null-if-supported.patch

Approval Request Comment
[Feature/regressing bug #]: -
[User impact if declined]: top-crash on android
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: very low
[String/UUID change made/needed]: none

Someone should test this on an affected device before respinning a build, though.

Comment 32

[Pulsebot]

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

Comment 33

[Carsten Book [:Tomcat]]

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

Comment 34

[Kelsey Gilbert [:jgilbert]]

I guess we can just track if this fixes the topcrash on nightly before spinning to other branches?

Comment 35

[u279076]

Here are the current stats:
> Fennec 45.0a1:  #38 @  0.35% (- 1.11%)
> Fennec 44.0a2: #119 @  0.12% (+ 0.00%)
> Fennec 43.0b1:   #1 @ 11.14% (-12.30%) 
> Fennec 42.0:     #1 @ 11.54% (- 9.06%)

Based on this I don't think we're going to see much movement in Nightly and volume on Beta/Release suggests we should get this to those users ASAP. Also, the numbers dropping significantly on Beta/Release might suggest people switching away from Fennec because of this issue.

If the risk isn't high I would like to see this uplifted to at least Beta. Flagging Kevin just in case he has a different read.

Comment 36

[Kevin Brosnan [Ex-Mozilla]]

We will need this landed on beta to be able to understand the impact. Users with 4-5 year old phones are not running Nightly or Aurora. There is only one beta Firefox for Android release per week. This would need to be on beta before Monday's go to build. Liz we really would like to test this fix in beta 4.

Comment 37

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

Nightly crash volume for Android isn't always a good guide. For example there are only 3 crashes on 45 in the last week, with the mozilla::gl::GLLibraryEGL::InitExtensionsFromDisplay signature and none for the other sig.  So testing on an affected build is probably the way.   

I can check back Sunday night or Monday morning so that we can still get this landed for the beta 4 build on Monday.

Comment 38

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

Comment on attachment 8686768 [details] [diff] [review]
(for uplift) 0001-Only-QueryString-with-null-if-supported.patch

This looks ok on m-c, approved for uplift to aurora and beta. 
We want to assess this fix on beta 4, gtb on Monday.

Comment 39

[Andy]

Under the guidance of [e-mail] I've tested this on my personal device: Samsung Galaxy S Advance (Android 2.3.6).
Tested on Firefox 43 Beta 2, it crashes with a clean profile, after restaring there is no crash, but Firefox is unusable, web pages are not loading.
Tested on latest Nightly (2015-11-15) build http://ftp.mozilla.org/pub/mobile/nightly/latest-mozilla-central-android-api-9/fennec-45.0a1.multi.android-arm.apk and Firefox starts without crashing,
and I can open web pages.

Comment 40

[u421692]

Marking this verified as fixed based on the previous comment. Andy will help us test this when patches will be uplifted to Beta and Release. Thanks Andy for the support.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/687f98cac212

Comment 42

[Carsten Book [:Tomcat]]

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

Comment 43

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

If this improves the world for the beta users, we should strongly consider getting 42.0.1 with this in it (together with bug 1198663)

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/687f98cac212

Comment 45

[Andy]

This issue does not reproduce anymore on Samsung Galaxy S Advance (Android 2.3.6), on Firefox 43 Beta 4 http://ftp.mozilla.org/pub/mobile/candidates/43.0b4-candidates/build1/android-api-9/multi/fennec-43.0b4.multi.android-arm.apk

Comment 46

[u421692]

Marking this verified as fixed based on the previous comment. Thanks Andy for testing.

Comment 47

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

Jeff, looks like this may be included in a 42.0.1 for mobile. Can you request uplift to m-r? Thanks.

Comment 48

[Kelsey Gilbert [:jgilbert]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #47)
> Jeff, looks like this may be included in a 42.0.1 for mobile. Can you
> request uplift to m-r? Thanks.

Already requested!

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-release/rev/c94a38c79136

Comment 50

[Andy]

This issue does not reproduce anymore on Samsung Galaxy S Advance (Android 2.3.6), on Firefox 42.0.1.

Comment 51

[u549602]

Marking this verified as fixed based on the previous comment.