Infinitely recursive function causes double error

[fatcerberus]

The following:

function f() { f(); }
f();

Causes Duktape to throw "DoubleError: Error in error handling". It would be more informative if this could be fixed to throw a "Too much recursion" error like SpiderMonkey does.

[svaarala]

When I modify the test to:

function f() { f(); }
try {
    f();
} catch (e) {
    print(e.stack || e);
}

I get:

$ ./duk _recursion.js 
RangeError: callstack limit
    duk_hthread_stacks.c:44
    f _recursion.js:1
    f _recursion.js:1
    f _recursion.js:1
    f _recursion.js:1
    f _recursion.js:1
    f _recursion.js:1
    f _recursion.js:1
    f _recursion.js:1
    f _recursion.js:1
    f _recursion.js:1
    [...]

So at least in this case the originally thrown error is indeed a RangeError, and Duktape does have a callstack limit to catch too deep recursion.

What kind of code evaluates the example?

[fatcerberus]

My engine is set up to call a game() function in the main script file, which bootstraps the rest of the game. This is analogous to main() in C/C++. The logic can get complicated at times (I have a makeshift threader that emulates pthreads using recursion), but the DoubleError happens even if I put the example as the first line(s) of game().

This isn't an issue for me currently, the discovery was prompted by me screwing around, as most are (I'm like a human fuzz tester!)

Hm... Wait. It occurs to me that now that, as part of my engine's error handling, I think I make a few Duktape calls (Duktape.act notably)... Could this be the cause of the double error? I guess I'm lucky it didn't turn into a triple error and segfault! :)

[svaarala]

Hehe, there's no "triple error" :) The "double error" handling is just for weird cases where error handling itself fails, so a fixed error object is thrown instead. It can happen e.g. when you run out of memory trying to create an error object.

If by error handling you mean errCreate/errThrow, those calls are protected and any errors are ignored so errors there shouldn't cause a "double error".

But if error handling happens after catching the error, Duktape is no longer in error creation/throwing state so any errors state would be just normal errors. An uncaught error leads to a fatal error as always, so if you do anything complex in the error handling, using a duk_safe_call() for the error handling is a good idea.

[fatcerberus]

By error handling I meant my duk_on_fatal handler, which catches any uncaught errors and displays an error screen before closing the game cleanly via exit(). Hypothetically, if that handler should throw, what would happen?

[svaarala]

The fatal error handler is called when it's no longer safe to continue execution within Duktape. This is also why the fatal error handler is not supposed to return: there's no safe way to continue. For similar reasons you probably shouldn't call into Duktape in a fatal error handler as doing so might be unsafe and might cause a longjmp if an error is thrown.

That's the general contract anyway. In the case of an uncaught error you should be able to call into Duktape although I wouldn't really recommend that as it's not "guaranteed" to work. Anyway, if you do so, you must protect against an error being thrown; if you don't and that error propagates outwards, the results are basically undefined. I'm not sure what happens now; it might cause another fatal error.

If you want to catch uncaught script errors (which is quite useful) it's probably best to use a protected call when you call into scripts and handle errors explicitly rather than relying on a fatal error handler.

[fatcerberus]

Actually, now that I review the code again, here's how I handle uncaught errors (this is in my engine's main()):

on_js_error:
    err_code = duk_get_error_code(g_duk, -1);
    duk_dup(g_duk, -1);
    err_msg = duk_safe_to_string(g_duk, -1);
    al_show_mouse_cursor(g_display);
    duk_get_prop_string(g_duk, -2, "lineNumber");
    line_num = duk_get_int(g_duk, -1);
    duk_pop(g_duk);
    duk_get_prop_string(g_duk, -2, "fileName");
    file_path = duk_get_string(g_duk, -1);
    if (file_path != NULL) {
        filename = strrchr(file_path, ALLEGRO_NATIVE_PATH_SEP);
        filename = filename != NULL ? filename + 1 : file_path;
        fprintf(stderr, "JS Error: %s:%i - %s\n", filename, line_num, err_msg);
        if (err_msg[strlen(err_msg) - 1] != '\n')
            duk_push_sprintf(g_duk, "`%s`, line: %i | %s", filename, line_num, err_msg);
        else
            duk_push_string(g_duk, err_msg);
    }
    else {
        fprintf(stderr, "JS Error: %s\n", err_msg);
        duk_push_string(g_duk, err_msg);
    }
    duk_fatal(g_duk, err_code, duk_get_string(g_duk, -1));

The call to game() is a protected call (duk_pcall()). If the protected call fails, I then branch to on_js_error:. Most of that code was written very early in development, so I don't remember now why I hand off the error to duk_fatal() afterwards. I feel like there must have been a reason.

In any case, this still doesn't explain why I'm getting a double error for infinite recursion. Incidentally: Why is that test not being turned into a tailcall anyway?

[svaarala]

Hmm, I'll try to reproduce the issue and see what's going on.

The test is not a tailcall because f() returns undefined (the default return value). It's equivalent to:

function f() { f(); return undefined; }

It would be a tailcall if defined as:

function f() { return f(); }

[fatcerberus]

So I figured out the cause. The RangeError is being converted to a double error because of my custom errCreate handler. What I gather is that since the callstack is ready to topple already, the attempt to call Duktape.errCreate() on the thrown error triggers the limit again, and well... the double error lives up to its name, even more than usual (it's the same error!)

I can confirm that removing the errCreate() from the equation leads to the correct error coming out.

Test:

Duktape.errCreate = function(err) { return err; }

try {
    function f() { f(); }
    f();
} catch(err) {
    print(err);
}

This prints DoubleError: error in error handling. If you comment out the errCreate declaration however, it outputs the correct RangeError: callstack limit.

[fatcerberus]

@svaarala Were you able to reproduce this?

[fatcerberus]

I noticed that duk_handle_call() can be provided with a flag to ignore the recursion limit. Would that be a solution to this?

[svaarala]

Not necessarily: Duktape.errCreate can make further calls.

[fatcerberus]

Would this work in call handling to work around the issue? #286 currently doublefaults if the debugger is attached and the callstack is full, so this is becoming a real issue now.

if (!thr->handling_error) {
   do reclimit check
}

[fatcerberus]

A better alternative that would avoid blowing the C stack in case of accidental infinite recursion might be to temporarily bump the callstack limit during error handling, allow say an extra 5-10 calls.

[svaarala]

Not really sure what the best approach is. The original reasoning for these limits was just to provide some sanity limit for misbehaving code, without much intent to make them work all that cleanly (just in a more useful manner than running out of memory or such).

Perhaps there could be two separate limits: one for normal code, another for error handling. By the way, in addition to call stack depth there's also a limit on the native recursion depth which may also be hit; and if we're at that limit we'd need to have a second limit for that too. At the moment call handling accepts a "no recursion limit" flag for this purpose, but it's not applied in every place (just for error augmentation IIRC), so it's not a full solution.

There will be call handling reworks for Duktape 1.4 and it's probably best to rework the call handling, error handling etc state as part of that, so perhaps a more cleaner solution will then be possible.

[fatcerberus]

I think if the native limit is reached, double faulting is acceptable: it's the same situation as if a malloc() fails, you're between a rock and hard place. But if only the Ecmascript limit is reached, it should be safe to just say callstack_limit += 10 for the duration of handling, no?

I should try making that change locally, see what happens.

[fatcerberus]

It's just that maintaining two separate limits feels like overengineering to me is all. I like to keep things simple. :)

[fatcerberus]

Because honestly, if you're trying to calculate the Mandelbrot set in an errCreate handler, um... Well, I have no words for that level of stupidity. ;)

[svaarala]

If you say += 10, then you're really establishing a second limit, aren't you?

[fatcerberus]

I guess so, but I see that as just "allowing headroom". The way you described the second limit, I was envisioning a whole extra depth-tracking framework with separate depth and limit variables, etc. That's why I said it was overengineering.

[svaarala]

No, I just meant that there'd logically be two limits, and both of them would be config options. The difference between them is the "+=" value you suggested. I didn't mean there'd be a separate limit counter, just a separate limit for the current counter. (I don't see why a separate counter would even make sense, why would it be better?)

Instead of bumping the current limit it might be easier to implement explicit state flagging for "handling error", and then compute the limit on-the-fly as (handling_error ? limit1 : limit2). This way there'd be no limit variable to restore, as it would just rely on the "handling error" state which is needed anyway. A similar solution could be used for other similar checks without doing a bump-and-restore for them too.

That's not to say a bump-and-restore wouldn't work, but one just has to be careful there's always a control path to restore the original state, in all corner cases like recursive errors, resumes and yields, etc. This kind of critical restore points are already used elsewhere, e.g. property table resize sets a mark-and-sweep to prevent compaction and finalizers (which would interfere with a resize) and is then required to always restore the flags to their previous state.

[fatcerberus]

Interestingly, having done this as an experiment:

thr->heap->handling_error = 1;
thr->heap->call_recursion_limit += 50;  /* headroom for error handling */

...still causes a double error for this when errCreate is active: function f() { f(); } f();

The only thing I can think of is that the native reclimit has been reached, but that doesn't make sense since Duktape should just be able to reuse the same bytecode executor instance for all the recursive calls of f()?

[svaarala]

heap->call_recursion_limit is the native call limit. thr->callstack_max is the unrelated call stack maximum size limit. The limit is checked against a potential new callstack size, so it's an approximate limit. To ensure there's sufficient headroom, that inaccuracy would need to be taken into account.

Right now the resize is just new_size = old_size + DUK_CALLSTACK_GROW_STEP so adding DUK_CALLSTACK_GROW_STEP and some extra should suffice.

But that resize computation should probably be proportional to the current size (e.g. (old_size >> 4) + something)) to ensure amortized resize cost is better bounded. Otherwise the larger and larger a callstack gets, the more time is spent in resizing because reallocs sometimes cause an actual copy.

Other code relying on the resize formula is a bit fragile so that would be nice to avoid if possible.

[svaarala]

By the way, I'm working on call handling (especially its performance) in Duktape 1.4 and many small details may change as a result, so something we work up here might not be directly applicable then.

[fatcerberus]

Duly noted. :)

I'm just trying to come up with a workable temporary fix, as this affects #286 and causes all sorts of weird crashes. For example, the following will segfault with error intercept enabled:

Duktape.fin(Object.prototype, function() { print("*munch*"); });
function f() { f(); } f();

Yes, there's totally a pig eating all the objects as they are finalized. :)

[fatcerberus]

As for resizing, I thought the general idiom was to resize in powers of 2?

[fatcerberus]

Got it!

[svaarala]

Powers of two are good for systems with a lot of memory to waste :)

[svaarala]

Btw, we can merge a simple fix too. Just wanted you to know these things will change soon :)

It'd be ideal to have a testcase for testing for a reasonable headroom (test-dev-error-handling-headroom.js or something), which would also serve to document what can be expected now. It would be a useful regression test for the rework.

[fatcerberus]

"First approximation" as you would call it:

thr->heap->handling_error = 1;
thr->callstack_max += (DUK_CALLSTACK_GROW_STEP * 2);  /* headroom for error handling */

error handling (augment, debug intercept, etc.)

thr->heap->handling_error = 0;
thr->callstack_max -= (DUK_CALLSTACK_GROW_STEP * 2);

longjmp

I'll open a pull after I write a testcase, that shouldn't be too much trouble.

[svaarala]

That shift + add growth model is used in other places in Duktape too (and not just Duktape of course). It works well for small values: the adder ensures unnecessary frequent resizes are avoided for small values. The shift ensures there's little waste and limited realloc copies for large values. Growing in powers of two means 25% waste on average.

For the buffer growth code, for example, the shifter is 4 and the adder is 64. The shifter means there's roughly 1/16 * 0.5 ~= 3% of waste for large values. The adder ensure small buffers also work well. The growth sequence from 0 bytes goes: 0, 64, 132, 204, 280, 361, 447, 538...

Naturally this is still a trade-off between performance and memory usage, and ideally these factors would be configurable for low memory and high performance environments separately. With the duk_config.h header now being autogenerated lifting a lot of these constants into duk_config.h will be straightforward.

[svaarala]

Looks good as long as there's a setjmp catchpoint for all operations in the middle.

It might be useful to add storing and checking for the previous value with DUK_USE_ASSERTIONS so that asserts would catch any assumptions going wrong.

[fatcerberus]

I did a quick search of the Duktape codebase, looks like callstack_max is only set in one place, so this should be adequate:

DUK_ASSERT(thr->callstack_max == DUK_CALLSTACK_DEFAULT_MAX);

[svaarala]

Yeah, that should be good enough for now. In fact callstack_max could be replaced by that define; I left it as a member variable originally because I had a thought it could be set for each thread separately. But since there's no API for that, it's a bit pointless to track it as a member.

[svaarala]

But now that it'd be bumped temporarily, it's actually needed of course :)

[svaarala]

Off topic: do you have other screenshots of the debugger? I'd like to collect a short list of debug client implementations (there are three I know of directly outside of Duktape's example web UI) and screenshots would be nice.

[fatcerberus]

Sure, I could whip up some quick screenshots. I guess you could add minisphere to the list of programs using Duktape too, although I don't actually have a webpage for it... just this forum post:

http://forums.spheredev.org/index.php/topic,1215.0.html

[sva-p]

What would be an appropriate short blurp for "minisphere, using Duktape for..."?

[fatcerberus]

Hm, "for scripting" makes it sound like the use case is more limited than it is. With minisphere you write your entire game in JS using an API provided by the engine, similar to how you write an application in C++ or C#...

I guess "using Duktape for game coding" or something similar would work.

[fatcerberus]

By the way, here's more screenshots: https://drive.google.com/open?id=0BxPKLRqQOUSNbFRvenU1V2hkYjQ

[fatcerberus]

Oh, and there's also minisphere's GitHub repo page you can link to, of course: https://github.com/fatcerberus/minisphere

[fatcerberus]

See #315 for the fix. After all the bugs I've reported, I figured it's time I gave something back. :-)

[svaarala]

Very much appreciated, though uncovering difficult-to-find bugs is very useful too :)

[svaarala]

Added this: https://github.com/svaarala/duktape/commit/b6be339151fbc3b8ec9ec78ae56382087a914a38

[fatcerberus]

Bah, stupid GitHub adding commit references every time I rebase. :-/