1357506 - Assertion failure: !constructorBox_, at js/src/frontend/Parser.h:106

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision bb38d935d699 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

class a {
  constructor() {
    "use asm";
  }
}



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x00000000004a5e7d in js::frontend::ParseContext::ClassStatement::setConstructorBox (this=<optimized out>, funbox=0x7ffff69fe2c0) at js/src/frontend/Parser.h:106
#0  0x00000000004a5e7d in js::frontend::ParseContext::ClassStatement::setConstructorBox (this=<optimized out>, funbox=0x7ffff69fe2c0) at js/src/frontend/Parser.h:106
#1  js::frontend::FunctionBox::initWithEnclosingParseContext (this=this@entry=0x7ffff69fe2c0, enclosing=enclosing@entry=0x7fffffffc220, kind=kind@entry=js::frontend::ClassConstructor) at js/src/frontend/Parser.cpp:551
#2  0x00000000004d4f14 in js::frontend::Parser<js::frontend::FullParseHandler>::innerFunction (this=this@entry=0x7fffffffcd70, pn=pn@entry=0x7ffff69fe0b0, outerpc=0x7fffffffc220, fun=..., fun@entry=..., toStringStart=toStringStart@entry=0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=js::frontend::YieldIsName, kind=js::frontend::ClassConstructor, generatorKind=js::NotGenerator, asyncKind=js::SyncFunction, tryAnnexB=false, inheritedDirectives=..., newDirectives=0x7fffffffbcf0) at js/src/frontend/Parser.cpp:3539
#3  0x00000000004adfe5 in js::frontend::Parser<js::frontend::FullParseHandler>::trySyntaxParseInnerFunction (this=this@entry=0x7fffffffcd70, pn=pn@entry=0x7ffff69fe0b0, fun=..., toStringStart=toStringStart@entry=0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, kind=js::frontend::ClassConstructor, generatorKind=js::NotGenerator, asyncKind=js::SyncFunction, tryAnnexB=false, inheritedDirectives=..., newDirectives=0x7fffffffbcf0) at js/src/frontend/Parser.cpp:3475
#4  0x00000000004d5243 in js::frontend::Parser<js::frontend::FullParseHandler>::functionDefinition (this=this@entry=0x7fffffffcd70, pn=0x7ffff69fe0b0, toStringStart=toStringStart@entry=0, inHandling=inHandling@entry=js::frontend::InAllowed, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, funName=..., funName@entry=..., kind=js::frontend::ClassConstructor, generatorKind=js::NotGenerator, asyncKind=js::SyncFunction, tryAnnexB=false) at js/src/frontend/Parser.cpp:3376
#5  0x00000000004d5f4f in js::frontend::Parser<js::frontend::FullParseHandler>::methodDefinition (this=this@entry=0x7fffffffcd70, toStringStart=0, propType=<optimized out>, funName=funName@entry=...) at js/src/frontend/Parser.cpp:9819
#6  0x00000000004ddd98 in js::frontend::Parser<js::frontend::FullParseHandler>::classDefinition (this=this@entry=0x7fffffffcd70, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, classContext=classContext@entry=js::frontend::Parser<js::frontend::FullParseHandler>::ClassStatement, defaultHandling=defaultHandling@entry=js::frontend::NameRequired) at js/src/frontend/Parser.cpp:7201
#7  0x00000000004e29e7 in js::frontend::Parser<js::frontend::FullParseHandler>::statementListItem (this=this@entry=0x7fffffffcd70, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName, canHaveDirectives=<optimized out>) at js/src/frontend/Parser.cpp:7686
#8  0x00000000004e2ee3 in js::frontend::Parser<js::frontend::FullParseHandler>::statementList (this=this@entry=0x7fffffffcd70, yieldHandling=yieldHandling@entry=js::frontend::YieldIsName) at js/src/frontend/Parser.cpp:4092
#9  0x00000000004af39a in js::frontend::Parser<js::frontend::FullParseHandler>::globalBody (this=this@entry=0x7fffffffcd70, globalsc=globalsc@entry=0x7fffffffc770) at js/src/frontend/Parser.cpp:2192
#10 0x0000000000aa8b3f in BytecodeCompiler::compileScript (this=this@entry=0x7fffffffc7c0, environment=..., environment@entry=..., sc=sc@entry=0x7fffffffc770) at js/src/frontend/BytecodeCompiler.cpp:385
#11 0x0000000000aa94b7 in BytecodeCompiler::compileGlobalScript (scopeKind=<optimized out>, this=0x7fffffffc7c0) at js/src/frontend/BytecodeCompiler.cpp:431
#12 js::frontend::CompileGlobalScript (cx=cx@entry=0x7ffff694c000, alloc=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, options=..., srcBuf=..., sourceObjectOut=sourceObjectOut@entry=0x0, sourceCompressionTaskOut=0x0) at js/src/frontend/BytecodeCompiler.cpp:638
#13 0x0000000000945954 in Compile (cx=cx@entry=0x7ffff694c000, options=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, srcBuf=..., script=...) at js/src/jsapi.cpp:4006
#14 0x0000000000945ada in Compile (script=..., length=<optimized out>, chars=0x7ffff43f1b30 u"class a {\n  constructor() {\n    \"use asm\";\n  }\n}\n", scopeKind=js::ScopeKind::Global, options=..., cx=0x7ffff694c000) at js/src/jsapi.cpp:4015
#15 Compile (cx=cx@entry=0x7ffff694c000, options=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, bytes=<optimized out>, length=49, script=..., script@entry=...) at js/src/jsapi.cpp:4030
#16 0x000000000094f5ac in Compile (cx=cx@entry=0x7ffff694c000, options=..., scopeKind=scopeKind@entry=js::ScopeKind::Global, fp=fp@entry=0x7ffff4327c00, script=...) at js/src/jsapi.cpp:4041
#17 0x000000000094f7f5 in JS::Compile (cx=cx@entry=0x7ffff694c000, options=..., file=file@entry=0x7ffff4327c00, script=..., script@entry=...) at js/src/jsapi.cpp:4081
[...]
#22 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8685


Marking fuzzblocker because the test is so simple that it gets hit very frequently.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: Remove assert that constructorBox can only be set once when parsing classes.

Both asm.js and syntax parsing can abort and rewind parsing of an inner
function. The bookkeeping to make sure that a class's constructor
FunctionBox is only set once is not worth it -- duplicate constructor
definitions already throw an early error.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8859366 [details] [diff] [review]
Remove assert that constructorBox can only be set once when parsing classes.

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

::: js/src/frontend/Parser.cpp
@@ -3451,5 @@
>                  // rewound to just before we tried the syntax parse for
>                  // correctness.
>                  parser->clearAbortedSyntaxParse();
>                  usedNames.rewind(token);
> -                funbox->resetForAbortedSyntaxParse(pc, kind);

Shouldn't you still set funbox->constructorBox_ to nullptr?

::: js/src/jit-test/tests/class/bug1357506.js
@@ +1,1 @@
> +class a {

Nit: I personally prefer when the test contains comments which explain what it tests.

Comment 3

[Shu-yu Guo [:shu]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #2)
> Comment on attachment 8859366 [details] [diff] [review]
> Remove assert that constructorBox can only be set once when parsing classes.
> 
> Review of attachment 8859366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/Parser.cpp
> @@ -3451,5 @@
> >                  // rewound to just before we tried the syntax parse for
> >                  // correctness.
> >                  parser->clearAbortedSyntaxParse();
> >                  usedNames.rewind(token);
> > -                funbox->resetForAbortedSyntaxParse(pc, kind);
> 
> Shouldn't you still set funbox->constructorBox_ to nullptr?

We could, but the resetting won't be done for asm.js either. Point of the patch was to just overwrite it on subsequent reparses, on both syntax and asm.js aborts.

> 
> ::: js/src/jit-test/tests/class/bug1357506.js
> @@ +1,1 @@
> > +class a {
> 
> Nit: I personally prefer when the test contains comments which explain what
> it tests.

Sure, will comment.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c9cf2db1e4
Remove assert that constructorBox can only be set once when parsing classes. (r=Yoric)

Comment 5

[Carsten Book [:Tomcat]]

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