748531 - Catch exceptions in JNI code

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta [:kats]]

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

Comment 1

[Kartikaya Gupta [:kats]]

Attachment: (1/3) Cleanup

No functional changes in this one, just fixing indentation and removing unnecessary "AndroidBridge::"

Comment 2

[Kartikaya Gupta [:kats]]

Attachment: (2/3) Propagate JNIEnv better

Remove a lot of expensive calls to GetJNIForThread in DrawWindowUnderlay and DrawWindowOverlay.

Comment 3

[Kartikaya Gupta [:kats]]

Attachment: (3/3) Guard against exceptions thrown in Java

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).

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

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;

Comment 6

[Kartikaya Gupta [:kats]]

Attachment: (1/3) Cleanup (v2)

Rebased on tip; carrying r+

Comment 7

[Kartikaya Gupta [:kats]]

Attachment: (2/3) Propagate JNIEnv better (v2)

Ditto

Comment 8

[Kartikaya Gupta [:kats]]

Attachment: (3/3) Guard against exceptions thrown in Java (v2)

Comment 9

[Kartikaya Gupta [:kats]]

Attachment: (3/3) Guard against exceptions thrown in Java (v2)

Whoops, forgot to qref a couple of compile fixes.

Comment 10

[Kartikaya Gupta [:kats]]

Try build is showing bustage on XUL but I don't know why :(

https://tbpl.mozilla.org/?tree=Try&rev=e3b5bd1568c9

Comment 11

[Kartikaya Gupta [:kats]]

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

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 13

[Kartikaya Gupta [:kats]]

Attachment: (3/3) Guard against exceptions thrown in Java (v3)

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).

Comment 14

[Kartikaya Gupta [:kats]]

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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9b437df4d25a
https://hg.mozilla.org/mozilla-central/rev/4700ff6f9cfe
https://hg.mozilla.org/mozilla-central/rev/1346fb90ab8f

Comment 16

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

QAwanted to verify not just this bug but all related bugs to this bug.  (dups and dependencies)

Comment 17

[Kartikaya Gupta [:kats]]

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 18

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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:

Comment 19

[Kartikaya Gupta [:kats]]

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.

Comment 20

[Kartikaya Gupta [:kats]]

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

Comment 21

[Kartikaya Gupta [:kats]]

Comment on attachment 619568 [details] [diff] [review]
(2/3) Propagate JNIEnv better (v2)

See ^

Comment 22

[Kartikaya Gupta [:kats]]

Comment on attachment 620853 [details] [diff] [review]
(3/3) Guard against exceptions thrown in Java (v3)

See ^

Comment 23

[Alex Keybl [:akeybl]]

We are leaving all non-beta+ bugs nominated for Aurora approval in the queue until FN14 Beta 1 is signed off on by QA.

Comment 25

[Kartikaya Gupta [:kats]]

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

Comment 27

[Xiao Meng Wei :xwei]

QAwanted since we need more verifications on the bug, also to see if it resurfaces in future builds.

Comment 28

[Aaron Train [:aaronmt]]

Verified Fixed:

Nightly (05/29), Aurora (05/29)
Galaxy Nexus (Android 4.0.4)