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

RESOLVED FIXED in Firefox 34

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aaronmt, Assigned: jchen)

Tracking

({crash, regression})

34 Branch
Firefox 34
All
Android
crash, regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox34 affected, fennec34+)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 3

3 years ago
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)
(Reporter)

Updated

3 years ago
Keywords: regression
(Assignee)

Comment 4

3 years ago
I contacted the addon author. We can put on a bandaid if this keeps being a problem.
(Reporter)

Comment 5

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

Comment 10

3 years ago
For now, I'm going to change the exception from IllegalArgumentException to a custom exception and catch that in EventDispatcher
(Assignee)

Comment 11

3 years ago
Created attachment 8468029 [details] [diff] [review]
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.
Attachment #8468029 - Flags: review?(blassey.bugs)
(Assignee)

Comment 12

3 years ago
Created attachment 8468031 [details] [diff] [review]
Test for NativeJSObject exceptions

Test proper behavior including EventDispatcher ignoring InvalidNameException.
Attachment #8468031 - Flags: review?(mark.finkle)
(Assignee)

Comment 13

3 years ago
Created attachment 8468032 [details] [diff] [review]
Catch invalid name exceptions in EventDispatcher (v1)

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

Comment 15

3 years ago
Created attachment 8468641 [details] [diff] [review]
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.
Attachment #8468032 - Attachment is obsolete: true
Attachment #8468641 - Flags: review+
(Assignee)

Comment 16

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

Updated

3 years ago
Blocks: 1047888
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
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Flags: qe-verify?
(Reporter)

Updated

3 years ago
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.