1046880 - crash in java.lang.IllegalArgumentException: Property does not exist at org.mozilla.gecko.util.NativeJSObject.getString(Native Method)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-d83e94ae-79a8-4687-80c3-07caa2140730.
=============================================================

java.lang.IllegalArgumentException: Property does not exist
	at org.mozilla.gecko.util.NativeJSObject.getString(Native Method)
	at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:144)
	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2200)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:354)
	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:178)

~150 crashes since the 26th of July across all devices

Comment 1

[Richard Newman [:rnewman]]

That means this is failing:

        // First try native listeners.
        final String type = message.getString("type");

presumably because there's a sender that isn't specifying a type.

We probably need to read through every message send to figure this out.

Pretty confident marking this as tracking.

Comment 2

[Richard Newman [:rnewman]]

I just manually looked through every sendEventToGecko and handleGeckoMessage under mobile/android. Didn't find any. Need better logging. Jim?

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Regression from bug 1039048 I guess. One of the crash comment says, "I think quickgestures Add-on is a cause".

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

I contacted the addon author. We can put on a bandaid if this keeps being a problem.

Comment 5

[Aaron Train [:aaronmt]]

There's many add-ons listed in the crash reports.

Comment 6

[Richard Newman [:rnewman]]

Making sure that this is on someone's plate.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

We'll need to use some try/catch handling. Even the older non-native event handling uses a large try/catch wrapper. We can't let the app crash because an add-on bungles a sendMessageToJava. It's really easy for add-ons to do it.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

This is the #2 crash on Nightly. Obviously, add-ons are not being as thorough as we would like. What's the plan to handle this specific issue?

Are we going to use optString (or try/catch)?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Jim Chen [:jchen :nchen] from comment #4)
> I contacted the addon author. We can put on a bandaid if this keeps being a
> problem.

After looking at a few of these crashes, QuickGestures does seem to be a common factor. I found the source using the MXR for addons (requires LDAP). There is an old style sendMessageToJava call here:

https://mxr.mozilla.org/addons/source/416672/bootstrap.js#165

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

For now, I'm going to change the exception from IllegalArgumentException to a custom exception and catch that in EventDispatcher

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Throw InvalidNameException when a property does not exist or has the wrong type (v1)

Use a custom exception so that we can catch it in EventDispatcher, while other IllegalArgumentExceptions are treated normally.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Test for NativeJSObject exceptions

Test proper behavior including EventDispatcher ignoring InvalidNameException.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Catch invalid name exceptions in EventDispatcher (v1)

Catch InvalidNameException and not crash.

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 8468032 [details] [diff] [review]
Catch invalid name exceptions in EventDispatcher (v1)

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

::: mobile/android/base/EventDispatcher.java
@@ +165,5 @@
> +            try {
> +                for (final NativeEventListener listener : listeners) {
> +                    listener.handleMessage(type, message, callback);
> +                }
> +            } catch (final NativeJSObject.InvalidNameException e) {

I have a gentle preference for InvalidPropertyException or InvalidFieldException, because you also throw this for incorrect types, and indeed not really for 'invalid' names!

@@ +174,5 @@
>              return;
>          }
>  
>          try {
>              // If we didn't find native listeners, try JSON listeners.

Do we need the same band-aid applied down here?

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Catch invalid name exceptions in EventDispatcher (v1.1)

(In reply to Richard Newman [:rnewman] from comment #14)
> Comment on attachment 8468032 [details] [diff] [review]
> Catch invalid name exceptions in EventDispatcher (v1)
> 
> Review of attachment 8468032 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/EventDispatcher.java
> @@ +165,5 @@
> > +            try {
> > +                for (final NativeEventListener listener : listeners) {
> > +                    listener.handleMessage(type, message, callback);
> > +                }
> > +            } catch (final NativeJSObject.InvalidNameException e) {
> 
> I have a gentle preference for InvalidPropertyException or
> InvalidFieldException, because you also throw this for incorrect types, and
> indeed not really for 'invalid' names!

Changed to InvalidPropertyException.

> @@ +174,5 @@
> >              return;
> >          }
> >  
> >          try {
> >              // If we didn't find native listeners, try JSON listeners.
> 
> Do we need the same band-aid applied down here?

These should generate JSONExceptions, which are already handled.

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/fx-team/rev/4ee866cfbfec
https://hg.mozilla.org/integration/fx-team/rev/7143dae56cf5
https://hg.mozilla.org/integration/fx-team/rev/73fca7e163ca

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ee866cfbfec
https://hg.mozilla.org/mozilla-central/rev/7143dae56cf5
https://hg.mozilla.org/mozilla-central/rev/73fca7e163ca