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

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: decoder, Assigned: shu)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla55
x86_64
Linux
assertion, jsbugmon, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 unaffected, firefox54 unaffected, firefox55 fixed, firefox-esr52 unaffected)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
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.
(Assignee)

Comment 1

2 months ago
Created attachment 8859366 [details] [diff] [review]
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.
Attachment #8859366 - Flags: review?(dteller)

Updated

2 months ago
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
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+
(Assignee)

Comment 3

2 months ago
(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

2 months ago
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)

Comment 5

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a2c9cf2db1e4
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee: nobody → shu
Blocks: 1216630
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.