737949 - Remove GLThread, don't initialize EGL from Java code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

It first appeared in 14.0a1/20120319 and occurs on Galaxy Nexus at startup.
The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e94edfdb1f5b&tochange=58a2cd0203ee

Signature 	libGLESv2_POWERVR_SGX540_120.so@0xb8ae More Reports Search
UUID	b2640624-477f-494e-9317-122952120321
Date Processed	2012-03-21 13:26:39
Uptime	2
Last Crash	2.5 hours before submission
Install Age	1.9 hours since version was first installed.
Install Time	2012-03-21 11:34:33
Product	FennecAndroid
Version	14.0a1
Build ID	20120321031151
Release Channel	nightly
OS	Linux
OS Version	0.0.0 Linux 3.0.8-g8e0f3bc #1 SMP PREEMPT Wed Dec 14 18:52:51 PST 2011 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x0
App Notes 	
EGL? EGL+ AdapterVendorID: tuna, AdapterDeviceID: Galaxy Nexus.
AdapterDescription: 'Android, Model: 'Galaxy Nexus', Product: 'yakju', Manufacturer: 'samsung', Hardware: 'tuna''.
samsung Galaxy Nexus
google/yakju/maguro:4.0.1/ITL41F/228551:user/release-keys
EMCheckCompatibility	True

Frame 	Module 	Signature [Expand] 	Source
0 	libc.so 	libc.so@0xdbac 	
1 	libGLESv2_POWERVR_SGX540_120.so 	libGLESv2_POWERVR_SGX540_120.so@0xb8ae 	
2 	libGLESv2_POWERVR_SGX540_120.so 	libGLESv2_POWERVR_SGX540_120.so@0xda66 	
3 	libGLESv2_POWERVR_SGX540_120.so 	libGLESv2_POWERVR_SGX540_120.so@0x5a38e 	
4 	libGLESv2_POWERVR_SGX540_120.so 	libGLESv2_POWERVR_SGX540_120.so@0xebc6 	
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=libGLESv2_POWERVR_SGX540_120.so%400xb8ae

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Bug 735353 is within libGLESv1_POWERVR_SGX540_120.so; this bug is wihin libGLESv2_POWERVR_SGX540_120.so

Comment 5

[Scoobidiver (away)]

With all the dupes, it's no longer specific to Nexus.

Comment 6

[Scoobidiver (away)]

There are different versions of this DLL, but I don't think crashes in 0x1..., 0x6..., 0x7..., 0xa..., 0xb... are correlated.
If you think so, bug 736436 and bug 736666 are a dupe of this bug.

Comment 7

[Ali Juma [:ajuma]]

libGLESv2_POWERVR_SGX540_120.so@0xebc6 is glDrawArrays, so this does indeed seem to be a dupe of Bug 736666 and Bug 737957.

Comment 8

[Scoobidiver (away)]

(In reply to Ali Juma [:ajuma] from comment #7)
> libGLESv2_POWERVR_SGX540_120.so@0xebc6 is glDrawArrays
How do you know this so that we don't group unrelated signatures in a single bug or file duplicated bugs?

Comment 9

[Ali Juma [:ajuma]]

(In reply to Scoobidiver from comment #8)
> (In reply to Ali Juma [:ajuma] from comment #7)
> > libGLESv2_POWERVR_SGX540_120.so@0xebc6 is glDrawArrays
> How do you know this so that we don't group unrelated signatures in a single
> bug or file duplicated bugs?

I have a local copy of drivers from some devices, and I'm using addr2line to map addresses to function names.

Comment 10

[Martijn Wargers (dead)]

I get a similar stacktrace with this, I filed bug 736666 for it.

Comment 11

[Joe Drew (not getting mail)]

This is in glClear, apparently.

joe@ubuntu:~/moz-gdb/lib/0149A4550C01701E/system/lib$ ~/android-ndk-r5c/toolchains/arm-linux-androideabi-4.4.3/prebuilt/linux-x86/bin/arm-linux-androideabi-addr2line -C -f -e libGLESv2_POWERVR_SGX540_120.so 0xb8ae
glClear
??:0

Comment 12

[Joe Drew (not getting mail)]

Ah, it's probably at some random address that happens to be near glClear - we have no such calls in our codebase. However, up on the callstack there's glDrawElements, which seems much more likely. However, we still don't have any calls to glDrawElements in our code. Maybe it's being called by general Android code?

STR desperately needed.

Comment 13

[Joe Drew (not getting mail)]

In an effort to reduce the number of moving parts (and event queues to manage), I've tried removing GLThread, instead running all the GLController methods directly from within FlexibleGLSurfaceView. This has lead to a crash, unfortunately.

The compositor thread, when spinning up, asks the Java code through JNI for an EGLSurface (GLController::provideEGLSurface). From within this, eglCreateWindowSurface is throwing an error, with the following in adb lolcat:

E/GeckoGLController( 2190): provideEGLSurface
E/SurfaceTexture(  117): [SurfaceView] connect: already connected (cur=1, req=1)
E/libEGL  ( 2190): EGLNativeWindowType 0x1bc658 already connected to another API
E/libEGL  ( 2190): eglCreateWindowSurface:374 error 300b (EGL_BAD_NATIVE_WINDOW)

It seems that eglCreateWindowSurface doesn't want to create a surface for a native window that is already bound to a surface. However, I've verified that the call to provideEGLSurface happens after GLController::disposeGLContext is called, so I'm not sure what else needs to happen for the native window to be disconnected.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Does the fix I landed for bug 738188 fix this?

Comment 15

[Scoobidiver (away)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> Does the fix I landed for bug 738188 fix this?
There have been no crashes with this signature since 14.0a1/20120323031214, long before the patch in bug 738188 landed. So either it's fixed by another bug or Galaxy Nexus users gave up because of those startup crashes.
That may be also under other close libGLESv2_POWERVR_SGX540_120.so signatures for devices different from Galaxy Nexus.

Comment 16

[Joe Drew (not getting mail)]

Attachment: initial stab at this

This is an initial stab at this. It removes GLThread and stops initializing OpenGL from Java before Gecko starts up.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 610701 [details] [diff] [review]
initial stab at this

Provided this doesn't regress much, I like it.

Comment 18

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

Comment on attachment 610701 [details] [diff] [review]
initial stab at this

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

Hm, looks ok to me but I have no confidence in my ability to review this code. pcwalton would be a better reviewer, since he wrote the FlexibleGLSurfaceView.

Comment 19

[Ali Juma [:ajuma]]

Comment on attachment 610701 [details] [diff] [review]
initial stab at this

This makes our GL context handling much saner.

Comment 20

[Patrick Walton (:pcwalton)]

The reason that FlexibleGLSurfaceView works this way is so that it can render a pannable/zoomable screenshot maintained entirely in Java before Gecko comes up. This was a product directive from the beginning. If Gecko itself is responsible for bringing up GL in all cases, then this cannot be done.

I'm ok with removing that possibility personally (I don't think it helps much at this point), but I can't unilaterally approve it.

Comment 21

[Joe Drew (not getting mail)]

OK. Let me deal with product; if you can do feedback on the patch, or possibly an initial review, I'd appreciate it.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

We do have Fennec drivers and Product approval for removing the code. We are communicating the decision outward. An initial feedback or review on the patch would be great.

Comment 23

[Patrick Walton (:pcwalton)]

Comment on attachment 610701 [details] [diff] [review]
initial stab at this

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

f=me then.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

This also seems to fix the drawing of scrollbars and backgrounds.

Comment 25

[Chris Lord [:cwiiis]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #24)
> This also seems to fix the drawing of scrollbars and backgrounds.

This really shouldn't affect that I don't think... Are you sure about this? The scrollbars/backgrounds disappearing is intermittent - I sometimes see them, sometimes I don't, and it can depend on the page, scroll position, etc.

Comment 26

[Jeff Muizelaar [:jrmuizel]]

(In reply to Chris Lord [:cwiiis] from comment #25)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #24)
> > This also seems to fix the drawing of scrollbars and backgrounds.
> 
> This really shouldn't affect that I don't think... Are you sure about this?
> The scrollbars/backgrounds disappearing is intermittent - I sometimes see
> them, sometimes I don't, and it can depend on the page, scroll position, etc.

I am not sure.

Comment 27

[Joe Drew (not getting mail)]

Attachment: v1

Almost the same as the previous version of this patch, but with more unused code removed.

Comment 28

[Patrick Walton (:pcwalton)]

Comment on attachment 610935 [details] [diff] [review]
v1

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

r=me

Comment 29

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 610935
	Branch: mozilla-central => try
Patch 610935 could not be applied to mozilla-central.
patching file mobile/android/base/Makefile.in
Hunk #1 FAILED at 118
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/Makefile.in.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patch 610935 could not be applied to mozilla-central.
patching file mobile/android/base/Makefile.in
Hunk #1 FAILED at 118
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/Makefile.in.rej
patching file mobile/android/base/gfx/FlexibleGLSurfaceView.java
Hunk #1 FAILED at 39
Hunk #2 FAILED at 56
Hunk #3 FAILED at 91
3 out of 3 hunks FAILED -- saving rejects to file mobile/android/base/gfx/FlexibleGLSurfaceView.java.rej
patching file mobile/android/base/gfx/GLController.java
Hunk #1 FAILED at 49
Hunk #2 FAILED at 74
Hunk #3 FAILED at 146
Hunk #4 FAILED at 236
4 out of 4 hunks FAILED -- saving rejects to file mobile/android/base/gfx/GLController.java.rej
unable to find 'mobile/android/base/gfx/GLThread.java' for patching
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/gfx/GLThread.java.rej
patching file mobile/android/base/gfx/LayerView.java
Hunk #1 FAILED at 97
1 out of 1 hunks FAILED -- saving rejects to file mobile/android/base/gfx/LayerView.java.rej
patching file widget/android/AndroidBridge.cpp
Hunk #1 FAILED at 1104
1 out of 1 hunks FAILED -- saving rejects to file widget/android/AndroidBridge.cpp.rej
patching file widget/android/AndroidFlexViewWrapper.cpp
Hunk #1 FAILED at 45
Hunk #2 FAILED at 81
2 out of 2 hunks FAILED -- saving rejects to file widget/android/AndroidFlexViewWrapper.cpp.rej
patching file widget/android/AndroidFlexViewWrapper.h
Hunk #1 FAILED at 52
1 out of 1 hunks FAILED -- saving rejects to file widget/android/AndroidFlexViewWrapper.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Comment 30

[Joe Drew (not getting mail)]

Rebased and repushed to try: https://tbpl.mozilla.org/?tree=Try&rev=320256571f7a

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 610935 [details] [diff] [review]
v1

This makes it so the code from GLThread.renderFrame() is called unconditionally, as though mGLThread != null. We don't want to call it at all.

Comment 32

[Joe Drew (not getting mail)]

Attachment: v2

Jeff pointed out that I had moved the contents of GLThread.requestRender into FlexibleGLSurfaceView.requestRender, but it was normally used only before the C++ compositor started up (and mGLThread was nulled out). The refactored code would, however, always be run, regardless of whether the C++ compositor was started up.

Since we've made the decision to stop using OpenGL from Java on startup, we don't need the GLThread.requestRender code at all, so I've removed it.

This is also rebased on current m-c trunk.

Carrying forward r=pcwalton, and adding r=jrmuizel.

Comment 33

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84463b2ba5ff

Comment 34

[Mozilla RelEng Bot]

Try run for 320256571f7a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=320256571f7a
Results (out of 237 total builds):
    exception: 1
    success: 214
    warnings: 22
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-320256571f7a

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/84463b2ba5ff