The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 11

Status

()

Core
Widget: Android
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougt, Assigned: dougt)

Tracking

Trunk
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

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

Comment 1

5 years ago
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.
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

5 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+
(Assignee)

Comment 6

5 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.
(Assignee)

Updated

5 years ago
Blocks: 722123
(Assignee)

Comment 7

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

followup 722123
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.
(Assignee)

Comment 9

5 years ago
awesome.  it passed try.
(Assignee)

Comment 10

5 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" ;)
(Assignee)

Comment 12

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/1f0c0a016330
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
(Assignee)

Updated

5 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+
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/127c54cbf58d
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7a18ca15f55
status-firefox11: --- → fixed
status-firefox12: --- → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.