Closed Bug 713803 Opened 13 years ago Closed 13 years ago

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


(Core Graveyard :: Widget: Android, defect)

Not set


(firefox11 fixed, firefox12 fixed)

Tracking Status
firefox11 --- fixed
firefox12 --- fixed


(Reporter: dougt, Assigned: dougt)



(Whiteboard: [qa-])


(1 file, 1 obsolete file)

Not sure both are right.... 

JNI() verses mJNIEnv in widget/src/android/*
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.
i am not working on these right now.  resetting assignee.
Assignee: doug.turner → nobody
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #592356 - Flags: review?(blassey.bugs)
Attached patch patch v.2Splinter Review
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+
> 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.
Blocks: 722123
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.
awesome.  it passed try.
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" ;)
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.
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
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-]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.