Remove Duktape specific error codes

[svaarala]

Remove Duktape specific error codes like DUK_ERR_API_ERROR, DUK_ERR_ALLOC_ERROR, etc, for several reasons:

• The custom error codes don't have an Ecmascript counterpart, e.g. DUK_ERR_ALLOC_ERROR is represented as a plain Error which is confusing.
• The custom error codes are not used very much (with a few exceptions like DUK_ERR_API_ERROR and DUK_ERR_INTERNAL_ERROR) and some were actually unused at this point.
• The internal convention was mixed in that RangeError was used for internal limit errors (mirroring what e.g. V8 does) so it makes sense to drop the custom error codes entirely and map internal error situations to the standard errors.

The following mapping is used for error codes that may matter:

• DUK_ERR_API_ERROR -> TypeError
• DUK_ERR_ALLOC_ERROR -> Error
• DUK_ERR_INTERNAL_ERROR -> Error

With this change in place it's no longer necessary to improve API error handling behavior for custom errors. Fixes #740.

Tasks:

• Remove custom error codes from API
• Fix error macros
• Improve error message / call site sharing for modified error macros
• Fix internal call sites to use standard errors as appropriate
• Fix API test failures
• Consider error mappings once more; should index/value stack size related API errors actually be RangeErrors?
• Touch up a few error messages
• Website and API documentation fixes
• 2.0 migration note
• Releases entry

[svaarala]

Footprint impact seems to be about 400-500 bytes smaller.

[fatcerberus]

Looking at the diff, I don't understand how footprint was reduced. It looks like you just removed some #defines and replaced code using the custom errors with standard ones--the code size should be unaffected?

[svaarala]

There are for example string conversions for the error codes, so there are fewer codes to convert now. Also, there are fewer error throwing helpers.

[svaarala]

The current branch changes all API errors to TypeError. I think changing some API errors to RangeErrors might make sense, e.g. for invalid value stack indices, attempt to push beyond currently allocated stack at least.

[saghul]

On 01/06/16 10:36, Sami Vaarala wrote:

The current branch changes all API errors to |TypeError|. I think
changing some API errors to |RangeError|s migth make sense, e.g. for
invalid value stack indices, attempt to push beyond currently allocated
stack at least.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#827 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AATYGNz3-W7hfUb03RowfxBZUsRtzUWaks5qHVKvgaJpZM4IrG9R.

Internal exceptions can be useful in order to know where the error is at
first sight, no?

Tangentially related: in sjs I'm considering using a SystemExit
exception (a la Python or Ruby) to exit the process, then catch it and
do a graceful shutdown, before calling exit(3).

Saúl Ibarra Corretgé
http://bettercallsaghul.com

[svaarala]

@saghul I'm not sure I understand your comment. The error messages and file/line information in the stack trace pinpoint the error quite well (up to the internal Duktape C source line).

In any case, e.g. DUK_ERR_API_ERROR was represented by a plain Ecmascript Error so that it would string coerce to for example Error: foo bar.

This pull would change that to either TypeError: foo bar or maybe RangeError: foo bar if the error is index/stack size related.

[saghul]

On 01/06/16 11:25, Sami Vaarala wrote:

@saghul https://github.com/saghul I'm not sure I understand your
comment. The error messages and file/line information in the stack trace
pinpoint the error quite well (up to the internal Duktape C source line).

In any case, e.g. |DUK_ERR_API_ERROR| was represented by a plain
Ecmascript |Error| so that it would string coerce to for example |Error:
foo bar|.

This pull would change that to either |TypeError: foo bar| or maybe
|RangeError: foo bar| if the error is index/stack size related.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#827 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AATYGL7063zVLb6POFOoknDr_J4quacpks5qHV4PgaJpZM4IrG9R.

Ah, I misunderstood. I thought it would show up as APIError in the
stracktrace... Sorry for the noise.

Saúl Ibarra Corretgé
http://bettercallsaghul.com

[svaarala]

@saghul That would be the other logical option, to promote internal errors to have Ecmascript counterparts.

There were quite a lot of different internal errors though, so at least half of them should be removed in any case :) And since it seems the prevailing approach is to map internal stuff to e.g. RangeErrors (V8 does this for example) the other clean option is that there are no custom errors for Duktape internals.

Then again, InternalError is used by some engines (Rhino IIRC).

[fatcerberus]

One thing I noticed reading the diff is that there's no longer a way to detect allocation errors so that Duktape will attempt to augment them. Will this cause issues? Namely I'm worried that the augmentation will fail with another allocation error, leading to an infinite loop.

In theory though the second error should become a DoubleError, stopping the cycle. Of course that has its own issues--namely obscuring the original cause of the error.

[fatcerberus]

It might be worth keeping InternalError (and have it string coerce to that) and then map allocation errors to it. Internal errors could then have augmentation skipped, since an internal error by definition comes from Duktape code (we wouldn't expose it through the public API)

Note that I agree with things like limits being hit should remain as RangeErrors - I'd only suggest that truly exceptional circumstances from which Duktape can't meaningfully recover be InternalErrors, such as out of memory situations.

Another idea (I seem to have a lot of them this morning...) is to make InternalError a "strong" error so that it keeps getting rethrown until the application exits Duktape completely. Right now it's possible for Ecmascript code to catch e.g. AllocError, which could be problematic.

[fatcerberus]

Hm, then again it would probably be better to generalize that "strong error" logic so that it could be used anywhere (e.g. executor timeout, which is a RangeError), rather than special-casing based on the error type.

[svaarala]

Special casing the allocation error shouldn't be necessary, and it's in fact quite possible that a large allocation fails but there's plenty of memory to give a proper error for it. The double error logic must work anyway, and it should always be able to catch an error augmenting the error itself.

[svaarala]

For concreteness sake, the double error handling must handle an allocation error while handling another error because it's possible that any error leads to an allocation error while handling it. The fact that the initial error is an allocation error shouldn't make any difference. It's quite probable that if the initial error is an allocation error, the error handling will fail too, but that shouldn't cause problems as it needs to work anyway. The current behavior is actually creating a semi-cold path which I dislike.

[fatcerberus]

That's a good point. Logically alloc errors don't fall under "internal" anyway, "internal error" to me implies the error is entirely the fault of the code generating it, i.e. Duktape has detected that it's in an invalid state. Sort of like a release-mode assertion failure. :)

[svaarala]

Release-mode assertion failures should technically lead into a fatal error, because there's no safe way to continue after an assertion fails :) There actually two kinds of internal errors now - some are assertion like (for example, there's an out-of-bounds argument for an indirect load opcode which should never happen because the compiler is supposed to work) and some are safe ways of avoiding issues ahead.

[fatcerberus]

The main thing I'm concerned about re: augmentation of allocation errors is that it's likely the augment will fail, which will convert it to a double error and thereby obscure the original cause (JS doesn't have nested exceptions). DoubleError usually indicates something is wrong with the application, for example a misbehaved errCreate, whereas alloc errors are in some sense "normal".

[svaarala]

But the behavior before this change was that we'd never even try to create a proper allocation error, treating it like a double error that already occurred. So why would the behavior be any worse?

[fatcerberus]

Hm, I admit I'm not entirely familiar with how the augmentation process works - but I thought for errors thrown by C the call site information was already there even if it's not augmented. If this is not the case then yes, you're right - the old behavior is no better. :-)

[svaarala]

Right, so at a high level, an error is always an object - to augment it requires memory for the property table. The "double error" is a pre-allocated error object which is thrown and re-thrown multiple times when actual error creation fails. The intent is to avoid allocation for creating the error - so it can't have any dynamic data.

There have been several requests, incidentally, to allow some kind of memory reserve for creating allocation error objects. That's in principle a good idea I think (at least as an option), but the details of it are rather awkward especially for low memory targets (and where else does clean out-of-memory handling really matter): the reserve needs to be released and locked, it cannot be too large (considering there are targets with 64kB of memory or less) but not too small either, etc, etc.

[svaarala]

Re: the C call site information, it goes into _Tracedata which is an array and needs allocation, so it's unfortunately not immune to allocation errors :-/

[fatcerberus]

Agree on clean out-of-memory handling not being a priority. Honestly in minisphere I don't even check the return value of malloc for small allocations - if a malloc returns NULL the engine will just harmlessly segfault. Probably not the cleanest thing to do, but a NULL defererence is not really exploitable to my knowledge.

Obviously that's not really an option for Duktape due to low-memory concerns making allocation errors an actual concern. On PC platforms with gigabytes of RAM the chance of failing to allocate a small structure is pretty much nil (or never, in the case of Linux) :)

[fatcerberus]

By the way before you get too nervous about the above, I do try to check allocations which are potentially unbounded (allocating an array with arbitrary size, e.g.) to avoid bugs like this:
http://tk-blog.blogspot.com/2009/01/exploitable-userland-null-pointer.html

The allocations I don't usually bother to check are things like:

something = calloc(1, sizeof(something_t));

[svaarala]

The most difficult case for handling allocation errors is a low memory environment with a sandbox that tries to protect against actively malicious code. In that case allocation errors cannot lead to fatal errors but must allow the sandbox to cleanly shut down. There's at least one issue in the error handling path which doesn't work well in this scenario (I fixed a few other issues a while back). In essence, the error handling path must not allocate anything, or if it does allocate something, it must be able to deal with an allocation error without causing an error. Otherwise a protected call may throw an error which makes sandboxing guarantees difficult to achieve.

[fatcerberus]

Anyway I'll leave it up to you to figure out the best way to proceed with this pull, since I'm clearly in over my head on this one.

[svaarala]

@fatcerberus Actually, I think you're right re: the augmentation processing -- re-checking how master handles this, an error object is created for an allocation error (but not a double error). It just doesn't get a throw-time augmentation (Duktape.errThrow()). It does get a stack trace etc, though, only user augmentation handling is missing.

This is a really minor difference in behavior in practice, and trying the errThrow() augmentation should not be an issue. If it is, it would also be an issue if errThrow() e.g. ran out of memory for a non-allocation related error.

[svaarala]

I have a few issues re: improving the error handling path and once I get to that I'll do a write-up of what the critical parts of that handling are. It's highly non-trivial, unfortunately, because a lot of features come into play (garbage collection, finalization, side effects, allocation errors, etc).

[svaarala]

Also, adding a setjmp catchpoint for error handling just makes the issue recursive: if an error occurs during error handling we still need to clean up the value stack, etc. So it needs pretty careful thought :)

[fatcerberus]

It's turtles all the way down. :)

[svaarala]

🐢 🐢

[svaarala]

Ok, now API related errors like attempt to push beyond allocated value stack, invalid index, invalid count (not enough elements for e.g. pop) are RangeErrors because they're related to operations being out of bounds. Type check calls like duk_require_boolean() use TypeError because a type/value doesn't match requirements. This matches common Ecmascript conventions so should be intuitive for calling code.

[svaarala]

Final footprint reduction ranges roughly from 300 to 700 bytes.