Opened 13 years ago
Last modified 3 years ago
#742 assigned task
Handle JS errors properly
Reported by: | Philip Taylor | Owned by: | Yves |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Backlog |
Component: | Core engine | Keywords: | |
Cc: | Patch: |
Description (last modified by )
Our current handling of SpiderMonkey errors is all broken: mostly we assume they don't happen, and if they do we report an error but don't propagate the exception. I've not noticed this being a significant problem in practice, but it's not nice.
I believe the way it's meant to work is (as here):
- If a JSAPI function returns
JS_FALSE
, it has reported an error and thrown an exception; either propagateJS_FALSE
back into JSAPI, or useJS_ClearPendingException
to catch it. - To trigger a JSAPI error, use
JS_ReportError
and returnJS_FALSE
.
Change History (5)
comment:1 by , 13 years ago
comment:2 by , 13 years ago
If you mean JSOPTION_STRICT
, that's completely separate from "use strict"
. (The latter is standard ES5 strict mode, the former is an old SpiderMonkey-specific feature, and they had an unfortunate name conflict.)
comment:3 by , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
The way error reporting works in SpiderMonkey will change in the future, so I'm going to wait until that has happened before spending too much time on it. https://wiki.mozilla.org/JavaScript:SpiderMonkey:LongTermPlans#Remove_error_reporting.2C_replace_it_with_exception-catching_mechanisms
comment:4 by , 4 years ago
Description: | modified (diff) |
---|
See Phab:rP23773 for a situation where we are not correctly picking up JS errors.
My suggestions are thus:
One option is throwing a C++ exception, since the boolean return value is used to indicate something different (see D2727). It's a typical 'trilean' -> true, false, ERROR. No other GUI code that I know of throws an exception, so it seems like something that shouldn't be looked at in a vacuum -> for another day.
Another alternative is calling JS_IsExceptionPending after each call to a ScriptEventWithReturn. This seems more productive, but less systematic, and somewhat invasive. It would probably be a better direction to have LOGERROR (or a variant thereof) also check for pending JS exceptions, make sure JS reports them correctly, then moves on.
A few related thoughts:
If we plan to keep USE_STRICT enabled, we can remove the ugly hack from the error reporter and not prepend "use strict"; to loaded scripts. Assuming they have equivalent behavior.
Slightly more information could be added to errors by checking the flags for exceptions (JSREPORT_IS_EXCEPTION) or strict warnings (JSREPORT_IS_STRICT).
Something should be done about js "out of memory" errors. It's an uncatchable error, but I doubt we want to keep executing the script as if nothing is wrong. Better would be to present a message to the user and end the game, but it seems messy if the error reporter is responsible for that.