Closed Bug 1052158 Opened 10 years ago Closed 10 years ago

Determine how to handle GeckoRequest error callbacks

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: bnicholson, Assigned: bnicholson)

References

Details

Attachments

(1 file)

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.
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)
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+
(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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: