Closed Bug 1357506 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: decoder, Assigned: shu)

References

Details

(4 keywords, Whiteboard: [jsbugmon:update,bisect][fuzzblocker])

Attachments

(1 file)

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.
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.
Attachment #8859366 - Flags: review?(dteller)
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.
Attachment #8859366 - Flags: review?(dteller) → review+
(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.
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c9cf2db1e4
Remove assert that constructorBox can only be set once when parsing classes. (r=Yoric)
https://hg.mozilla.org/mozilla-central/rev/a2c9cf2db1e4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee: nobody → shu
Blocks: 1216630
You need to log in before you can comment on or make changes to this bug.