Closed Bug 1046880 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

34 Branch
All
Android
defect
Not set
critical

Tracking

(firefox34 affected, fennec34+)

RESOLVED FIXED
Firefox 34
Tracking Status
firefox34 --- affected
fennec 34+ ---

People

(Reporter: aaronmt, Assigned: jchen)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files, 1 obsolete file)

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
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.
tracking-fennec: ? → 34+
I just manually looked through every sendEventToGecko and handleGeckoMessage under mobile/android. Didn't find any. Need better logging. Jim?
Flags: needinfo?(nchen)
Regression from bug 1039048 I guess. One of the crash comment says, "I think quickgestures Add-on is a cause".
Blocks: 1039048
Flags: needinfo?(nchen)
Keywords: regression
I contacted the addon author. We can put on a bandaid if this keeps being a problem.
There's many add-ons listed in the crash reports.
Making sure that this is on someone's plate.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
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.
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)?
(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
For now, I'm going to change the exception from IllegalArgumentException to a custom exception and catch that in EventDispatcher
Use a custom exception so that we can catch it in EventDispatcher, while other IllegalArgumentExceptions are treated normally.
Attachment #8468029 - Flags: review?(blassey.bugs)
Test proper behavior including EventDispatcher ignoring InvalidNameException.
Attachment #8468031 - Flags: review?(mark.finkle)
Catch InvalidNameException and not crash.
Attachment #8468032 - Flags: review?(mark.finkle)
Attachment #8468031 - Flags: review?(mark.finkle) → review+
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?
Attachment #8468032 - Flags: review?(mark.finkle) → review+
Attachment #8468029 - Flags: review?(blassey.bugs) → review+
(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.
Attachment #8468032 - Attachment is obsolete: true
Attachment #8468641 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4ee866cfbfec
https://hg.mozilla.org/mozilla-central/rev/7143dae56cf5
https://hg.mozilla.org/mozilla-central/rev/73fca7e163ca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Flags: qe-verify? → qe-verify-
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.