1032460 - Fix JNI pointers to use longs

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

bug 1030899 comment 18

06-30 21:19:25.961  7532  7582 W System.err: java.lang.NoSuchFieldError: no "I" field "mEGLSurface" in class "Lcom/google/android/gles_jni/EGLSurfaceImpl;" or its superclasses
06-30 21:19:25.962  7532  7582 W System.err: 	at org.mozilla.gecko.GeckoAppShell.nativeInit(Native Method)
06-30 21:19:25.962  7532  7582 W System.err: 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:320)
06-30 21:19:25.962  7532  7582 W System.err: 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:174)

which is something you'll probably want to deal with on a separate bug. (any private fields you're pulling out of Android classes where the field corresponds to a pointer will be a long in L, on both LP32 and LP64. you'll need to fix your JNI.)

Comment 1

[enh]

for the record, i tested http://ftp.mozilla.org/pub/mozilla.org/mobile/releases/latest-beta/android/en-US/fennec-31.0b2.en-US.android-arm.apk

Comment 2

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

Attachment: Fix getting JNI EGLSurface for Android L

Comment 3

[Sylvestre Ledru [:Sylvestre]]

James, we will take it for aurora and beta. Please fill an uplift request when ready. Thanks.

Comment 4

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/0404029fb378

Comment 5

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

Let's test a nightly build on L after this lands to make sure it's right.

Comment 6

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

(And then we can uplift, I mean)

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Sure. FYI, we should release beta 6 today. It could be part of beta 7 (gtb next Thursday) or beta 8 next Monday.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

Android's next beta is 8.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/0404029fb378

Comment 10

[Sylvestre Ledru [:Sylvestre]]

James, could you request the uplift for aurora? Thanks

Comment 11

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

Comment on attachment 8448305 [details] [diff] [review]
Fix getting JNI EGLSurface for Android L

Approval Request Comment
[Feature/regressing bug #]: Android L release
[User impact if declined]: Crash on Android L
[Describe test coverage new/current, TBPL]: new testing on Android L so far
[Risks and why]: reasons
[String/UUID change made/needed]: none

Comment 12

[enh]

is there a nightly build that contains this patch yet? i'll test it for you if so.

Comment 13

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

(In reply to enh from comment #12)
> is there a nightly build that contains this patch yet? i'll test it for you
> if so.

Yeah, it's in the latest Nightly. We haven't audited for any other instances of this type of thing, though.

Comment 14

[enh]

are neither of these the latest nightly?

http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2014-07-11-00-40-06-mozilla-aurora-android/en-US/fennec-32.0a2.en-US.android-arm.apk

http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2014-07-11-03-02-01-mozilla-central-android/fennec-33.0a1.multi.android-arm.apk

both of those crash in the same way in the same place:

07-11 17:11:55.177  9091  9156 W System.err: java.lang.NoSuchFieldError: no "I" field "mEGLSurface" in class "Lcom/google/android/gles_jni/EGLSurfaceImpl;" or its superclasses
07-11 17:11:55.177  9091  9156 W System.err: 	at org.mozilla.gecko.GeckoAppShell.nativeInit(Native Method)
07-11 17:11:55.178  9091  9156 W System.err: 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:320)
07-11 17:11:55.178  9091  9156 W System.err: 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:176)

oh... i'm testing on AOSP where the API level won't be high enough for your code to fire.

on an internal build i still see a crash, but it's a new one (and an obscure one). it's not clear what the actual problem is, but i see this:

E/GeckoLibLoad(24431): Load sqlite start
W/GeckoLinker(24431): /data/app/org.mozilla.fennec-1/base.apk!/assets/libnss3.so: unhandled flags #8 not handled
W/GeckoLinker(24431): /data/app/org.mozilla.fennec-1/base.apk!/assets/libnss3.so: Relocation to NULL @0x00147c64
W/GeckoLinker(24431): /data/app/org.mozilla.fennec-1/base.apk!/assets/libnss3.so: Relocation to NULL @0x00147fdc for symbol "__cxa_begin_cleanup"
W/GeckoLinker(24431): /data/app/org.mozilla.fennec-1/base.apk!/assets/libnss3.so: Relocation to NULL @0x00147fe0 for symbol "__cxa_type_match"
E/GeckoLibLoad(24431): Load sqlite done
E/GeckoLibLoad(24431): Load nss start
E/GeckoLibLoad(24431): Load nss done
W/GeckoLinker(24431): /data/app/org.mozilla.fennec-1/base.apk!/assets/libxul.so: unhandled flags #8 not handled
W/GeckoLinker(24431): /data/app/org.mozilla.fennec-1/base.apk!/assets/libmozalloc.so: unhandled flags #8 not handled
W/GeckoLinker(24431): /data/app/org.mozilla.fennec-1/base.apk!/assets/libmozalloc.so: Relocation to NULL @0x00003f68
W/GeckoLinker(24431): /data/app/org.mozilla.fennec-1/base.apk!/assets/libmozalloc.so: Relocation to NULL @0x00003fdc for symbol "__cxa_begin_cleanup"
W/GeckoLinker(24431): /data/app/org.mozilla.fennec-1/base.apk!/assets/libmozalloc.so: Relocation to NULL @0x00003fe0 for symbol "__cxa_type_match"

and this:

W/art     (24431): Attempt to remove local handle scope entry from IRT, ignoring

before the crash, about which all i see is:

I/Zygote  (  205): Process 24431 exited due to signal (11)

is there a system property or something i can set so you guys don't set your own signal handler? (which i assume is why i don't see anything from debuggerd.)

Comment 15

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

(In reply to enh from comment #14)
> 
> is there a system property or something i can set so you guys don't set your
> own signal handler? (which i assume is why i don't see anything from
> debuggerd.)

This could be related to the ondemand decompression in the linker, which uses a signal handler (for SEGV) to determine when to do more work. You should be able to disable that by passing "--es env0 MOZ_LINKER_ONDEMAND=0" to the 'am start' command. Something like this:

adb shell am start org.mozilla.fennec/.App --es env0 MOZ_LINKER_ONDEMAND=0

We do have our own crash reporting code that also attaches to SEGV. From your log there I wouldn't imagine that would be set up yet, though.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Brian Carlstrom]

There still is an issue related to this change. When run with CheckJNI the runtime reports:

JNI DETECTED ERROR IN APPLICATION: attempt to access field long com.google.android.gles_jni.EGLSurfaceImpl.mEGLSurface of type long with the wrong type int

this is because even though the code looks up the field based on long vs int:

 +        const char* jniType = mAPIVersion >= 20 ? "J" : "I";
 +        jEGLSurfacePointerField = getField("mEGLSurface", jniType);

It still access it using GetIntField unconditionally vs GetLongField when mAPIVersion >= 20

https://dxr.mozilla.org/mozilla-central/source/widget/android/AndroidBridge.cpp#744
            env->GetIntField(eglSurface.Get(), jEGLSurfacePointerField));

Comment 18

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

This bug already has a fix, so let's clone it for the other fixes we need.

Comment 19

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

nm, we already have bug 1172567