[proxy] fixing harmony/proxy.js tests and improving error messages + some drive-by fixes

[proxy] fixing for-in for proxies, fixing harmony/proxy.js tests, improving error messages and some drive-by fixes

- Enable old proxy tests. Many tests had to be updated. Proxies are instantiated differently and there is no distinction between normal proxies and callable function proxies anymore. Most additional changes are related to the getOwnPropertyDescriptor trap which is not accessed as often anymore for new proxies.
- For-in still used the old-style proxy semantics which skip the [[Has]] check for each loop-iteration. This CL uses the slower but correct runtime fallback for proxies.

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)

Committed: https://crrev.com/df2a92972b0a087080e67496177e879e9409d5b0
Cr-Commit-Position: refs/heads/master@{#32801}

[Camillo Bruni, 2015-12-10 14:53:43]

Description was changed from

==========
\# Enter a description of the change.
[proxy] fixing harmoing/proxy.js tests and improving error messages + some
fly-by fixes

BUG=v8:1543

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)
==========

to

==========
[proxy] fixing harmoing/proxy.js tests and improving error messages + some
fly-by fixes

BUG=v8:1543

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)
==========

[Camillo Bruni, 2015-12-10 14:57:06]

cbruni@chromium.org changed reviewers:
+ jkummerow@chromium.org, neis@chromium.org

[Camillo Bruni, 2015-12-10 14:57:07]

PTAL
neis: I would like to land incrementally, specially since I had to do some
fly-by fixes for some minor bugs that appeared.

proxy.js is quite ok now, I commented out a section with enumerate tests which
we will have to enable later-on.

[neis, 2015-12-10 15:13:50]

https://codereview.chromium.org/1516843002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1516843002/diff/1/src/objects.cc#newcode6841
src/objects.cc:6841: }
(!trap_result_obj->BooleanValue() means that the boolean value of the trap
result is false.  The trap itself is not expected to return a boolean, we always
convert it.

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/es6/regress/re...
File test/mjsunit/es6/regress/regress-cr493566.js (left):

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/es6/regress/re...
test/mjsunit/es6/regress/regress-cr493566.js:55: 
Not sure why I just deleted this.  Could you try to enable it?

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/harmony/proxie...
File test/mjsunit/harmony/proxies-with.js (right):

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-with.js:38: // }, x, y, z)
This can be done now :)

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/mjsunit.status...
test/mjsunit/mjsunit.status:94: 'harmony/proxies-with': [SKIP],
Can't proxies-with be enabled?

[caitp (gmail), 2015-12-10 15:15:25]

https://codereview.chromium.org/1516843002/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1516843002/diff/1/src/builtins.cc#newcode1803
src/builtins.cc:1803: if (args.length() < 2) {
Perhaps it makes sense to implement this "if index < count, return value at
index, otherwise return undefined" pattern in the Arguments class, since it's
reused again here, and might be useful in other places.

[Jakob Kummerow, 2015-12-10 15:35:04]

https://codereview.chromium.org/1516843002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1516843002/diff/1/src/objects.cc#newcode4727
src/objects.cc:4727: NewTypeError(MessageTemplate::kProxyTrapReturnedNonBoolean,
Nope, the problem is that it returned a falsish value when it should have
returned a truish value.

https://codereview.chromium.org/1516843002/diff/1/src/objects.cc#newcode4785
src/objects.cc:4785: NewTypeError(MessageTemplate::kProxyTrapReturnedNonBoolean,
same here

https://codereview.chromium.org/1516843002/diff/1/src/objects.cc#newcode7348
src/objects.cc:7348: NewTypeError(MessageTemplate::kProxyTrapReturnedNonBoolean,
same here

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/for-in-opt.js
File test/mjsunit/for-in-opt.js (left):

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/for-in-opt.js#...
test/mjsunit/for-in-opt.js:18: assertEquals(["0"], f("a"));
What's wrong with this? (I mean the entire file.)

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/harmony/proxie...
File test/mjsunit/harmony/proxies-with.js (right):

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/harmony/proxie...
test/mjsunit/harmony/proxies-with.js:35: // TODO(cbruni): enable once we have
[[Call]] working.
Don't we have [[Call]] working?

[Camillo Bruni, 2015-12-10 15:35:32]

Description was changed from

==========
[proxy] fixing harmoing/proxy.js tests and improving error messages + some
fly-by fixes

BUG=v8:1543

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)
==========

to

==========
[proxy] fixing harmony/proxy.js tests and improving error messages + some fly-by
fixes

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)
==========

[Camillo Bruni, 2015-12-10 15:36:07]

Description was changed from

==========
[proxy] fixing harmony/proxy.js tests and improving error messages + some fly-by
fixes

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)
==========

to

==========
[proxy] fixing harmony/proxy.js tests and improving error messages + some
drive-by fixes

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)
==========

[neis, 2015-12-10 16:08:13]

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/for-in-opt.js
File test/mjsunit/for-in-opt.js (left):

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/for-in-opt.js#...
test/mjsunit/for-in-opt.js:18: assertEquals(["0"], f("a"));
On 2015/12/10 15:35:03, Jakob wrote:
> What's wrong with this? (I mean the entire file.)

I tried to figure out what it was testing (in order to meaningfully convert the
test) but gave up.

[Camillo Bruni, 2015-12-10 20:19:55]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-10 20:20:08]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516843002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516843002/100001

[Camillo Bruni, 2015-12-10 20:20:39]

Description was changed from

==========
[proxy] fixing harmony/proxy.js tests and improving error messages + some
drive-by fixes

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)
==========

to

==========
[proxy] fixing for-in for proxies, fixing harmony/proxy.js tests, improving
error messages and some drive-by fixes

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)
==========

[Camillo Bruni, 2015-12-10 20:22:14]

PTAL
There was a minor hiccup with for-in and proxies which I fixed, now we can run
most tests.

https://codereview.chromium.org/1516843002/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1516843002/diff/1/src/builtins.cc#newcode1803
src/builtins.cc:1803: if (args.length() < 2) {
On 2015/12/10 at 15:15:25, caitp wrote:
> Perhaps it makes sense to implement this "if index < count, return value at
index, otherwise return undefined" pattern in the Arguments class, since it's
reused again here, and might be useful in other places.

right, I'll put it on my Friday-cleanup list :)

https://codereview.chromium.org/1516843002/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1516843002/diff/1/src/objects.cc#newcode4727
src/objects.cc:4727: NewTypeError(MessageTemplate::kProxyTrapReturnedNonBoolean,
On 2015/12/10 at 15:35:03, Jakob wrote:
> Nope, the problem is that it returned a falsish value when it should have
returned a truish value.

updated to kProxyReturnedFalsish

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/for-in-opt.js
File test/mjsunit/for-in-opt.js (left):

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/for-in-opt.js#...
test/mjsunit/for-in-opt.js:18: assertEquals(["0"], f("a"));
On 2015/12/10 at 16:08:13, neis wrote:
> On 2015/12/10 15:35:03, Jakob wrote:
> > What's wrong with this? (I mean the entire file.)
> 
> I tried to figure out what it was testing (in order to meaningfully convert
the test) but gave up.

enabled it again (for in + proxies + prototype walks had a minor bug)

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1516843002/diff/1/test/mjsunit/mjsunit.status...
test/mjsunit/mjsunit.status:94: 'harmony/proxies-with': [SKIP],
On 2015/12/10 at 15:13:50, neis wrote:
> Can't proxies-with be enabled?

yes indeed.

https://codereview.chromium.org/1516843002/diff/80001/test/mjsunit/es6/regres...
File test/mjsunit/es6/regress/regress-cr493566.js (right):

https://codereview.chromium.org/1516843002/diff/80001/test/mjsunit/es6/regres...
test/mjsunit/es6/regress/regress-cr493566.js:101: assertEquals(3, target.y);
this doesn't make any sense to me, especially since I have a [[Set]] trap in the
proxy which is never called.

https://codereview.chromium.org/1516843002/diff/80001/test/mjsunit/harmony/re...
File test/mjsunit/harmony/regress/regress-405844.js (left):

https://codereview.chromium.org/1516843002/diff/80001/test/mjsunit/harmony/re...
test/mjsunit/harmony/regress/regress-405844.js:13: Object.observe(functionProxy,
function(){});
object.observe is no longer active, so no need to maintain this code.

[commit-bot: I haz the power, 2015-12-10 20:35:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 20:35:23]

Dry run: Try jobs failed on following builders:
  v8_linux_arm_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/11326)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)

[Jakob Kummerow, 2015-12-10 21:12:14]

Looks good, but that enumerate test still doesn't seem to be quite working yet.
Also, most of it is commented out, see below.

https://codereview.chromium.org/1516843002/diff/100001/test/mjsunit/harmony/p...
File test/mjsunit/harmony/proxies-enumerate.js (right):

https://codereview.chromium.org/1516843002/diff/100001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-enumerate.js:6: /*
leftover?

[Camillo Bruni, 2015-12-11 10:48:17]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-11 10:48:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516843002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516843002/120001

[Camillo Bruni, 2015-12-11 10:51:01]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-11 10:51:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516843002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516843002/140001

[commit-bot: I haz the power, 2015-12-11 11:35:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-11 11:35:18]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2015-12-11 11:45:47]

cbruni@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Camillo Bruni, 2015-12-11 11:45:48]

PTAL

bmeurer: could you check compiler/* again?

=> needed Smi(1) push for for-in map checks for now, will investigate

[neis, 2015-12-11 12:08:15]

https://codereview.chromium.org/1516843002/diff/140001/test/mjsunit/harmony/p...
File test/mjsunit/harmony/proxies.js (right):

https://codereview.chromium.org/1516843002/diff/140001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies.js:53: /*
What happens when you remove the comment around the next 1400 lines?

[Camillo Bruni, 2015-12-11 12:40:55]

https://codereview.chromium.org/1516843002/diff/140001/test/mjsunit/harmony/p...
File test/mjsunit/harmony/proxies.js (right):

https://codereview.chromium.org/1516843002/diff/140001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies.js:53: /*
On 2015/12/11 at 12:08:15, neis wrote:
> What happens when you remove the comment around the next 1400 lines?

yawn... I really forgot all the debug comments I had (yes it was too late
yesterday), thanks :P

[Camillo Bruni, 2015-12-11 12:48:30]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-11 12:48:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516843002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516843002/160001

[commit-bot: I haz the power, 2015-12-11 13:00:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-11 13:00:36]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)

[Jakob Kummerow, 2015-12-11 13:14:43]

lgtm

[Benedikt Meurer, 2015-12-11 13:20:22]

LGTM on compiler

[Camillo Bruni, 2015-12-11 13:22:02]

The CQ bit was checked by cbruni@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-11 13:22:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516843002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516843002/160001

[commit-bot: I haz the power, 2015-12-11 14:06:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-11 14:06:53]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2015-12-11 14:24:47]

The CQ bit was checked by cbruni@chromium.org

[commit-bot: I haz the power, 2015-12-11 14:24:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1516843002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1516843002/160001

[commit-bot: I haz the power, 2015-12-11 14:55:25]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-11 14:56:07]

Description was changed from

==========
[proxy] fixing for-in for proxies, fixing harmony/proxy.js tests, improving
error messages and some drive-by fixes

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)
==========

to

==========
[proxy] fixing for-in for proxies, fixing harmony/proxy.js tests, improving
error messages and some drive-by fixes

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)

Committed: https://crrev.com/df2a92972b0a087080e67496177e879e9409d5b0
Cr-Commit-Position: refs/heads/master@{#32801}
==========

[commit-bot: I haz the power, 2015-12-11 14:56:08]

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

[adamk, 2015-12-11 23:29:42]

Hi Camillo, just a note that it would have been nice for this CL to have a
better description, e.g., what did it fix? how? etc. This is useful both for
archaeology and for folks  skimming v8-reviews (as I tend to do).

[dingxiangfei2009, 2015-12-12 01:27:01]

dingxiangfei2009@gmail.com changed reviewers:
+ dingxiangfei2009@gmail.com

[dingxiangfei2009, 2015-12-12 01:27:02]

The file name has a typo, I believe.

https://codereview.chromium.org/1516843002/diff/180001/test/mjsunit/harmony/p...
File test/mjsunit/harmony/proxies-cross-realm-ecxeption.js (right):

https://codereview.chromium.org/1516843002/diff/180001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-cross-realm-ecxeption.js:1: // Copyright 2015 the
V8 project authors. All rights reserved.
The file name has a typo

[Camillo Bruni, 2015-12-14 08:33:34]

Description was changed from

==========
[proxy] fixing for-in for proxies, fixing harmony/proxy.js tests, improving
error messages and some drive-by fixes

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)

Committed: https://crrev.com/df2a92972b0a087080e67496177e879e9409d5b0
Cr-Commit-Position: refs/heads/master@{#32801}
==========

to

==========
[proxy] fixing for-in for proxies, fixing harmony/proxy.js tests, improving
error messages and some drive-by fixes

- Enable old proxy tests. Many tests had to be updated. Proxies are instantiated
differently and there is no distinction between normal proxies and callable
function proxies anymore. Most additional changes are related to the
getOwnPropertyDescriptor trap which is not accessed as often anymore for new
proxies.
- For-in still used the old-style proxy semantics which skip the [[Has]] check
for each loop-iteration. This CL uses the slower but correct runtime fallback
for proxies.

BUG=v8:1543
LOG=n

patch from issue 1519473002 at patchset 1 (http://crrev.com/1519473002#ps1)

Committed: https://crrev.com/df2a92972b0a087080e67496177e879e9409d5b0
Cr-Commit-Position: refs/heads/master@{#32801}
==========

[Camillo Bruni, 2015-12-14 08:36:00]

On 2015/12/12 at 01:27:02, dingxiangfei2009 wrote:
> The file name has a typo, I believe.
> 
>
https://codereview.chromium.org/1516843002/diff/180001/test/mjsunit/harmony/p...
> File test/mjsunit/harmony/proxies-cross-realm-ecxeption.js (right):
> 
>
https://codereview.chromium.org/1516843002/diff/180001/test/mjsunit/harmony/p...
> test/mjsunit/harmony/proxies-cross-realm-ecxeption.js:1: // Copyright 2015 the
V8 project authors. All rights reserved.
> The file name has a typo

yes indeed :), not this is the WIP CL (uploaded to the wrong issue).

[Camillo Bruni, 2015-12-14 08:36:57]

On 2015/12/11 at 23:29:42, adamk wrote:
> Hi Camillo, just a note that it would have been nice for this CL to have a
better description, e.g., what did it fix? how? etc. This is useful both for
archaeology and for folks  skimming v8-reviews (as I tend to do).

You're right... I updated the description here to give at least some idea what's
going on ;)