1234387 - Assertion failure: initialized, at js/src/vm/Printer.cpp:130 with OOM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 388bdc46ba51 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-simulator=arm --enable-debug, run with --ion-eager min.js):

var max = 40;
setJitCompilerOption("ion.warmup.trigger", max - 10);
oomTest(() => backtrace() || (this) || (this) ? (yield) : (yield));


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x086d19bb in js::Sprinter::checkInvariants (this=0xffffbd74) at js/src/vm/Printer.cpp:130
#0  0x086d19bb in js::Sprinter::checkInvariants (this=0xffffbd74) at js/src/vm/Printer.cpp:130
#1  0x086d1a7f in InvariantChecker (p=0xffffbd74, this=<synthetic pointer>) at js/src/vm/Printer.h:61
#2  js::Sprinter::vprintf (this=0xffffbd74, fmt=0x8b6d078 "#%d %14p %c   %s:%d (%p @ %d)\n", ap=0xffffbd28 "") at js/src/vm/Printer.cpp:206
#3  0x086cb41f in js::GenericPrinter::printf (this=this@entry=0xffffbd74, fmt=fmt@entry=0x8b6d078 "#%d %14p %c   %s:%d (%p @ %d)\n") at js/src/vm/Printer.cpp:51
#4  0x085950a7 in js::DumpBacktrace (cx=cx@entry=0xf7a7b020) at js/src/jsobj.cpp:3658
#5  0x086ab099 in DumpBacktrace (cx=0xf7a7b020, argc=0, vp=0xf4cfd050) at js/src/builtin/TestingFunctions.cpp:2191
#6  0x08695b6a in js::CallJSNative (cx=0xf7a7b020, native=0x86ab070 <DumpBacktrace(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#7  0x08692dc6 in js::Invoke (cx=0xf7a7b020, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:460
#8  0x08682b83 in Interpret (cx=cx@entry=0xf7a7b020, state=...) at js/src/vm/Interpreter.cpp:2781
#9  0x08692aed in js::RunScript (cx=cx@entry=0xf7a7b020, state=...) at js/src/vm/Interpreter.cpp:407
#10 0x08692e7e in js::Invoke (cx=0xf7a7b020, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:478
#11 0x08693d2e in js::Invoke (cx=cx@entry=0xf7a7b020, thisv=..., fval=..., argc=argc@entry=0, argv=argv@entry=0x0, rval=rval@entry=...) at js/src/vm/Interpreter.cpp:512
#12 0x084f70e8 in JS_CallFunction (cx=cx@entry=0xf7a7b020, obj=..., fun=fun@entry=..., args=..., rval=rval@entry=...) at js/src/jsapi.cpp:2803
#13 0x086b714b in OOMTest (cx=0xf7a7b020, argc=1, vp=0xffffc920) at js/src/builtin/TestingFunctions.cpp:1165
#14 0x08695b6a in js::CallJSNative (cx=0xf7a7b020, native=0x86b6f30 <OOMTest(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#35 main (argc=3, argv=0xffffd914, envp=0xffffd924) at js/src/shell/js.cpp:6878
eax	0x0	0
ebx	0x97cb438	159167544
ecx	0xf7e4388c	-136038260
edx	0x0	0
esi	0xffffbd74	-17036
edi	0x0	0
ebp	0xffffbcc8	4294950088
esp	0xffffbcb0	4294950064
eip	0x86d19bb <js::Sprinter::checkInvariants() const+75>
=> 0x86d19bb <js::Sprinter::checkInvariants() const+75>:	movl   $0x82,0x0
   0x86d19c5 <js::Sprinter::checkInvariants() const+85>:	call   0x80f9130 <abort()>


Likely a bug in a shell-only debug function, so not s-s.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: DumpBacktrace: Check the result of Sprinter::init function.

The diff is awful, but apart from the reindentation, this adds a result
string which is used to either carry the default OOM message, or the result.

DumpBacktrace is a debug only function, so printing on stdout is fine, and
not returning error code (the false argument of Sprinter constructor) is
also ok.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8701114 [details] [diff] [review]
DumpBacktrace: Check the result of Sprinter::init function.

Review of attachment 8701114 [details] [diff] [review]:
-----------------------------------------------------------------

Why don't we just leverage the error reporting mechanism in Sprinter? Seems like checking 

if (!sprinter.init())
  return;

at the start of the function ought to be enough, no?

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: dumpbacktrace.patch

That is, doing this. Either this, or just making DumpBacktrace properly fallible.

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8701442 [details] [diff] [review]
dumpbacktrace.patch

Review of attachment 8701442 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TestingFunctions.cpp
@@ +2190,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>      DumpBacktrace(cx);
>      args.rval().setUndefined();
> +    // DumpBacktrace can OOM.
> +    return !cx->isExceptionPending();

I would prefer not to do that, because if we do that then we might change even more the behaviour of the program while we are trying to debug it.

DumpBacktrace was made such that we can call it from gdb.  So changing the isExceptionPending flag is a nasty side-effect that I think we should skip, as this might add more confusion in the debugging session.

Also, maybe we should forbid OOM with the OOMTest function when we allocate within the DumpBacktrace function, such that we can use the DumpBacktrace function while debugging a OOMTest.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8701114 [details] [diff] [review]
DumpBacktrace: Check the result of Sprinter::init function.

Review of attachment 8701114 [details] [diff] [review]:
-----------------------------------------------------------------

OK, your argument that this shouldn't change the behavior of the program makes sense. That being said, I think a user of the debugger won't use the long variant (the one in builtin/TestingFunctions) but they're more likely to use the one that just takes a JSContext* argument.
I am not a big fan of the indentation, but this is a function intended for debugging anyway, so might as well r+

::: js/src/jsobj.cpp
@@ +3641,5 @@
>  {
> +    Sprinter sprinter(cx, false);
> +    static const char* oomMessage = "js::DumpBacktrace: OOM\n";
> +    const char* result = oomMessage;
> +    if (sprinter.init()) {

how about:

if (!sprinter.init()) {
  fprintf(stdout, "js::DumpBacktrace: OOM\n");
  return;
}

so you don't have to use a static string + do the |result| dance? I know this implies that we wouldn't call OutputDebugStringA in the case of XP_WIN32, but well, that's not supposed to be very likely anyways.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd7c6bc37fb5

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fd7c6bc37fb5