[runtime] support new Proxy() instead of Proxy.create and install getPrototypeOf trap

[runtime] support new Proxy() instead of Proxy.create and install getPrototypeOf trap

LOG=N
BUG=v8:1543
Committed: https://crrev.com/24e058d0ede601c683b1128419c9f7775d546af8
Cr-Commit-Position: refs/heads/master@{#31983}

[Camillo Bruni, 2015-11-09 12:58:26]

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

[commit-bot: I haz the power, 2015-11-09 12:58:38]

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

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

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

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

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

[Camillo Bruni, 2015-11-11 12:38:20]

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

[Camillo Bruni, 2015-11-11 12:38:21]

PTAL
For now I skip the membrane example. The main issues is that we try to wrap null
in a Proxy, which is not allowed in ES6. The example trap implementation should
server for all other traps.

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

https://codereview.chromium.org/1417063011/diff/1/src/objects.cc#newcode8064
src/objects.cc:8064: if (map->IsJSObjectMap()) {
Probably should only allow this during bootstrapping.

[Camillo Bruni, 2015-11-11 13:39:48]

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

[commit-bot: I haz the power, 2015-11-11 13:39:58]

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

[commit-bot: I haz the power, 2015-11-11 14:10:29]

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

[commit-bot: I haz the power, 2015-11-11 14:10:30]

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

[Jakob Kummerow, 2015-11-12 12:04:24]

Looking good, bunch o' comments, mostly nits.

And you probably want to take a look at that try job failure.

https://codereview.chromium.org/1417063011/diff/120001/src/js/proxy.js
File src/js/proxy.js (right):

https://codereview.chromium.org/1417063011/diff/120001/src/js/proxy.js#newcode42
src/js/proxy.js:42: throw MakeTypeError(kProxyTargetNonObject, "createFunction")
This is surprising

https://codereview.chromium.org/1417063011/diff/120001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1417063011/diff/120001/src/messages.h#newcode185
src/messages.h:185: T(ProxyTargetNonObject, "Proxy.% called with non-object as
target")          \
nit: please alpha-sort

https://codereview.chromium.org/1417063011/diff/120001/src/messages.h#newcode191
src/messages.h:191: T(ProxyHandlerNonObject, "Proxy.create called with no-object
as handler")    \
nit: please alpha-sort

https://codereview.chromium.org/1417063011/diff/120001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1417063011/diff/120001/src/objects-inl.h#newc...
src/objects-inl.h:6411: Object* JSProxy::target() const { return
map()->prototype(); }
As discussed, we probably prefer an explicit |target| field with a regular
accessor to it (see Georg's CL). This doesn't need to block this CL; I'm fine
with landing as-is and adding a comment about our future intentions.

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc#newcode848
src/objects.cc:848: // static
Please also mention the spec chapter:
// ES6 9.5.1

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc#newcode853
src/objects.cc:853: //  2.  If handler is null, throw a TypeError exception.
nit: drop a space before "If"

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc#newcode856
src/objects.cc:856: // TODO(cbruni): throw correct error message
nit: comments, including TODOs, should begin capitalized and end with
appropriate punctuation.

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc#newcode862
src/objects.cc:862: // TODO(cbruni): Probably change the type of target to
JSObject directly
Can a target not be another Proxy? :-)

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc#newcode879
src/objects.cc:879: //  8. If Type(handlerProto) is either Object nor Null,
throw a TypeError.
s/either/neither/

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc#newcode880
src/objects.cc:880: if (!handler_proto->IsSpecObject() ||
!handler_proto->IsNull()) {
s/||/&&/

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc#newcode903
src/objects.cc:903: 
nit: two blank lines

https://codereview.chromium.org/1417063011/diff/120001/src/runtime/runtime-de...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/1417063011/diff/120001/src/runtime/runtime-de...
src/runtime/runtime-debug.cc:1453: ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate,
prototype,
Yeah, I think this is going to cause trouble, because callers aren't prepared to
handle exceptions. Maybe add a TODO(1543) saying that we need to come up with a
solution?

https://codereview.chromium.org/1417063011/diff/120001/test/mjsunit/harmony/p...
File test/mjsunit/harmony/proxies-hash.js (right):

https://codereview.chromium.org/1417063011/diff/120001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies-hash.js:34: test(construct, handler, function(h){
return new Proxy({},h)} )
nit: spaces between '){', ',h', and ')}'

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

https://codereview.chromium.org/1417063011/diff/120001/test/mjsunit/harmony/p...
test/mjsunit/harmony/proxies.js:1641: // var f = Proxy.createFunction({},
function() {})
how about adding a TODO so this shows up when grepping?

[Camillo Bruni, 2015-11-12 15:50:35]

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

[commit-bot: I haz the power, 2015-11-12 15:50:47]

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

[Camillo Bruni, 2015-11-12 16:19:48]

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

[commit-bot: I haz the power, 2015-11-12 16:20:03]

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

[Camillo Bruni, 2015-11-12 16:34:12]

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

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

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

[commit-bot: I haz the power, 2015-11-12 16:57:38]

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

[commit-bot: I haz the power, 2015-11-12 16:57:39]

Dry run: This issue passed the CQ dry run.

[rossberg, 2015-11-12 17:04:39]

Looks mostly okay, but I would prefer waiting with landing this CL until Georg
has landed the introduction of a proper target slot and you can rebase on that.

https://codereview.chromium.org/1417063011/diff/200001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1417063011/diff/200001/src/builtins.cc#newcod...
src/builtins.cc:1581: Object::GetPrototype(isolate, target));
That is not correct, I'm afraid. The proxy handler may have a custom trap.

Edit: Ah, I get it, you're abusing the prototype for the target. You dirty
bastard! Please let's not do such hacks, not even temporarily.

https://codereview.chromium.org/1417063011/diff/200001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1417063011/diff/200001/src/factory.cc#newcode...
src/factory.cc:1945: Map::SetPrototype(map, target);
Not sure that makes sense. For the new proxy semantics, the prototype slot in
the map actually makes very little sense, and should never be used. We should
probably null it out to catch potential impl bugs.

https://codereview.chromium.org/1417063011/diff/200001/src/js/proxy.js
File src/js/proxy.js (right):

https://codereview.chromium.org/1417063011/diff/200001/src/js/proxy.js#newcod...
src/js/proxy.js:192: // //Set up non-enumerable properties of the Proxy object.
Nit: weird comment delimiter?

https://codereview.chromium.org/1417063011/diff/200001/src/js/proxy.js#newcod...
src/js/proxy.js:194: "createFunction", ProxyCreateFunction
Shouldn't this go away as well?

https://codereview.chromium.org/1417063011/diff/200001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1417063011/diff/200001/src/messages.h#newcode185
src/messages.h:185: T(ProxyHandlerNonObject, "Proxy.create called with no-object
as handler")    \
It's no longer named that, isn't it?

https://codereview.chromium.org/1417063011/diff/200001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1417063011/diff/200001/src/objects.cc#newcode...
src/objects.cc:5625: } else if (IsJSProxy()) {
A JSObject can never be a JSProxy.

https://codereview.chromium.org/1417063011/diff/200001/src/objects.cc#newcode...
src/objects.cc:5640: } else if (object->IsJSProxy()) {
Saem here.

[Toon Verwaest, 2015-11-12 19:39:05]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2015-11-12 19:39:05]

https://codereview.chromium.org/1417063011/diff/200001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/1417063011/diff/200001/src/builtins.cc#newcod...
src/builtins.cc:1581: Object::GetPrototype(isolate, target));
On 2015/11/12 17:04:39, rossberg wrote:
> That is not correct, I'm afraid. The proxy handler may have a custom trap.
> 
> Edit: Ah, I get it, you're abusing the prototype for the target. You dirty
> bastard! Please let's not do such hacks, not even temporarily.

Independent of whether the target is stored as the prototype (it really
shouldn't, we've discussed this before...), this is Object::GetPrototype, which
calls to JSProxy::GetPrototype in case it's a JSProxy; which in turn calls to
the trap if there is one.

[Camillo Bruni, 2015-11-12 19:49:17]

Description was changed from

==========
[runtime] support new Proxy() instead of Proxy.create

LOG=N
BUG=v8:1543
==========

to

==========
[runtime] support new Proxy() instead of Proxy.create and install getPrototypeOf
trap

LOG=N
BUG=v8:1543
==========

[Camillo Bruni, 2015-11-12 20:20:56]

thanks for the feedback. I'll merge tomorrow after Georg has landed his CL.

https://codereview.chromium.org/1417063011/diff/120001/src/js/proxy.js
File src/js/proxy.js (right):

https://codereview.chromium.org/1417063011/diff/120001/src/js/proxy.js#newcode42
src/js/proxy.js:42: throw MakeTypeError(kProxyTargetNonObject, "createFunction")
On 2015/11/12 at 12:04:24, Jakob wrote:
> This is surprising

blind rename :), reverting.

https://codereview.chromium.org/1417063011/diff/120001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/1417063011/diff/120001/src/objects-inl.h#newc...
src/objects-inl.h:6411: Object* JSProxy::target() const { return
map()->prototype(); }
On 2015/11/12 at 12:04:24, Jakob wrote:
> As discussed, we probably prefer an explicit |target| field with a regular
accessor to it (see Georg's CL). This doesn't need to block this CL; I'm fine
with landing as-is and adding a comment about our future intentions.

added todo.

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1417063011/diff/120001/src/objects.cc#newcode862
src/objects.cc:862: // TODO(cbruni): Probably change the type of target to
JSObject directly
On 2015/11/12 at 12:04:24, Jakob wrote:
> Can a target not be another Proxy? :-)

changed to JSReceiver

https://codereview.chromium.org/1417063011/diff/120001/src/runtime/runtime-de...
File src/runtime/runtime-debug.cc (right):

https://codereview.chromium.org/1417063011/diff/120001/src/runtime/runtime-de...
src/runtime/runtime-debug.cc:1453: ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate,
prototype,
On 2015/11/12 at 12:04:24, Jakob wrote:
> Yeah, I think this is going to cause trouble, because callers aren't prepared
to handle exceptions. Maybe add a TODO(1543) saying that we need to come up with
a solution?

added.

https://codereview.chromium.org/1417063011/diff/200001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/1417063011/diff/200001/src/factory.cc#newcode...
src/factory.cc:1945: Map::SetPrototype(map, target);
On 2015/11/12 at 17:04:39, rossberg wrote:
> Not sure that makes sense. For the new proxy semantics, the prototype slot in
the map actually makes very little sense, and should never be used. We should
probably null it out to catch potential impl bugs.

same as above, will go away.

https://codereview.chromium.org/1417063011/diff/200001/src/js/proxy.js
File src/js/proxy.js (right):

https://codereview.chromium.org/1417063011/diff/200001/src/js/proxy.js#newcod...
src/js/proxy.js:194: "createFunction", ProxyCreateFunction
On 2015/11/12 at 17:04:39, rossberg wrote:
> Shouldn't this go away as well?

yes, I'll leave this as an exercise for the interested reader... 
Cleanup Friday is ahead, will see if I have time to do it.

https://codereview.chromium.org/1417063011/diff/200001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1417063011/diff/200001/src/messages.h#newcode185
src/messages.h:185: T(ProxyHandlerNonObject, "Proxy.create called with no-object
as handler")    \
On 2015/11/12 at 17:04:39, rossberg wrote:
> It's no longer named that, isn't it?

vim auto actions have been enabled :D, will fix.

https://codereview.chromium.org/1417063011/diff/200001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1417063011/diff/200001/src/objects.cc#newcode...
src/objects.cc:5625: } else if (IsJSProxy()) {
On 2015/11/12 at 17:04:39, rossberg wrote:
> A JSObject can never be a JSProxy.

indeed, wrong way around.

[neis, 2015-11-12 23:22:51]

neis@chromium.org changed reviewers:
+ neis@chromium.org

[neis, 2015-11-12 23:22:51]

https://codereview.chromium.org/1417063011/diff/200001/src/js/proxy.js
File src/js/proxy.js (right):

https://codereview.chromium.org/1417063011/diff/200001/src/js/proxy.js#newcod...
src/js/proxy.js:190: %FunctionSetPrototype(GlobalProxy, new GlobalObject());
"Proxy" shouldn't have a "prototype" property.

https://codereview.chromium.org/1417063011/diff/200001/src/js/proxy.js#newcod...
src/js/proxy.js:197: %AddNamedProperty(GlobalProxy.prototype, "constructor",
GlobalProxy, DONT_ENUM);
See above.

[Jakob Kummerow, 2015-11-13 10:47:51]

LGTM with two more nits.

I'm still a bit torn on the tests issue. I think I'm fine with landing newly
written per-trap tests as we go along, and then once we *think* everything
should work look at the bulk of existing proxies tests (that are skipped until
then). Obviously that last step is a requirement before staging the flag.

https://codereview.chromium.org/1417063011/diff/160001/src/messages.h
File src/messages.h (right):

https://codereview.chromium.org/1417063011/diff/160001/src/messages.h#newcode185
src/messages.h:185: T(ProxyHandlerNonObject, "Proxy.create called with no-object
as handler")    \
nit: s/Proxy.create/new Proxy/, s/no-/non-/

https://codereview.chromium.org/1417063011/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/1417063011/diff/160001/src/objects.cc#newcode888
src/objects.cc:888: Maybe<bool> isExtensible = JSReceiver::IsExtensible(target);
nit: is_extensible

[Camillo Bruni, 2015-11-13 13:39:19]

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

[commit-bot: I haz the power, 2015-11-13 13:39:27]

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

[commit-bot: I haz the power, 2015-11-13 13:59:02]

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

[commit-bot: I haz the power, 2015-11-13 13:59:03]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2015-11-13 13:59:32]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2015-11-13 13:59:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkummerow@chromium.org
Link to the patchset: https://codereview.chromium.org/1417063011/#ps260001
(title: "removing unreachable code")

[commit-bot: I haz the power, 2015-11-13 13:59:47]

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

[commit-bot: I haz the power, 2015-11-13 14:02:36]

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

[commit-bot: I haz the power, 2015-11-13 14:02:37]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/7705)

[Camillo Bruni, 2015-11-13 14:06:10]

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

[Camillo Bruni, 2015-11-13 14:06:11]

PTAL
- heap.h for adding internalized strings for all the Proxy traps.

[Michael Starzinger, 2015-11-13 14:12:14]

LGTM (rubber-stamped "heap", didn't look at the rest at all).

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

The CQ bit was checked by cbruni@chromium.org

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

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

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

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-11-14 23:21:32]

Patchset 14 (id:??) landed as
https://crrev.com/24e058d0ede601c683b1128419c9f7775d546af8
Cr-Commit-Position: refs/heads/master@{#31983}