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 wraitii)

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 propagate JS_FALSE back into JSAPI, or use JS_ClearPendingException to catch it.
  • To trigger a JSAPI error, use JS_ReportError and return JS_FALSE.

Change History (5)

comment:1 by historic_bruno, 13 years ago

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.

comment:2 by Philip Taylor, 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 Yves, 10 years ago

Owner: set to Yves
Status: newassigned

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 wraitii, 4 years ago

Description: modified (diff)

See Phab:rP23773 for a situation where we are not correctly picking up JS errors.

The above is probably incorrect and soon outdated -> JS_ReportError was mangling stack traces apparently.

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.

Last edited 4 years ago by wraitii (previous) (diff)

comment:5 by wraitii, 3 years ago

In 24187:

Improve JS Exception handling.

  • Check for pending exceptions after function calls and script executions.
  • Call LOGERROR instead of JS_ReportError when there is a conversion error in FromJSVal, since that can only be called from C++ (where JS errors don't really make sense). Instead, C++ callers of FromJSVal should handle the failure and, themselves, either report an error or simply do something else.
  • Wrap JS_ReportError since that makes updating it later easier.

This isn't a systematical fix since ToJSVal also ought return a boolean for failures, and we probably should trigger errors instead of warnings on 'implicit' conversions, rather a preparation diff.

Part of the SM52 migration, stage: SM45 compatible (actually SM52 incompatible, too).

Based on a patch by: Itms

Comments by: Vladislavbelov, Stan`

Refs #742, #4893

Differential Revision: https://code.wildfiregames.com/D3093

Note: See TracTickets for help on using tickets.