svaarala / duktape

Duktape - embeddable Javascript engine with a focus on portability and compact footprint
MIT License
5.93k stars 514 forks source link

Continuing after fatal error #1620

Open dbsGen opened 7 years ago

dbsGen commented 7 years ago

The wrong js script will make duktape run into duk_fatal_raw, and it will freeze the program. I try to remove the loop but then the program got crashed.

Any way to make the program still running, when I eval the wrong js script?

    DUK_EXTERNAL void duk_fatal_raw(duk_context *ctx, const char *err_msg) {
  duk_hthread *thr = (duk_hthread *) ctx;

  DUK_ASSERT_CTX_VALID(ctx);
  DUK_ASSERT(thr != NULL);
  DUK_ASSERT(thr->heap != NULL);
  DUK_ASSERT(thr->heap->fatal_func != NULL);

  DUK_D(DUK_DPRINT("fatal error occurred: %s", err_msg ? err_msg : "NULL"));

  /* fatal_func should be noreturn, but noreturn declarations on function
   * pointers has a very spotty support apparently so it's not currently
   * done.
   */
  thr->heap->fatal_func(thr->heap->heap_udata, err_msg);

  /* If the fatal handler returns, all bets are off.  It'd be nice to
   * print something here but since we don't want to depend on stdio,
   * there's no way to do so portably.
   */
  DUK_D(DUK_DPRINT("fatal error handler returned, all bets are off!"));
  for (;;) {
      /* loop forever, don't return (function marked noreturn) */
  }
  }
fatcerberus commented 7 years ago

You need to use one of the "protected" functions to catch any errors; uncaught errors will go into the fatal error handler like you say. Read up on duk_peval() etc. in the Duktape API.

@svaarala The current default fatal error behavior seems to be catching a lot of people off-guard; I wonder what could be done to improve it? Maybe the protected calls need to be better emphasized in documentation... not sure.

dbsGen commented 7 years ago

@fatcerberus Thanks, I will try it later.

svaarala commented 7 years ago

@fatcerberus Concrete suggestions welcome :)

The first issue is that there should be a readable explanation of the how and why. These are currently covered by:

The second issue is whether it's easy to find when getting to use Duktape. I've been assuming new users would at least read the Programming model section, but maybe they don't. If so, maybe it could be moved somewhere else.

The third issue that comes to mind is example code. I'm not sure if all (or even most) trivial examples incorporate error handling properly. This could be changed if there are shortcomings here.

One could also try to enforce somehow that the initial call into Duktape was a protected one and fail an informative way otherwise. There are two problems with this:

fatcerberus commented 7 years ago

For what it’s worth, when I got started with Duktape, I didn’t know about the protected calls for several months, and even when I did first incorporate proper error handling for execution of the main script, it was NOT by adding a protected call but rather by overriding the fatal error handler. So there’s definitely a deficiency in the documentation here, but I’m not sure what the problem is concretely yet.

I’ll look over the current documentation when I get a chance and see if I can come up with any suggestions for improvement.

svaarala commented 7 years ago

If you didn't encounter protected calls that probably means you didn't read the "Programming model" guide section ;-)

svaarala commented 7 years ago

So what I was trying to get to above is that one issue is (1) does the documentation exist, and another is (2) do users read it. I've been assuming users would read Getting started and Programming model at a minimum. Otherwise one would be unaware of value stack index behavior, Duktape/C function conventions, error handling, relationship between Ecmascript functions and Duktape/C functions, etc. Error handling is covered under Programming model in http://duktape.org/guide.html#error-handling and it covers protected calls and fatal error handling.

If users tend not to read the documentation that exists, it doesn't matter if the specific Guide sections are improved. Rather, the question becomes why users don't read the documentation: is it too deep in the guide, should the introduction more strongly recommend reading the sections, etc.

With some libraries it's possible to print useful diagnostic messages to e.g. stderr so users will get some feedback even if they don't read the documentation. For example, when calling a deprecated API function one might print a diagnostic message recommending an alternative. Extending that, unprotected calls could maybe generate a friendly warning when using a development oriented build. But, for Duktape specifically this approach seems too awkward to me for several reasons.

svaarala commented 7 years ago

One more point re: documentation placement is that the solution is sometimes not to have more documentation but instead to have less.

Originally there was no Wiki so the Guide and the API documents tried to cover all relevant topics. Now with the Wiki in place (barebones as it is) I've slowly moved some detailed discussion into the Wiki and the Guide now just introduces the subject and points to the Wiki. Ideally the Guide would be an umbrella document covering all relevant high level topics, but point to other places (Wiki etc) for worked out examples.

There's much more to do in this regard. Maybe if the Guide was much smaller, that would motivate users to read the critical sections (like Getting started and Programming model) before tackling the API.

It's also possible to go too much in this direction so that the Guide just becomes a topic index. So, IMO the Guide should introduce each top level concept in a "normative", carefully written out fashion, while the Wiki can be more free-form and example based, and it's OK to have overlap between Wiki pages.

fatcerberus commented 7 years ago

If you didn't encounter protected calls that probably means you didn't read the "Programming model" guide section ;-)

Guilty as charged. :)

I came into Duktape having some passing familiarity with Lua, so my basis for getting started was just the little example you give on the main page:

/* test.c */
#include <stdio.h>
#include "duktape.h"

int main(int argc, char *argv[]) {
  duk_context *ctx = duk_create_heap_default();
  duk_eval_string(ctx, "1+2");
  printf("1+2=%d\n", (int) duk_get_int(ctx, -1));
  duk_destroy_heap(ctx);
  return 0;
}

That example for me was pretty self-explanatory: I already understood the value stack model because of Lua, so that little sample was enough of a jumpstart for me to get a project (miniSphere) going, and I learned the rest incrementally by studying individual functions in the API reference (http://duktape.org/api.html). I didn't read the Guide at all until much later (http://duktape.org/guide.html). I'm a pretty hands-on learner, so this is typical for me. I learn more about how things work by using them than I do reading about them. Of course that leads to pitfalls but I'm usually good at digging myself out of such holes. :)

As someone who learned Duktape primarily through the API reference, what I think might help is a note under the sections for unprotected functions that if these are called at the top level and an error is thrown, it will be fatal. Right now, e.g. duk_eval() just says:

Evaluate the Ecmascript source code at the top of the stack, and leave a single return value on top of the stack. May throw an error, errors are not caught automatically. The filename associated with the temporary eval function is "eval".

It's almost like a footnote, easy to miss. Some emphasis of that point, plus a cross-reference to the protected variant, would go a long way, I think.

Of course if someone doesn't read any documentation at all, there's nothing we can do about that. But for those that do, sometimes a little bit of redundancy in the documentation to cover multiple points of entry can help.

fatcerberus commented 7 years ago

For what it's worth, "errors are not caught automatically", to a C developer, doesn't usually imply that the thrown error will propagate through the C code (they probably just expect Duktape to set an error flag somewhere, which is how most C libraries work). In C++ this kind of propagation is more expected, but C doesn't have exceptions, so control flow of that kind are not a normal thing to encounter. It was only my Lua experience that prevented me personally from falling victim to it :)

fatcerberus commented 7 years ago

Just took a quick look at the Lua API reference, documentation for lua_call() says:

Any error inside the called function is propagated upwards (with a longjmp).

Note the explicit reference to longjmp, "propogated upwards", and that it's a separate paragraph. Now I sit up and take notice that I may lose control if an error occurs :)

svaarala commented 7 years ago

But note that virtually every call except protected API calls are unprotected. It's not just compile/eval/call functions that may throw and cause a fatal error.

fatcerberus commented 7 years ago

True, but the most likely ones to fail (and the ones that seem to bite most people) are the ones that compile/execute code.

svaarala commented 7 years ago

I guess the following minimal changes might improve the chances of being noticed:

I'm somewhat torn about covering a certain topic in multiple entrypoints. Yes, it makes it more likely for users to catch a certain note. But when applied to dozens of topics, the documentation will be much longer and there will be a lot of cross-references, masking some of the actual content. What's worse, the different scattered descriptions may easily fall out of sync, making the documentation inconsistent and more confusing.

fatcerberus commented 7 years ago

Those suggestions sound good to me. I don't expect a lot of duplication of text (that's hard to keep in sync, like you say); just a minimal cross-reference with emphasis where needed would probably go a long way for people like me that learn using only an API reference + Intellisense. :)

fatcerberus commented 7 years ago

@dbsGen Sorry I hijacked your issue :)

svaarala commented 7 years ago

[...] just a minimal cross-reference with emphasis where needed would probably go a long way for people like me that learn using only an API reference + Intellisense. :)

To be clear, I think most people are like that at times (me included), so I'm fully symphatetic to that scenario :)

From a documentation point of view it's quite challenging however. In many cases you can get a 95% working outcome just by guessing, but you may be lacking the critical 5% that only comes into play in special cases. It may then be painful to fix that 5% (incidentally I just managed to get myself into this situation with Node.js recently). So unfortunately there's a core set of non-obvious concepts that one must know to work effectively with any library - and you either learn that by reading something or by trial and error (sometimes way too late in the process). The smaller that set is, the better, obviously.

I'm sure there's a lot to improve in documentation in this regard. A couple of ideas come to mind (partly already covered above):

As I said above sometimes frameworks can warn about common mistakes and that would be great if it could be done in some non-intrusive way ("Warning: initial call into Duktape is not protected, consider using protected calls"; show at most once per execution). This is maybe something to consider down the road. It's not very difficult but also not trivial.

fatcerberus commented 7 years ago

Regarding that warning: If I remember correctly, the original default fatal error behavior in 1.x was to print something to the console and then segfault, but that was removed later for portability reasons and replaced with an infinite loop. So from the end-user point of view it's just a lockup. I can understand why people get thrown off.

So maybe we could bring back the "segfault on purpose" behavior, and then add a comment in duk_fatal_raw() that, if you get to this point, you should read the documentation to learn about protected calls, etc. This way users tracing the crash through their debugger will get stopped at a point where the comment is clearly in view.

svaarala commented 7 years ago

The default is actually abort() which is more portable and predictable than a segfault. An infinite loop is used to prevent progress only if the user provides a fatal error handler that returns (violating the contract).

svaarala commented 7 years ago

@fatcerberus Re: the suggestion to bring back the default segfault behavior, AFAIK most debuggers treat abort() similarly and allow debugging the call site. And abort() is preferable because it's more portable and predictable.

I agree with the suggestion that maybe the abort() location could provide a useful protected call reference for users that encounter the abort() in a debugger.

This is how an abort looks like with Valgrind. Source:

#include <stdio.h>
#include <stdlib.h>

void main(void) {
    printf("foo\n");
    abort();
    printf("bar\n");
}

Normal output without valgrind:

$ /tmp/test
foo
Aborted

Output with valgrind:

$ valgrind /tmp/test
==17761== Memcheck, a memory error detector
==17761== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==17761== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==17761== Command: /tmp/test
==17761== 
foo
==17761== 
==17761== Process terminating with default action of signal 6 (SIGABRT)
==17761==    at 0x4E6F428: raise (raise.c:54)
==17761==    by 0x4E71029: abort (abort.c:89)
==17761==    by 0x400578: main (test.c:6)
==17761== 
==17761== HEAP SUMMARY:
==17761==     in use at exit: 0 bytes in 0 blocks
==17761==   total heap usage: 1 allocs, 1 frees, 1,024 bytes allocated
==17761== 
==17761== All heap blocks were freed -- no leaks are possible
==17761== 
==17761== For counts of detected and suppressed errors, rerun with: -v
==17761== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Aborted

The output includes the callstack, just like with a segfault, at least with Valgrind.

I actually considered removing the default fatal error handler and always requiring an explicit fatal error handling in heap creation. While this adds some boilerplate (a minimal fatal function is a one-liner so not by a lot) it would force an introduction to fatal errors and protected calls. I think that change might make sense because many users seem to ignore the strong recommendation to provide a fatal error handler, despite it being mentioned in the API document, Guide, the Wiki, etc.

svaarala commented 7 years ago

1621 adds a comment to the abort() call site.