Note: There are a few cases of duplicates in user autocompletion which are being worked on.

AutoLocalJNIFrame should use JNIForThread()

RESOLVED FIXED in mozilla11

Status

()

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

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 577014 [details] [diff] [review]
Patch v1

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

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All

Comment 1

6 years ago
Who is using JNI on another thread?
(Assignee)

Comment 2

6 years ago
(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.
related to bug 704785?
(Assignee)

Comment 4

6 years ago
(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 5

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

Comment 6

6 years ago
Created attachment 577612 [details] [diff] [review]
Patch v2
Attachment #577014 - Attachment is obsolete: true
Attachment #577612 - Flags: review?(mwu)
(Assignee)

Comment 7

6 years ago
Created attachment 577621 [details] [diff] [review]
Patch v3
Attachment #577612 - Attachment is obsolete: true
Attachment #577612 - Flags: review?(mwu)
Attachment #577621 - Flags: review?(mwu)

Comment 8

6 years ago
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+
(Assignee)

Updated

6 years ago
Flags: in-testsuite-
Whiteboard: [needs review]
Target Milestone: --- → mozilla11
(Assignee)

Updated

6 years ago
Attachment #577621 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/3a5731a60002
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.