Closed Bug 1032460 Opened 6 years ago Closed 6 years ago

Fix JNI pointers to use longs

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33
Tracking Status
firefox31 + wontfix
firefox32 + fixed
firefox33 + fixed
fennec 32+ ---

People

(Reporter: kbrosnan, Assigned: snorp)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.)
Assignee: nobody → snorp
Attachment #8448305 - Flags: review?(blassey.bugs)
Attachment #8448305 - Flags: review?(blassey.bugs) → review+
James, we will take it for aurora and beta. Please fill an uplift request when ready. Thanks.
Flags: needinfo?(snorp)
Let's test a nightly build on L after this lands to make sure it's right.
(And then we can uplift, I mean)
Sure. FYI, we should release beta 6 today. It could be part of beta 7 (gtb next Thursday) or beta 8 next Monday.
Android's next beta is 8.
https://hg.mozilla.org/mozilla-central/rev/0404029fb378
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
tracking-fennec: ? → 32+
James, could you request the uplift for aurora? Thanks
Flags: needinfo?(snorp)
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
Attachment #8448305 - Flags: approval-mozilla-aurora?
Flags: needinfo?(snorp)
Attachment #8448305 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
is there a nightly build that contains this patch yet? i'll test it for you if so.
(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.
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.)
(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.
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));
This bug already has a fix, so let's clone it for the other fixes we need.
You need to log in before you can comment on or make changes to this bug.