Make proxies work as prototypes.

Make proxies work as prototypes.
Fix a couple of other proxy bugs along the way.
Refactor trap invocation in native code.

R=kmillikin@chromium.org
BUG=v8:1543
TEST=

Committed: http://code.google.com/p/v8/source/detail?r=9312

[Kevin Millikin (Chromium), 2011-08-31 10:15:31]

I took a quick look.  I think there are two places that are GC hazards,
mentioned below.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode140
src/objects.cc:140: if (IsSmi()) {
You can use IsNumber() here and remove the clause for IsHeapNumber() below.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode231
src/objects.cc:231: Object** args[] = { receiver.location(), name.location() };
We seem to have a mix of this style and

Handle<Object> args[] = { receiver, name };
Handle<Object> result = CallTrap(..., HandleVector(args, ARRAY_SIZE(args));

I prefer the Handle version, partly because mentioning Handle<T>::location()
indicates something special and weird going on and so I prefer to keep it rare.

Also, seeing Handle in a signature makes it obvious (?) that the function is in
the handle world and should not be called from the raw pointer world.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode1882
src/objects.cc:1882: Object* value) {
Indentation is now off.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode1910
src/objects.cc:1910: result->HandlerResult(JSProxy::cast(pt));
I like to write

return result->HandlerResult(JSProxy::cast(pt));

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2295
src/objects.cc:2295: JSReceiver* receiver_raw, String* name_raw) {
Also one parameter per line here.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2329
src/objects.cc:2329: const char* name, Object* derived, int argc, Object***
args) {
I think there's a GC hazard here.  derived is a raw pointer, and looking up a
symbol in the symbol table can cause allocation and thus GC (it may have to
allocate the symbol and may also have to grow the symbol table).

Change derived to Handle<Object> in the signature?

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2353
src/objects.cc:2353: MaybeObject* JSObject::SetPropertyForResult(LookupResult*
result,
I think SetPropertyForResult needs a better name but I'm at a loss for ideas.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2377
src/objects.cc:2377: JSObject* object = JSObject::cast(this);
Isn't this already JSObject*?

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2411
src/objects.cc:2411: Handle<Object> result = proxy->CallTrap(
There's also a GC hazard here.  CallTrap can silently perform GC and there's no
way for the caller of this function to find that out.

http://codereview.chromium.org/7799026/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7799026/diff/1/src/objects.h#newcode6613
src/objects.h:6613: int argc, Object*** args);
We consistently use one line per parameter in declarations that don't fit on a
single line.  Even when they're related like here.

http://codereview.chromium.org/7799026/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7799026/diff/1/src/runtime.cc#newcode4496
src/runtime.cc:4496: if (result) return isolate->heap()->true_value();
You can also write this without the if as:

return isolate->heap()->ToBoolean(result);

[rossberg, 2011-08-31 13:28:32]

Good catch, thanks!

http://codereview.chromium.org/7799026/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode140
src/objects.cc:140: if (IsSmi()) {
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> You can use IsNumber() here and remove the clause for IsHeapNumber() below.

Done.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode231
src/objects.cc:231: Object** args[] = { receiver.location(), name.location() };
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> We seem to have a mix of this style and
> 
> Handle<Object> args[] = { receiver, name };
> Handle<Object> result = CallTrap(..., HandleVector(args, ARRAY_SIZE(args));
> 
> I prefer the Handle version, partly because mentioning Handle<T>::location()
> indicates something special and weird going on and so I prefer to keep it
rare.
> 
> Also, seeing Handle in a signature makes it obvious (?) that the function is
in
> the handle world and should not be called from the raw pointer world.

I agree. I fully handlified the type of CallTrap. But that seems to require an
ugly reinterpret_cast for args. Is there a reason whys Execution::Call and
friends do not properly type the args array with Handle<>[]? Or even use a
HandleVector, like e.g. Throw() etc? Is that perhaps worth changing?

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode1882
src/objects.cc:1882: Object* value) {
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> Indentation is now off.

Done.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode1910
src/objects.cc:1910: result->HandlerResult(JSProxy::cast(pt));
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> I like to write
> 
> return result->HandlerResult(JSProxy::cast(pt));

I know. :) Done.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2295
src/objects.cc:2295: JSReceiver* receiver_raw, String* name_raw) {
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> Also one parameter per line here.

Done.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2329
src/objects.cc:2329: const char* name, Object* derived, int argc, Object***
args) {
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> I think there's a GC hazard here.  derived is a raw pointer, and looking up a
> symbol in the symbol table can cause allocation and thus GC (it may have to
> allocate the symbol and may also have to grow the symbol table).
> 
> Change derived to Handle<Object> in the signature?

Done.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2353
src/objects.cc:2353: MaybeObject* JSObject::SetPropertyForResult(LookupResult*
result,
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> I think SetPropertyForResult needs a better name but I'm at a loss for ideas.

Same here. IMHO some bigger clean-up of the whole Get/Set interface is in order
soon anyway, I'd leave it for that.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2377
src/objects.cc:2377: JSObject* object = JSObject::cast(this);
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> Isn't this already JSObject*?

Right. I had moved the method to JSReceiver, but undid that and overlooked this.

http://codereview.chromium.org/7799026/diff/1/src/objects.cc#newcode2411
src/objects.cc:2411: Handle<Object> result = proxy->CallTrap(
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> There's also a GC hazard here.  CallTrap can silently perform GC and there's
no
> way for the caller of this function to find that out.

Argh, of course. The good news is that the only caller is
JSReceiver::SetProperty, which is already documented to potentially cause GC.

The bad news is that a GC might invalidate `result' -- specifically,
result->holder, which is used for a number of methods on LookupResult below, so
it's not enough to handlify the holder separately from `result'.

I think I can get around that problem here by fully separating the code paths
(in this case, this only required duplicating the !IsFound case). But in
general, is there a way to properly deal with GC vs LookupResult?

http://codereview.chromium.org/7799026/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7799026/diff/1/src/objects.h#newcode6613
src/objects.h:6613: int argc, Object*** args);
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> We consistently use one line per parameter in declarations that don't fit on a
> single line.  Even when they're related like here.

Done.

http://codereview.chromium.org/7799026/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7799026/diff/1/src/runtime.cc#newcode4496
src/runtime.cc:4496: if (result) return isolate->heap()->true_value();
On 2011/08/31 10:15:31, Kevin Millikin wrote:
> You can also write this without the if as:
> 
> return isolate->heap()->ToBoolean(result);

Done.

[Kevin Millikin (Chromium), 2011-09-15 10:11:23]

LGTM now.