Throw syntax error when a getter/setter has the wrong number of params

Throw syntax error when a getter/setter has the wrong number of params

We used to allow any number of parameters in getters and setters to
match JSC. This is a violation of ES5.1 and both SpiderMonkey and
Chakra throw on these syntax errors.

BUG=v8:3371
LOG=Y
R=marja@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21868

[arv (Not doing code reviews), 2014-06-11 23:28:30]

cleanup

[arv (Not doing code reviews), 2014-06-11 23:32:33]

cleanup

[arv (Not doing code reviews), 2014-06-11 23:35:34]

PTAL

I'm also disabling the tests in Blink and will reenable them once this has
rolled.

See https://code.google.com/p/chromium/issues/detail?id=383596

[marja, 2014-06-12 07:25:14]

Code LGTM. Looks like a reasonable thing to do. There might be some surprising
breakage though, like, some scripts might have the wrong get/set parameter count
inside lazy functions which were never executed. But I think this is worth
trying out nevertheless.

[marja, 2014-06-12 07:25:16]

Code LGTM. Looks like a reasonable thing to do. There might be some surprising
breakage though, like, some scripts might have the wrong get/set parameter count
inside lazy functions which were never executed. But I think this is worth
trying out nevertheless.

[arv (Not doing code reviews), 2014-06-12 17:40:04]

On 2014/06/12 at 07:25:16, marja wrote:
> Code LGTM. Looks like a reasonable thing to do. There might be some surprising
breakage though, like, some scripts might have the wrong get/set parameter count
inside lazy functions which were never executed. But I think this is worth
trying out nevertheless.

Thanks. Can you land this for me?

The following throws an error as expected:

```js
var s = 'return /*' + Array(1e6).join(' ') + '*/ {get x(error) {}};';
Function(s);
```

Is this a valid way to test the preparser? (I got the idea from Andy's CL at
https://codereview.chromium.org/332443002)

If not, then I need to investigate more time on how we do early syntax errors
for strict functions that have duplicate property names in object literals.

[marja, 2014-06-13 14:05:51]

On 2014/06/12 17:40:04, arv wrote:
> On 2014/06/12 at 07:25:16, marja wrote:
> > Code LGTM. Looks like a reasonable thing to do. There might be some
surprising
> breakage though, like, some scripts might have the wrong get/set parameter
count
> inside lazy functions which were never executed. But I think this is worth
> trying out nevertheless.
> 
> Thanks. Can you land this for me?
> 
> The following throws an error as expected:
> 
> ```js
> var s = 'return /*' + Array(1e6).join(' ') + '*/ {get x(error) {}};';
> Function(s);
> ```
> 
> Is this a valid way to test the preparser? (I got the idea from Andy's CL at
> https://codereview.chromium.org/332443002)
> 
> If not, then I need to investigate more time on how we do early syntax errors
> for strict functions that have duplicate property names in object literals.

No, afaics that doesn't invoke the PreParser.

PreParser is only invoked for parsing inside lazy functions, i.e.,

function top_level_func() {
  // this is parsed with PreParser
  ...
}

under a bunch of conditions (laziness needs to be allowed, the code needs to be
long enough). Especially, if the function is in parenthesis, it's not lazy.

With your code snippet, we end up parsing this:
(function() { return /*  ...  */ { get x(error){}}; })

(You can verify this by putting source->Print(stderr); inside
Parser::ParseProgram().)

So it's not lazy, since the function is in parenthesis. (The relevant thing to
check is is_lazily_parsed inside ParseFunctionLiteral.)

However, test-parsing.cc parses the same code with Parser and PreParser and
tests that they produce the same errors, so I think this CL is good.

Hmm, duplicate property names in object literals in PreParser? Isn't that taken
care by ObjectLiteralChecker already?

[arv (Not doing code reviews), 2014-06-13 14:24:24]

On 2014/06/13 at 14:05:51, marja wrote:
> On 2014/06/12 17:40:04, arv wrote:
> > On 2014/06/12 at 07:25:16, marja wrote:
> > > Code LGTM. Looks like a reasonable thing to do. There might be some
surprising
> > breakage though, like, some scripts might have the wrong get/set parameter
count
> > inside lazy functions which were never executed. But I think this is worth
> > trying out nevertheless.
> > 
> > Thanks. Can you land this for me?
> > 
> > The following throws an error as expected:
> > 
> > ```js
> > var s = 'return /*' + Array(1e6).join(' ') + '*/ {get x(error) {}};';
> > Function(s);
> > ```
> > 
> > Is this a valid way to test the preparser? (I got the idea from Andy's CL at
> > https://codereview.chromium.org/332443002)
> > 
> > If not, then I need to investigate more time on how we do early syntax
errors
> > for strict functions that have duplicate property names in object literals.
> 
> No, afaics that doesn't invoke the PreParser.
> 
> PreParser is only invoked for parsing inside lazy functions, i.e.,
> 
> function top_level_func() {
>   // this is parsed with PreParser
>   ...
> }
> 
> under a bunch of conditions (laziness needs to be allowed, the code needs to
be long enough). Especially, if the function is in parenthesis, it's not lazy.
> 
> With your code snippet, we end up parsing this:
> (function() { return /*  ...  */ { get x(error){}}; })
> 
> (You can verify this by putting source->Print(stderr); inside
Parser::ParseProgram().)
> 
> So it's not lazy, since the function is in parenthesis. (The relevant thing to
check is is_lazily_parsed inside ParseFunctionLiteral.)
> 
> However, test-parsing.cc parses the same code with Parser and PreParser and
tests that they produce the same errors, so I think this CL is good.

Thanks for explaining this. It is good to know that test-parsing.cc tests both.

> Hmm, duplicate property names in object literals in PreParser? Isn't that
taken care by ObjectLiteralChecker already?

My point was that I would have to look closer, looking at how the
ObjectLiteralChecker is invoked.

[marja, 2014-06-13 14:55:11]

Oh and I'll land this CL next week, since it's Friday after-work-a-clock here.

[marja, 2014-06-16 07:37:57]

Hmm, I ran make quickcheck for this CL, but it fails...

The failing test is mozilla/data/js1_5/extensions/regress-367501-01.js. It does
this:

  try
  { 
    expect = 'undefined';
    var a = { set x() {} };
    actual = a.x + '';
  }
  catch(ex)
  {
  }

The corresponding bug is https://bugzila.mozilla.org/show_bug.cgi?id=367501
which, afaics, is about doing the wrong thing when code like this is ran (like,
writing to / reading from a wrong location in memory).

I guess it's safe to disable this test in V8?

[arv (Not doing code reviews), 2014-06-16 13:55:09]

On 2014/06/16 at 07:37:57, marja wrote:
> Hmm, I ran make quickcheck for this CL, but it fails...
> 
> The failing test is mozilla/data/js1_5/extensions/regress-367501-01.js. It
does this:
> 
>   try
>   { 
>     expect = 'undefined';
>     var a = { set x() {} };
>     actual = a.x + '';
>   }
>   catch(ex)
>   {
>   }
> 
> The corresponding bug is https://bugzila.mozilla.org/show_bug.cgi?id=367501
which, afaics, is about doing the wrong thing when code like this is ran (like,
writing to / reading from a wrong location in memory).
> 
> I guess it's safe to disable this test in V8?

SpiderMonkey also throws a syntax error for this and no code is run.

I think we should just remove these tests
(mozilla/data/js1_5/extensions/regress-367501-*.js)

[marja, 2014-06-16 14:43:15]

I discussed with rossberg@ and he says that disabling the test on the V8 side is
the way to go. (At some point V8 should probably pull a more fresh Mozilla test
suite..)

[arv (Not doing code reviews), 2014-06-16 14:58:00]

On 2014/06/16 at 14:43:15, marja wrote:
> I discussed with rossberg@ and he says that disabling the test on the V8 side
is the way to go. (At some point V8 should probably pull a more fresh Mozilla
test suite..)

Makes sense... I'm not having much luck finding where to add these exceptions
though. Any pointers?

Thanks.

[rossberg, 2014-06-16 15:03:27]

On 2014/06/16 14:58:00, arv wrote:
> On 2014/06/16 at 14:43:15, marja wrote:
> > I discussed with rossberg@ and he says that disabling the test on the V8
side
> is the way to go. (At some point V8 should probably pull a more fresh Mozilla
> test suite..)
> 
> Makes sense... I'm not having much luck finding where to add these exceptions
> though. Any pointers?
> 
> Thanks.

You have to add a suitable line to test/mozilla/mozilla.status

[arv (Not doing code reviews), 2014-06-16 15:35:11]

Add test exception

[arv (Not doing code reviews), 2014-06-16 15:35:50]

On 2014/06/16 at 15:03:27, rossberg wrote:
> On 2014/06/16 14:58:00, arv wrote:
> > On 2014/06/16 at 14:43:15, marja wrote:
> > > I discussed with rossberg@ and he says that disabling the test on the V8
side
> > is the way to go. (At some point V8 should probably pull a more fresh
Mozilla
> > test suite..)
> > 
> > Makes sense... I'm not having much luck finding where to add these
exceptions
> > though. Any pointers?
> > 
> > Thanks.
> 
> You have to add a suitable line to test/mozilla/mozilla.status

Thanks.

PTAL

[marja, 2014-06-16 16:00:01]

still lgtm; will land this now

[marja, 2014-06-16 16:00:26]

On 2014/06/16 16:00:01, marja wrote:
> still lgtm; will land this now

Argh, oh wait, the tree is throttled and the sheriff is not around :( I take
that back.

[marja, 2014-06-17 07:23:41]

Committed patchset #4 manually as r21868 (tree was closed).

[ulan, 2014-06-17 16:10:37]

On 2014/06/17 07:23:41, marja wrote:
> Committed patchset #4 manually as r21868 (tree was closed).

Today's roll with this CL breaks inspector console tests:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...

arv@, should this tests be disabled?

[arv (Not doing code reviews), 2014-06-17 16:20:01]

On 2014/06/17 16:10:37, ulan wrote:
> On 2014/06/17 07:23:41, marja wrote:
> > Committed patchset #4 manually as r21868 (tree was closed).
> 
> Today's roll with this CL breaks inspector console tests:
>
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40...
> 
> arv@, should this tests be disabled?

These tests have been added to TestExpectations already:

https://src.chromium.org/viewvc/blink?revision=176055&view=revision

I'm confused why they are showing up.

[arv (Not doing code reviews), 2014-06-17 16:26:33]

On 2014/06/17 16:20:01, arv wrote:

> I'm confused why they are showing up.

Some of the tests timeout, not fail.

https://codereview.chromium.org/334393005/

[arv (Not doing code reviews), 2014-06-17 16:32:07]

On 2014/06/17 16:26:33, arv wrote:
> On 2014/06/17 16:20:01, arv wrote:
> 
> > I'm confused why they are showing up.
> 
> Some of the tests timeout, not fail.
> 
> https://codereview.chromium.org/334393005/

Landed test expectation change. Sorry for the interuptions.