Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 713803 - JNI() verses mJNIEnv in widget/src/android/*
: JNI() verses mJNIEnv in widget/src/android/*
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Doug Turner (:dougt)
: Jim Chen [:jchen] [:darchons]
Depends on:
Blocks: 722123
  Show dependency treegraph
Reported: 2011-12-27 20:47 PST by Doug Turner (:dougt)
Modified: 2012-02-09 15:52 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v.1 (71.66 KB, patch)
2012-01-27 19:44 PST, Doug Turner (:dougt)
no flags Details | Diff | Splinter Review
patch v.2 (85.47 KB, patch)
2012-01-28 16:59 PST, Doug Turner (:dougt)
blassey.bugs: review+
blassey.bugs: approval‑mozilla‑aurora+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2011-12-27 20:47:59 PST
Not sure both are right.... 

JNI() verses mJNIEnv in widget/src/android/*
Comment 1 Doug Turner (:dougt) 2012-01-02 10:07:14 PST
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 Doug Turner (:dougt) 2012-01-12 14:01:55 PST
i am not working on these right now.  resetting assignee.
Comment 3 Doug Turner (:dougt) 2012-01-27 19:44:45 PST
Created attachment 592356 [details] [diff] [review]
patch v.1
Comment 4 Doug Turner (:dougt) 2012-01-28 16:59:18 PST
Created attachment 592453 [details] [diff] [review]
patch v.2
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-01-28 17:50:11 PST
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.
Comment 6 Doug Turner (:dougt) 2012-01-28 19:14:51 PST
> 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.
Comment 7 Doug Turner (:dougt) 2012-01-28 22:41:06 PST

followup 722123
Comment 8 Phil Ringnalda (:philor) 2012-01-29 00:54:51 PST
Backed out in

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 Doug Turner (:dougt) 2012-01-29 10:15:06 PST
awesome.  it passed try.
Comment 10 Doug Turner (:dougt) 2012-01-29 10:16:50 PST
and I think I know what this is..
Comment 11 Phil Ringnalda (:philor) 2012-01-29 10:25:17 PST
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 Doug Turner (:dougt) 2012-01-29 12:41:19 PST
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.
Comment 13 Marco Bonardo [::mak] 2012-01-30 02:50:10 PST
Comment 14 Brad Lassey [:blassey] (use needinfo?) 2012-01-31 11:44:27 PST
Comment on attachment 592453 [details] [diff] [review]
patch v.2

[Triage Comment]

Note You need to log in before you can comment on or make changes to this bug.