1022369 - Parser bug: regex after var statement and asi

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Peter van der Zee]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140428193838

Steps to reproduce:

```
var identifier
/bar/g
```


Actual results:

Syntax error


Expected results:

Should be fine (though weird).

Regular expressions on new lines don't cause ASI if and only if the first forward slash COULD be intepreted as a division. However, in the case of a var statement that doesn't end with an initializer, it cannot validly become a division (`var x/y`). Therefor, ASI should be applied and `/bar/g` becomes a single regex token.

Basically: `Function("var identifier\n/bar/g")` should not throw.

Comment 1

[André Bargull [:anba]]

The test case asserts with inbound. Cc'ing :arai because this issue is similar to bug 1195578.


js> Function("var identifier\n/bar/g")
Assertion failure: next.type != TOK_DIV && next.type != TOK_REGEXP (next token requires contextual specifier to be parsed unambiguously), at /home/andre/hg/mozilla-inbound/js/src/frontend/TokenStream.h:481

#0  0x00000000004a52c0 in js::frontend::TokenStream::addModifierException (this=0x7fffffffb610, modifierException=js::frontend::Token::OperandIsNone)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/TokenStream.h:480
#1  0x0000000000493f94 in js::frontend::MatchOrInsertSemicolon (ts=..., modifier=js::frontend::Token::None) at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:1474
#2  0x00000000004b0854 in js::frontend::Parser<js::frontend::FullParseHandler>::statement (this=0x7fffffffb5e0, yieldHandling=js::frontend::YieldIsName, canHaveDirectives=true)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:6605
#3  0x00000000004b26fd in js::frontend::Parser<js::frontend::FullParseHandler>::statements (this=0x7fffffffb5e0, yieldHandling=js::frontend::YieldIsName)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:3106
#4  0x00000000004b14ed in js::frontend::Parser<js::frontend::FullParseHandler>::functionBody (this=0x7fffffffb5e0, inHandling=js::frontend::InAllowed, 
    yieldHandling=js::frontend::YieldIsName, kind=js::frontend::Statement, type=js::frontend::Parser<js::frontend::FullParseHandler>::StatementListBody)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:1141
#5  0x0000000000492f89 in js::frontend::Parser<js::frontend::FullParseHandler>::standaloneFunctionBody (this=0x7fffffffb5e0, fun=..., formals=..., generatorKind=js::NotGenerator, 
    inheritedDirectives=..., newDirectives=0x7fffffffaed0, enclosingStaticScope=...) at /home/andre/hg/mozilla-inbound/js/src/frontend/Parser.cpp:971
#6  0x000000000068c999 in BytecodeCompiler::compileFunctionBody (this=0x7fffffffaf60, fun=..., formals=..., generatorKind=js::NotGenerator)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/BytecodeCompiler.cpp:708
#7  0x000000000068dbbb in CompileFunctionBody (cx=0x7ffff6908c00, fun=..., options=..., formals=..., srcBuf=..., enclosingStaticScope=..., generatorKind=js::NotGenerator)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/BytecodeCompiler.cpp:922
#8  0x000000000068dc34 in js::frontend::CompileFunctionBody (cx=0x7ffff6908c00, fun=..., options=..., formals=..., srcBuf=..., enclosingStaticScope=...)
    at /home/andre/hg/mozilla-inbound/js/src/frontend/BytecodeCompiler.cpp:932
#9  0x0000000000dacb0b in FunctionConstructor (cx=0x7ffff6908c00, argc=1, vp=0x7ffff3df70a8, generatorKind=js::NotGenerator) at /home/andre/hg/mozilla-inbound/js/src/jsfun.cpp:1927
#10 0x0000000000dacc59 in js::Function (cx=0x7ffff6908c00, argc=1, vp=0x7ffff3df70a8) at /home/andre/hg/mozilla-inbound/js/src/jsfun.cpp:1935
#11 0x000000000076cde6 in js::CallJSNative (cx=0x7ffff6908c00, native=0xdacc2e <js::Function(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/andre/hg/mozilla-inbound/js/src/jscntxtinlines.h:235
#12 0x00000000007405af in js::Invoke (cx=0x7ffff6908c00, args=..., construct=js::NO_CONSTRUCT) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:763
#13 0x000000000074ffab in Interpret (cx=0x7ffff6908c00, state=...) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:3067
#14 0x000000000074019e in js::RunScript (cx=0x7ffff6908c00, state=...) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:704
#15 0x0000000000741844 in js::ExecuteKernel (cx=0x7ffff6908c00, script=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=js::EXECUTE_GLOBAL, evalInFrame=..., 
    result=0x7fffffffd440) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:978
#16 0x0000000000741b49 in js::Execute (cx=0x7ffff6908c00, script=..., scopeChainArg=..., rval=0x7fffffffd440) at /home/andre/hg/mozilla-inbound/js/src/vm/Interpreter.cpp:1012
#17 0x0000000000d52c6f in ExecuteScript (cx=0x7ffff6908c00, scope=..., script=..., rval=0x7fffffffd440) at /home/andre/hg/mozilla-inbound/js/src/jsapi.cpp:4362
#18 0x0000000000d53002 in JS_ExecuteScript (cx=0x7ffff6908c00, scriptArg=..., rval=...) at /home/andre/hg/mozilla-inbound/js/src/jsapi.cpp:4387
#19 0x000000000043120d in EvalAndPrint (cx=0x7ffff6908c00, bytes=0x7ffff3dfdc80 "Function(\"var identifier\\n/bar/g\")\n", '\344' <repeats 28 times>, <incomplete sequence \344>, 
    length=35, lineno=1, compileOnly=false, out=0x7ffff6f6a740 <_IO_2_1_stdout_>) at /home/andre/hg/mozilla-inbound/js/src/shell/js.cpp:487
#20 0x00000000004316d3 in ReadEvalPrintLoop (cx=0x7ffff6908c00, in=0x7ffff6f69980 <_IO_2_1_stdin_>, out=0x7ffff6f6a740 <_IO_2_1_stdout_>, compileOnly=false)
    at /home/andre/hg/mozilla-inbound/js/src/shell/js.cpp:551
#21 0x0000000000431896 in Process (cx=0x7ffff6908c00, filename=0x0, forceTTY=true) at /home/andre/hg/mozilla-inbound/js/src/shell/js.cpp:582
#22 0x000000000044646a in ProcessArgs (cx=0x7ffff6908c00, op=0x7fffffffd940) at /home/andre/hg/mozilla-inbound/js/src/shell/js.cpp:5801
#23 0x000000000044778c in Shell (cx=0x7ffff6908c00, op=0x7fffffffd940, envp=0x7fffffffdb48) at /home/andre/hg/mozilla-inbound/js/src/shell/js.cpp:6121
#24 0x0000000000448b07 in main (argc=1, argv=0x7fffffffdb38, envp=0x7fffffffdb48) at /home/andre/hg/mozilla-inbound/js/src/shell/js.cpp:6470

Comment 2

[Tooru Fujisawa [:arai]]

try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cca9d65dd07

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Fix modifier used for ASI after do-while.

While adding a test to pass regexp to test_syntax, I hit assertion failure after do-while.
As |do {} while (true) false| is valid syntax, we should get token next to TOK_RP as Operand.  There TOK_DIV shouldn't happen, and only ASI or next statement gets the token, Operand should be fine.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Correctly execute |Function("var x\n/regex/g")| without throwing a SyntaxError.

Added comment and tests for modifier change patch.
I think modifier-regexp-vs-div.js will catch almost cases for ASI.
might be better changing syntax test to automatically check ASI everywhere tho, I'll leave it to another bug for now.

green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cca9d65dd07

Comment 5

[Tooru Fujisawa [:arai]]

Comment on attachment 8660236 [details] [diff] [review]
Part 0: Fix modifier used for ASI after do-while.

Moved to bug 1204368, for clarity.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 8660237 [details] [diff] [review]
Part 1: Correctly execute |Function("var x\n/regex/g")| without throwing a SyntaxError.

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

::: js/src/frontend/Parser.cpp
@@ +4208,5 @@
> +            // should be gotten with Operand.
> +            //
> +            //   var foo
> +            //   /bar/g;
> +            //

With the current phrasing you'd need a "the" before "following", just to note.  But I'm not a big fan of a comment ending in a blank line, just to avoid code running up against code.  So how about this wording:

          // The '=' context after a variable name in a declaration is an
          // opportunity for ASI, and thus for the next token to start an
          // ExpressionStatement:
          //
          //  var foo   // VariableDeclaration
          //  /bar/g;   // ExpressionStatement
          //
          // Therefore we must get the token here as Operand.

::: js/src/jit-test/tests/parser/modifier-regexp-vs-div.js
@@ +5,5 @@
> +  "\n/bar/g; @",
> +];
> +
> +function check_syntax_error(e, code) {
> +  // No need to check exception type

Hm, not assertEq(e instanceof SyntaxError, true), because why not?  I assume the answer is I don't remember/understand the test_syntax scheme well enough, but right now I'm not getting it.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a6d07612688

Comment 8

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

> ::: js/src/jit-test/tests/parser/modifier-regexp-vs-div.js
> @@ +5,5 @@
> > +  "\n/bar/g; @",
> > +];
> > +
> > +function check_syntax_error(e, code) {
> > +  // No need to check exception type
> 
> Hm, not assertEq(e instanceof SyntaxError, true), because why not?  I assume
> the answer is I don't remember/understand the test_syntax scheme well
> enough, but right now I'm not getting it.

It should be better to test that there, added.  It wasn't there because that test is for assertion in TokenStream, and I thought passing it without crash should be sufficient.  Testing it will make it clearer :D

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3a6d07612688