Closed
Bug 748531
Opened 12 years ago
Closed 12 years ago
Catch exceptions in JNI code
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox14 verified, firefox15 verified, blocking-fennec1.0 betaN+, fennec15+)
VERIFIED
FIXED
Firefox 15
People
(Reporter: kats, Assigned: kats)
References
Details
Crash Data
Attachments
(3 files, 5 obsolete files)
10.72 KB,
patch
|
kats
:
review+
mfinkle
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
8.69 KB,
patch
|
kats
:
review+
mfinkle
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
69.32 KB,
patch
|
blassey
:
review+
mfinkle
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
All those Call*Method functions we're calling from AndroidBridge and AndroidJavaWrappers could be throwing exceptions. We should be calling ExceptionCheck/ExceptionDescribe/ExceptionClear or some such after all such calls as described at http://developer.android.com/guide/practices/design/jni.html#exceptions
Updated•12 years ago
|
tracking-fennec: --- → 15+
blocking-fennec1.0: ? → -
Updated•12 years ago
|
blocking-fennec1.0: - → soft
Assignee | ||
Comment 1•12 years ago
|
||
No functional changes in this one, just fixing indentation and removing unnecessary "AndroidBridge::"
Attachment #619167 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 2•12 years ago
|
||
Remove a lot of expensive calls to GetJNIForThread in DrawWindowUnderlay and DrawWindowOverlay.
Attachment #619168 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 3•12 years ago
|
||
This adds some macros to try to eliminate lots of duplicated boilerplate, and also guard against exceptions thrown from JNI calls (I searched for all calls using "->Call" and made sure they were either the last thing in the scope of an AutoLocalJNIFrame or had a JNIExceptionCheck after them (or both, in some cases).
Attachment #619174 -
Flags: review?(blassey.bugs)
Comment 4•12 years ago
|
||
Comment on attachment 619167 [details] [diff] [review] (1/3) Cleanup Review of attachment 619167 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/AndroidBridge.cpp @@ +2007,4 @@ > > + jobject surface = env->CallStaticObjectMethod(cls, method); > + if (surface) > + env->NewGlobalRef(surface); this should be: surface = env->NewGlobalRef(surface); but snorp just landed a patch to make that change. Also, most of the plugin related parts of this patch are already bit rotted.
Attachment #619167 -
Flags: review?(blassey.bugs) → review+
Updated•12 years ago
|
Attachment #619168 -
Flags: review?(blassey.bugs) → review+
Comment 5•12 years ago
|
||
Comment on attachment 619174 [details] [diff] [review] (3/3) Guard against exceptions thrown in Java Review of attachment 619174 [details] [diff] [review]: ----------------------------------------------------------------- I don't like the GET_JNIENV_OR_RETURN macro. In general I'm not a fan of hiding variable declarations in macros. Also, there is a general tide at mozilla against returns being in macros, though I'm less concerned with that. snorp just added this https://hg.mozilla.org/integration/mozilla-inbound/diff/72e069288a7e/widget/android/AndroidBridge.h#l1.31, let's use it. So where you now have ON_EXCEPTION_RETURN(stuff), you can replace it with if (frame->CheckForException()) return;
Attachment #619174 -
Flags: review?(blassey.bugs) → review-
Assignee | ||
Comment 6•12 years ago
|
||
Rebased on tip; carrying r+
Attachment #619167 -
Attachment is obsolete: true
Attachment #619567 -
Flags: review+
Assignee | ||
Comment 7•12 years ago
|
||
Ditto
Attachment #619168 -
Attachment is obsolete: true
Attachment #619568 -
Flags: review+
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #619174 -
Attachment is obsolete: true
Attachment #619569 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 9•12 years ago
|
||
Whoops, forgot to qref a couple of compile fixes.
Attachment #619569 -
Attachment is obsolete: true
Attachment #619569 -
Flags: review?(blassey.bugs)
Attachment #619573 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 10•12 years ago
|
||
Try build is showing bustage on XUL but I don't know why :( https://tbpl.mozilla.org/?tree=Try&rev=e3b5bd1568c9
Assignee | ||
Comment 11•12 years ago
|
||
I downloaded the build from comment 10 and ran it on my device, it ran fine so I wasn't able to diagnose the problem. I pushed another build to try with more logging to try and figure out where/why it's dying. The new try push is at https://tbpl.mozilla.org/?tree=Try&rev=f9c29695b035
Comment 12•12 years ago
|
||
Comment on attachment 619573 [details] [diff] [review] (3/3) Guard against exceptions thrown in Java (v2) Review of attachment 619573 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/AndroidBridge.cpp @@ +260,5 @@ > > JNIEnv *env = GetJNIEnv(); > if (!env) > return; > + AutoLocalJNIFrame jniFrame(env, 1); I think this can be jniFrame(env, 0), since no local variables are created within the frame. Assuming that's right, change the all of them. @@ +271,5 @@ > AndroidBridge::NotifyIMEEnabled(int aState, const nsAString& aTypeHint, > const nsAString& aActionHint) > { > ALOG_BRIDGE("AndroidBridge::NotifyIMEEnabled"); > + remove extra whitespace @@ +317,5 @@ > AndroidBridge::NotifyIMEChange(const PRUnichar *aText, PRUint32 aTextLen, > int aStart, int aEnd, int aNewEnd) > { > ALOG_BRIDGE("AndroidBridge::NotifyIMEChange"); > + remove extra whitespace @@ -320,2 @@ > return; > - } no curly braces @@ +360,2 @@ > AutoLocalJNIFrame jniFrame(env, 1); > + remove extra whitespace @@ +365,5 @@ > void > AndroidBridge::EnableLocationHighAccuracy(bool aEnable) > { > ALOG_BRIDGE("AndroidBridge::EnableLocationHighAccuracy"); > + remove extra whitespace @@ +381,2 @@ > ALOG_BRIDGE("AndroidBridge::EnableSensor"); > + remove extra whitespace @@ +386,2 @@ > AutoLocalJNIFrame jniFrame(env, 1); > + remove extra whitespace... actually just remove all the white space changes @@ +872,4 @@ > static bool once = false; > > JNIEnv *env = GetJNIEnv(); > + if (!env || once) check once before calling GetJNIEnv()
Attachment #619573 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 13•12 years ago
|
||
New version. It reverts the whitespace back to the original haphazard state and addresses the other review comments. In addition, I had to make some changes to fix XUL bustage - I'm now passing in JNIEnv* and AutoLocalJNIFrame* to some AndroidGeckoSurfaceView functions (the ones that return a jobject) so that the ref they return is properly scoped. Finally, I also added a check on the return value for PushLocalFrame as snorp suggested. Re-requesting review based on above changes. Latest try build is at https://tbpl.mozilla.org/?tree=Try&rev=746941750f0f (I have an older one at https://tbpl.mozilla.org/?tree=Try&rev=fc0182305cb9 which is showing some green on XUL, so I assume my XUL woes are finally over).
Attachment #620853 -
Flags: review?(blassey.bugs)
Updated•12 years ago
|
Attachment #620853 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #619573 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
I had to merge when I rebased, so I pushed another try build to https://tbpl.mozilla.org/?tree=Try&rev=e97b066f5bb6 as well; it's starting to look green so I'm landing before I have to do another merge. https://hg.mozilla.org/integration/mozilla-inbound/rev/9b437df4d25a https://hg.mozilla.org/integration/mozilla-inbound/rev/4700ff6f9cfe https://hg.mozilla.org/integration/mozilla-inbound/rev/1346fb90ab8f
Comment 15•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9b437df4d25a https://hg.mozilla.org/mozilla-central/rev/4700ff6f9cfe https://hg.mozilla.org/mozilla-central/rev/1346fb90ab8f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
QAwanted to verify not just this bug but all related bugs to this bug. (dups and dependencies)
Keywords: qawanted
Assignee | ||
Comment 17•12 years ago
|
||
Just to be clear, what I would expect to see is that the crashes in the dependencies drop off in builds that have this fix. There will likely still be an out-of-memory crash left somewhere but all the signatures from that crash should be pretty much the same, and that should be trackable by a single bug rather than the ~15 bugs we have now for all the different signatures it manifests as.
Comment on attachment 619567 [details] [diff] [review] (1/3) Cleanup (v2) [Approval Request Comment] Regression caused by (bug #): User impact if declined: Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): String changes made by this patch:
Attachment #619567 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 19•12 years ago
|
||
Comment on attachment 619567 [details] [diff] [review] (1/3) Cleanup (v2) Canceling aurora request. This should bake a little longer first. I'll request approval for all three patches in a day or two.
Attachment #619567 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 20•12 years ago
|
||
Comment on attachment 619567 [details] [diff] [review] (1/3) Cleanup (v2) [Approval Request Comment] Regression caused by (bug #): various User impact if declined: possible JNI crashes Testing completed (on m-c, etc.): baked on m-c, doesn't seem to regress anything Risk to taking this patch (and alternatives if risky): mobile only, but the patch itself is probably medium risk given how much code it touches String changes made by this patch: none
Attachment #619567 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 21•12 years ago
|
||
Comment on attachment 619568 [details] [diff] [review] (2/3) Propagate JNIEnv better (v2) See ^
Attachment #619568 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 22•12 years ago
|
||
Comment on attachment 620853 [details] [diff] [review] (3/3) Guard against exceptions thrown in Java (v3) See ^
Attachment #620853 -
Flags: approval-mozilla-aurora?
Comment 23•12 years ago
|
||
We are leaving all non-beta+ bugs nominated for Aurora approval in the queue until FN14 Beta 1 is signed off on by QA.
Updated•12 years ago
|
blocking-fennec1.0: soft → betaN+
Updated•12 years ago
|
Attachment #619567 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #619568 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #620853 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1e06f6c5ed38 https://hg.mozilla.org/releases/mozilla-aurora/rev/853dce95eac2 https://hg.mozilla.org/releases/mozilla-aurora/rev/67abaf68e33f
Updated•12 years ago
|
Crash Signature: [@ mozilla::AndroidBridge::AutoLocalJNIFrame::~AutoLocalJNIFrame | mozilla::AndroidBridge::PumpMessageLoop ]
Comment 27•12 years ago
|
||
QAwanted since we need more verifications on the bug, also to see if it resurfaces in future builds.
Comment 28•12 years ago
|
||
Verified Fixed: Nightly (05/29), Aurora (05/29) Galaxy Nexus (Android 4.0.4)
Status: RESOLVED → VERIFIED
Keywords: qawanted
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•