Last Comment Bug 713803 - JNI() verses mJNIEnv in widget/src/android/*
: JNI() verses mJNIEnv in widget/src/android/*
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Doug Turner (:dougt)
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
patch v.1 (71.66 KB, patch)
2012-01-27 19:44 PST, Doug Turner (:dougt)
no flags Details | Diff | 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 | 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():

http://mxr.mozilla.org/mozilla-central/source/widget/src/android/AndroidBridge.h#107

and that will call AttachCurrentThread():

http://mxr.mozilla.org/mozilla-central/source/widget/src/android/AndroidBridge.cpp#247


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
https://hg.mozilla.org/integration/mozilla-inbound/rev/59d8d92f9f3c

followup 722123
Comment 8 Phil Ringnalda (:philor) 2012-01-29 00:54:51 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/87e6229b4073

The logs don't seem terribly useful, but

https://tbpl.mozilla.org/php/getParsedLog.php?id=8917273&tree=Mozilla-Inbound&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=8917350&tree=Mozilla-Inbound&full=1

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:

https://tbpl.mozilla.org/?tree=Try&rev=67d36d2da557

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f0c0a016330
Comment 13 Marco Bonardo [::mak] 2012-01-30 02:50:10 PST
https://hg.mozilla.org/mozilla-central/rev/1f0c0a016330
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.