Parser: Fix crash on stack overflow when lazy-parsing arrow functions

Parser: Fix crash on stack overflow when lazy-parsing arrow functions

The problem manifests itself when parsing manages to return something
meaningful in the presence of a stack overflow. This happens because
calling ParserBase::Next() will still return one valid token on stack
overflow, before starting to return invalid tokens.

Take the following input as example:

        a.map(v => v + 1);
              | |
       already   next token
        parsed   (which will be an invalid token
  (identifier)   because of a stack overflow)

The "v" may have been already parsed into a VariableProxy, then if a
stack overflow occurs, next token will be an invalid token (instead
of Token::ARROW), but the parser will return the VariableProxy.

This always happens when lazy-parsing arrow functions, so the position
in the input stream where the the arrow function code ends is known.
This fix adds a check that ensures that parsing ended at the end
position of the arrow function.

BUG=465671
LOG=N

Committed: https://crrev.com/3c3ce1bca831c03feab9c8fbfe95edd3c0e0447e
Cr-Commit-Position: refs/heads/master@{#27325}

[aperez, 2015-03-18 22:11:26]

aperez@igalia.com changed reviewers:
+ marja@chromium.org, mstarzinger@chromium.org, svenpanne@chromium.org,
wingo@igalia.com

[wingo, 2015-03-19 07:48:45]

Non-OWNER LGTM.  Please add a test for the parenthesized version as well -- i.e.
(x)=>x+1 in addition to x=x+1.

Is it possible that this strange situation arises in eager parsing?  Or in eager
parsing would we always detect the Token::ILLEGAL and bail?

https://codereview.chromium.org/1023483003/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1023483003/diff/1/src/parser.cc#newcode1154
src/parser.cc:1154: DCHECK(expression->IsFunctionLiteral());
One thing I didn't get when first reading this code is that this DCHECK must be
true because the function was already pre-parsed, so we know what to expect. 
Perhaps a one-line comment here?

[marja, 2015-03-19 11:00:26]

https://codereview.chromium.org/1023483003/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1023483003/diff/1/src/parser.cc#newcode1149
src/parser.cc:1149: // Scanning must end at the same position that was recorded
Hmm, this seems very indirect. Can't we know in a more direct way whether a
stack overflow has happened?

And why do we have ok == true in case of the stack overflow? Isn't that weird?
So we manage to parse a partial body successfully, but then... there should be
some part which reads the invalid token produced by the stack overflow and sets
ok to false. What's up with that?

[aperez, 2015-03-19 12:23:07]

On 2015/03/19 11:00:26, marja wrote:
> https://codereview.chromium.org/1023483003/diff/1/src/parser.cc
> File src/parser.cc (right):
> 
> https://codereview.chromium.org/1023483003/diff/1/src/parser.cc#newcode1149
> src/parser.cc:1149: // Scanning must end at the same position that was
recorded
> Hmm, this seems very indirect. Can't we know in a more direct way whether a
> stack overflow has happened?

That would be checking for Token::ILLEGAL and/or “stack_overflow_” in more
places
of the parser, or coming up with some systematic way handling those situations
(more on this below).

The good thing of checking that parsing end where it's suppossed to be is that
it will catch all stack overflow issues, either existing or future, despite how
the parser behaves, and regardless of which intermediate valid parse is done
(because we always know that the end of the input *must* be reached).
 
> And why do we have ok == true in case of the stack overflow? Isn't that weird?
> So we manage to parse a partial body successfully, but then... there should be
> some part which reads the invalid token produced by the stack overflow and
sets
> ok to false. What's up with that?

The code which first detects the stack overflow is in the scanner, which does
not have access to the “ok” flag. The parser itself checks for stack overflow
in some places, but it looks to me that most of those places were added to
cover some specific test cases that have been found to make the parser crash,
and the whole handling of stack overflow doesn't seem very systematic to me...

So, the idea of this patch is to solve the particular case in which arrow
functions with a concise body end up being parsed as a valid expression
before the end of the input is reached, and get this crasher bug solved.
Ideally later the handling of stack overflows should be rethought in such
a way that they are always detected independently of how the parser works
(TBH, using Token::ILLEGAL to signal stack overflows looks like a kludge to
me, provided that there is a “stack_overflow_” flag already...)

[aperez, 2015-03-19 12:42:30]

The patch only needs to cover the case of lazy parsing.

I have checked eager parsing with a similar test case using
“eval('v => v + 1')” in place of the arrow function. It works
fine in the current Git HEAD, without needing the fix.

https://codereview.chromium.org/1023483003/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1023483003/diff/1/src/parser.cc#newcode1154
src/parser.cc:1154: DCHECK(expression->IsFunctionLiteral());
On 2015/03/19 07:48:45, wingo wrote:
> One thing I didn't get when first reading this code is that this DCHECK must
be
> true because the function was already pre-parsed, so we know what to expect. 
> Perhaps a one-line comment here?

Acknowledged.

[wingo, 2015-03-19 13:21:40]

Still non-owner LGTM with a nit and a question

On 2015/03/19 12:42:30, aperez wrote:
> I have checked eager parsing with a similar test case using
> “eval('v => v + 1')” in place of the arrow function. It works
> fine in the current Git HEAD, without needing the fix.

Can you confirm that you ran this test with lazy parsing disabled?  Thanks :)

https://codereview.chromium.org/1023483003/diff/1/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1023483003/diff/1/src/parser.cc#newcode1149
src/parser.cc:1149: // Scanning must end at the same position that was recorded
On 2015/03/19 11:00:26, marja wrote:
> And why do we have ok == true in case of the stack overflow? Isn't that weird?

I thought so too but to expand on what Adri found:

  - You go to parse an Assignment expression
  - You get the VariableProxy
  - You start to look ahead to see if the next token indicates a compound
expression, like = or + or something
  - The peeked token Token::ILLEGAL doesn't match any of the tokens that can
indicate a compoint expression
  - You return the VariableProxy instead, a valid expression, OK=true.

If this were a function(){} then we'd get an error expecting to consume the
right-bracket.  Since it's an arrow function, depending on where we are in
parsing it can succeed or not, as described in the CL message.

Agreed that it is very weird though!

https://codereview.chromium.org/1023483003/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1023483003/diff/20001/src/parser.cc#newcode1154
src/parser.cc:1154: // The input is known to be a function that is being
lazy-parsed.
lazy-parsed is not great terminology IMO.  How about

// The pre-parser saw an arrow function here, so the full parser must produce a
FunctionLiteral.

[marja, 2015-03-19 13:44:57]

On 2015/03/19 13:21:40, wingo wrote:
> I thought so too but to expand on what Adri found:
> 
>   - You go to parse an Assignment expression
>   - You get the VariableProxy
>   - You start to look ahead to see if the next token indicates a compound
> expression, like = or + or something
>   - The peeked token Token::ILLEGAL doesn't match any of the tokens that can
> indicate a compoint expression
>   - You return the VariableProxy instead, a valid expression, OK=true.

Isn't this a bug? :)

(I'm OOO this week, I can have a closer look next week, unless somebody
currently @ work has time for this.)

[marja, 2015-03-19 14:01:50]

Hmm, and is the reason why this doesn't occur for non-array functions that we
still get to expect "}" and thus don't parse partial expressions? (Just a guess.
Maybe it's not that.)

[wingo, 2015-03-19 14:14:15]

On 2015/03/19 14:01:50, marja wrote:
> Hmm, and is the reason why this doesn't occur for non-array functions that we
> still get to expect "}" and thus don't parse partial expressions? (Just a
guess.
> Maybe it's not that.)

Exactly.  The other case when we're parsing a script we check for EOS.  The
problem is that the arrow function doesn't have a delimiter like that.  This
patch uses the pre-parsed end position as the delimiter instead.

[marja, 2015-03-19 14:15:48]

Ah okay, then this might actually be the right thing, and the thing above is
*not* a bug after all.

I'll review later or somebody else can review instead...

[marja, 2015-03-19 14:19:18]

Nah, I reviewed it after all, lgtm, but pls add a comment along the lines "this
is a problem only for arrow functions w/ single statement bodies, since there is
no end token such as "}" for normal functions".

[aperez, 2015-03-19 23:55:32]

On 2015/03/19 14:19:18, marja wrote:
> Nah, I reviewed it after all, lgtm, but pls add a comment along the lines
"this
> is a problem only for arrow functions w/ single statement bodies, since there
is
> no end token such as "}" for normal functions".

Comment added.

And thanks for reviewing even when you were OOO :)

[aperez, 2015-03-19 23:57:25]

On 2015/03/19 13:21:40, wingo wrote:
> Still non-owner LGTM with a nit and a question
> 
> On 2015/03/19 12:42:30, aperez wrote:
> > I have checked eager parsing with a similar test case using
> > “eval('v => v + 1')” in place of the arrow function. It works
> > fine in the current Git HEAD, without needing the fix.
> 
> Can you confirm that you ran this test with lazy parsing disabled?  Thanks :)

Yes, checked as well, HEAD still working fine for that particular case
when “--no-lazy” is added to the d8 command line :)

[aperez, 2015-03-19 23:57:39]

https://codereview.chromium.org/1023483003/diff/20001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1023483003/diff/20001/src/parser.cc#newcode1154
src/parser.cc:1154: // The input is known to be a function that is being
lazy-parsed.
On 2015/03/19 13:21:40, wingo wrote:
> lazy-parsed is not great terminology IMO.  How about
> 
> // The pre-parser saw an arrow function here, so the full parser must produce
a
> FunctionLiteral.

Acknowledged.

[aperez, 2015-03-19 23:57:45]

The CQ bit was checked by aperez@igalia.com

[aperez, 2015-03-19 23:57:46]

The patchset sent to the CQ was uploaded after l-g-t-m from wingo@igalia.com,
marja@chromium.org
Link to the patchset: https://codereview.chromium.org/1023483003/#ps40001
(title: "Fix nits as per review comments")

[commit-bot: I haz the power, 2015-03-19 23:58:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1023483003/40001

[commit-bot: I haz the power, 2015-03-20 00:17:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-20 00:18:01]

Patchset 3 (id:??) landed as
https://crrev.com/3c3ce1bca831c03feab9c8fbfe95edd3c0e0447e
Cr-Commit-Position: refs/heads/master@{#27325}

[rossberg, 2015-03-20 10:03:48]

On 2015/03/20 00:18:01, I haz the power (commit-bot) wrote:
> Patchset 3 (id:??) landed as
> https://crrev.com/3c3ce1bca831c03feab9c8fbfe95edd3c0e0447e
> Cr-Commit-Position: refs/heads/master@{#27325}

Sorry I'm late, but can you please create a follow-up CL that moves the
regression tests to the harmony/regress/ dir?

[rossberg, 2015-03-20 10:30:16]

On 2015/03/20 10:03:48, rossberg wrote:
> On 2015/03/20 00:18:01, I haz the power (commit-bot) wrote:
> > Patchset 3 (id:??) landed as
> > https://crrev.com/3c3ce1bca831c03feab9c8fbfe95edd3c0e0447e
> > Cr-Commit-Position: refs/heads/master@{#27325}
> 
> Sorry I'm late, but can you please create a follow-up CL that moves the
> regression tests to the harmony/regress/ dir?

NM, already did it.

[aperez, 2015-03-20 18:11:15]

On 2015/03/20 10:30:16, rossberg wrote:
> On 2015/03/20 10:03:48, rossberg wrote:
> > On 2015/03/20 00:18:01, I haz the power (commit-bot) wrote:
> > > Patchset 3 (id:??) landed as
> > > https://crrev.com/3c3ce1bca831c03feab9c8fbfe95edd3c0e0447e
> > > Cr-Commit-Position: refs/heads/master@{#27325}
> > 
> > Sorry I'm late, but can you please create a follow-up CL that moves the
> > regression tests to the harmony/regress/ dir?
> 
> NM, already did it.

Oh, I just saw this now. Thanks for taking care of moving them.

[marja, 2015-03-23 10:56:32]

https://codereview.chromium.org/1023483003/diff/40001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/1023483003/diff/40001/src/parser.cc#newcode1154
src/parser.cc:1154: // bodies, since there is no end token suck as "}" for
normal
... unintentional swearing? :)

[aperez, 2015-03-23 11:27:53]

On 2015/03/23 10:56:32, marja wrote:
> https://codereview.chromium.org/1023483003/diff/40001/src/parser.cc
> File src/parser.cc (right):
> 
>
https://codereview.chromium.org/1023483003/diff/40001/src/parser.cc#newcode1154
> src/parser.cc:1154: // bodies, since there is no end token suck as "}" for
> normal
> ... unintentional swearing? :)

VERY unintentional, indeed. Fix here:
https://codereview.chromium.org/1022413003