Determine how to handle GeckoRequest error callbacks

RESOLVED FIXED in Firefox 36

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

Trunk
Firefox 36
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Right now, errors that occur in JS during a GeckoRequest can be handled by catching the exception on the JS side, then responding to the GeckoRequest with any error data in the callback object. We should consider whether we want to use onError for this. We do have a GeckoRequest#onError callback already, but we advise against using it in the comments because the origin of the error is unclear in Java.
(Assignee)

Comment 1

4 years ago
Created attachment 8522559 [details] [diff] [review]
Forward the GeckoRequest error stack to Java

Looking at bug 1097098, it's impossible to figure out where we threw in Gecko, making it difficult to fix bugs. We should forward the error message/stack to use in the RuntimeException.

With this patch, errors will now look like this:

E/GeckoCrashHandler( 7383): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 72678 ("Gecko")
E/GeckoCrashHandler( 7383): java.lang.RuntimeException: Unhandled error for GeckoRequest Robocop:GeckoRequestException: error!
E/GeckoCrashHandler( 7383): JS stack:
E/GeckoCrashHandler( 7383): add_exception_listener/<@testGeckoRequest.js:18:11
E/GeckoCrashHandler( 7383): requestHandler.observe<@resource://gre/modules/Messaging.jsm:147:28
E/GeckoCrashHandler( 7383): TaskImpl_run@resource://gre/modules/Task.jsm:314:40
E/GeckoCrashHandler( 7383): TaskImpl@resource://gre/modules/Task.jsm:275:3
E/GeckoCrashHandler( 7383): createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:14
E/GeckoCrashHandler( 7383): 
E/GeckoCrashHandler( 7383): 	at org.mozilla.gecko.util.GeckoRequest.onError(GeckoRequest.java:89)
E/GeckoCrashHandler( 7383): 	at org.mozilla.gecko.GeckoAppShell$3.handleMessage(GeckoAppShell.java:438)
E/GeckoCrashHandler( 7383): 	at org.mozilla.gecko.EventDispatcher.dispatchEvent(EventDispatcher.java:168)
E/GeckoCrashHandler( 7383): 	at org.mozilla.gecko.GeckoAppShell.handleGeckoMessage(GeckoAppShell.java:2260)
E/GeckoCrashHandler( 7383): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoCrashHandler( 7383): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoCrashHandler( 7383): 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
E/GeckoCrashHandler( 7383): 	at org.mozilla.gecko.GeckoAppShell.runGecko(GeckoAppShell.java:371)
E/GeckoCrashHandler( 7383): 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:190)
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #8522559 - Flags: review?(wjohnston)
Nice!
Comment on attachment 8522559 [details] [diff] [review]
Forward the GeckoRequest error stack to Java

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

I think you can do this a little more resiliently (so that you didn't have to rewrite your test). What happens if you "throw 'Error'" with this code? I guess you just get nothing in the Java stack?

::: mobile/android/modules/Messaging.jsm
@@ +158,5 @@
>  
> +      Messaging.sendRequest({
> +        type: "Gecko:Request" + wrapper.id,
> +        error: {
> +          message: e.message,

I would probably do e.message || e to try and catch things like throw "Error"

@@ +159,5 @@
> +      Messaging.sendRequest({
> +        type: "Gecko:Request" + wrapper.id,
> +        error: {
> +          message: e.message,
> +          stack: e.stack

Building the stack using Components.stack would also make this work even if the error wasn't and Error-object. i.e. something like:

var stackString = "";
var s = Components.stack
while (s) {
  stackString += s.formattedStack + "\n" 
  s = s.caller;
}
Attachment #8522559 - Flags: review?(wjohnston) → review+
(Assignee)

Comment 4

4 years ago
(In reply to Wesley Johnston (:wesj) from comment #3)
> I would probably do e.message || e to try and catch things like throw "Error"

Went a step further and changed this to "e.message || (e && e.toString()" in case the user does something like "throw true" (which would previously fail at the optString call in Java).

> Building the stack using Components.stack would also make this work even if
> the error wasn't and Error-object

Simply using "Components.stack.formattedStack" seems to give us a stack, so I made this change. But I'm not sure how useful it is; the stack will always end up looking like this in such situations:

requestHandler.observe<@resource://gre/modules/Messaging.jsm:163:10
TaskImpl_run@resource://gre/modules/Task.jsm:314:39
TaskImpl@resource://gre/modules/Task.jsm:275:2
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:249:13

https://hg.mozilla.org/integration/fx-team/rev/1b946e43e5dc
https://hg.mozilla.org/mozilla-central/rev/1b946e43e5dc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.