JNI() verses mJNIEnv in widget/src/android/*

RESOLVED FIXED in Firefox 11



7 years ago
7 years ago


(Reporter: dougt, Assigned: dougt)


Mac OS X

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed)


(Whiteboard: [qa-])


(1 attachment, 1 obsolete attachment)



7 years ago
Not sure both are right.... 

JNI() verses mJNIEnv in widget/src/android/*

Comment 1

7 years ago
JNI() looks like it calls EnsureJNIThread():


and that will call AttachCurrentThread():


if someone uses mJNIEnv on a thread that hasn't called AttachCurrentThread(), it will crash.  But, that is probably not happening here.

This bug, I think, is just about code consistency.

Comment 2

7 years ago
i am not working on these right now.  resetting assignee.
Assignee: doug.turner → nobody

Comment 3

7 years ago
Created attachment 592356 [details] [diff] [review]
patch v.1
Attachment #592356 - Flags: review?(blassey.bugs)

Comment 4

7 years ago
Created attachment 592453 [details] [diff] [review]
patch v.2
Assignee: nobody → doug.turner
Attachment #592356 - Attachment is obsolete: true
Attachment #592356 - Flags: review?(blassey.bugs)
Attachment #592453 - Flags: review?(blassey.bugs)
Comment on attachment 592453 [details] [diff] [review]
patch v.2

Review of attachment 592453 [details] [diff] [review]:

::: dom/plugins/base/android/ANPEvent.cpp
@@ -66,5 @@
>  anp_event_postEvent(NPP inst, const ANPEvent* event)
>  {
>    LOG("%s", __PRETTY_FUNCTION__);
> -  if (!mozilla::AndroidBridge::Bridge()) {
> -    LOG("no bridge in %s!!!!", __PRETTY_FUNCTION__);

still need to null check the bridge

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +752,5 @@
>    void RequestSurface() {
> +    JNIEnv* env = GetJNIForThread();
> +    if (!env)
> +      return;
> +    mozilla::AndroidBridge::Bridge()->PostToJavaThread(env, this);

should null check the bridge as well

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1682,5 @@
>    JNIEnv* env = GetJNIForThread();
> +  if (!env)
> +    return false;
> +
> +  AndroidBridge::AutoLocalJNIFrame frame(env, 1);

you're already bit-rotted. mbrubeck landed a patch with this change already

@@ +1748,5 @@
> +  JNIEnv* env = GetJNIForThread();
> +  if (!env)
> +    return;
> +
> +  AndroidBridge::AutoLocalJNIFrame frame(env, 1);

again, mbrubeck just did this

::: widget/android/AndroidBridge.cpp
@@ +184,5 @@
>      return true;
>  }
>  JNIEnv *
> +AndroidBridge::GetJNI()

GetJNIEnv() or even better just JNIEnv()

Also, having static JNI() and non-static GetJNI() is confusing. I'm thinking we should rename JNI() to GetJNIEnvStatic() and GetJNI() to JNIEnv().

@@ +189,4 @@
>  {
> +    if ((void*)pthread_self() != mThread) {
> +        ALOG_BRIDGE("###!!!!!!! Something's grabbing the JNIEnv from the wrong thread! (thr %p should be %p)",
> +                    (void*)pthread_self(), (void*)mThread);

we should return null here, right?

@@ +222,5 @@
>  AndroidBridge::NotifyIME(int aType, int aState)
>  {
>      ALOG_BRIDGE("AndroidBridge::NotifyIME");
> +
> +    JNIEnv *env = AndroidBridge::JNI();

so what's the difference between JNI() and GetJNI()?

@@ +1156,4 @@
>  }
>  void
> +AndroidBridge::PostToJavaThread(JNIEnv *env, nsIRunnable* aRunnable, bool aMainThread)

I'm still of the opinion that if this is called on a non-main gecko thread we should post an event to the main gecko thread. Please file a follow up to do that.
Attachment #592453 - Flags: review?(blassey.bugs) → review+

Comment 6

7 years ago
> so what's the difference between JNI() and GetJNI()?

On naming...

JNI() is a static getter on the AndroidBridge.  It is consistent with the other static getters.

static AndroidBridge *Bridge()
static JavaVM *VM()
static jclass GetGeckoAppShellClass()

So, I am going to JNI() -> JNIEnv().

I am going to remove GetJNI() and just have the callers in widget/ use just the newly-named JNIEnv() static.

> I'm still of the opinion that if this is called on a non-main gecko thread we should post an event to the main gecko thread. Please file a follow up to do that.

This is a good goal, but audio tracks hit JNI to access android audio APIs on multiple threads.  Pushing these accesses through our event system sounds like a bad idea (pun, yes!).  But others should do this!  I'll file the followup.


7 years ago
Blocks: 722123
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/87e6229b4073

The logs don't seem terribly useful, but


three times each on native and XUL, "application crashed (minidump found)" though it can't be bothered to do anything with the minidump.

Comment 9

7 years ago
awesome.  it passed try.

Comment 10

7 years ago
and I think I know what this is..
I think you mean "failed in exactly the same way on try, but because C1 has such a high failure rate, that part of my eyes is burned out and I can't even see two orange C1s" ;)

Comment 12

7 years ago
very small fix.  Pushed to try and works now:


we were not detaching threads.  really, we shouldn't have to do this.  the tegra roms are justing configured to abort if this happens.

Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12


7 years ago
Attachment #592453 - Flags: approval-mozilla-aurora?
Comment on attachment 592453 [details] [diff] [review]
patch v.2

[Triage Comment]
Attachment #592453 - Flags: approval-mozilla-beta+
Attachment #592453 - Flags: approval-mozilla-aurora?
Attachment #592453 - Flags: approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.