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

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Doug Turner (:dougt)]

Not sure both are right.... 

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

Comment 1

[Doug Turner (:dougt)]

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)]

i am not working on these right now.  resetting assignee.

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.2

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

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)]

> 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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59d8d92f9f3c

followup 722123

Comment 8

[Phil Ringnalda (:philor)]

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)]

awesome.  it passed try.

Comment 10

[Doug Turner (:dougt)]

and I think I know what this is..

Comment 11

[Phil Ringnalda (:philor)]

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)]

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]]

https://hg.mozilla.org/mozilla-central/rev/1f0c0a016330

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 592453 [details] [diff] [review]
patch v.2

[Triage Comment]

Comment 15

[Doug Turner (:dougt)]

https://hg.mozilla.org/releases/mozilla-beta/rev/127c54cbf58d
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7a18ca15f55