Closed Bug 705386 Opened 8 years ago Closed 8 years ago

AutoLocalJNIFrame should use JNIForThread()

Categories

(Core :: Widget: Android, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: mounir, Assigned: mounir)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
Using JNI() forbids us to use AutoLocalJNIFrame if the method isn't called in the main thread. We should use JNIForThread() instead to allow such usage.
Attachment #577014 - Flags: review?(mwu)
OS: Linux → All
Hardware: x86_64 → All
Who is using JNI on another thread?
(In reply to Michael Wu [:mwu] from comment #1)
> Who is using JNI on another thread?

All callers of GetJNIForThread() could use JNI on another thread (which I realize is mostly WebSMS methods). However, I wrote a method that is called from another thread which means I can't call it with the current implementation of AutoLocalJNIFrame.
(In reply to Naoki Hirata :nhirata from comment #3)
> related to bug 704785?

The stack track lets me think the crash in bug 704785 happens in the main thread. In that case, it wouldn't be related.
Comment on attachment 577014 [details] [diff] [review]
Patch v1

I don't want AutoLocalJNIFrame (with JNIForThread) to be used by other threads unless the code explicitly asks for it. In most cases, getting a JNIEnv on a different thread isn't a good idea unless you really need it and have made sure that it is safe.
Attachment #577014 - Flags: review?(mwu) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #577014 - Attachment is obsolete: true
Attachment #577612 - Flags: review?(mwu)
Attached patch Patch v3Splinter Review
Attachment #577612 - Attachment is obsolete: true
Attachment #577612 - Flags: review?(mwu)
Attachment #577621 - Flags: review?(mwu)
Comment on attachment 577621 [details] [diff] [review]
Patch v3

Review of attachment 577621 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: widget/src/android/AndroidBridge.h
@@ +253,5 @@
>          // the AutoLocalJNIFrame's scope INVALID; be sure that you locked down
>          // any local refs that you need to keep around in global refs!
>          void Purge() {
> +            mJNIEnv->PopLocalFrame(NULL);
> +            mJNIEnv->PushLocalFrame(mEntries);

You can probably use Push() here too.
Attachment #577621 - Flags: review?(mwu) → review+
Flags: in-testsuite-
Whiteboard: [needs review]
Target Milestone: --- → mozilla11
Attachment #577621 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/3a5731a60002
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.