Added access check to Runtime_GetPrototype.

Added access check to Runtime_GetPrototype.


BUG=93759
Committed: http://code.google.com/p/v8/source/detail?r=9016

[Mads Ager (chromium), 2011-08-24 08:23:15]

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

http://codereview.chromium.org/7701023/diff/1/src/runtime.cc#newcode687
src/runtime.cc:687: if (obj->IsJSGlobalProxy()) {
You shouldn't restrict this to global proxies. Any object can require access
checks. When navigating a page the proxy is detached and access checks are
enabled on the actual global object instead (the actual global becomes its own
proxy).

http://codereview.chromium.org/7701023/diff/1/src/runtime.cc#newcode699
src/runtime.cc:699: obj = obj->GetPrototype();
Since access checks can be enabled on any JSObject I guess the access check
should actually be moved in here?

[Mads Ager (chromium), 2011-08-24 08:23:58]

Please add regression tests for this. Set up some prototype chains with access
checks at various points through the API.

[Lasse Reichstein, 2011-08-24 13:39:41]

Good comments.
Please take a new look.

http://codereview.chromium.org/7701023/diff/4001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7701023/diff/4001/src/runtime.cc#newcode695
src/runtime.cc:695: }
Should we detect a JSGlobalProxy and return the global object's prototype
instead of just the global object? I.e., should the "get prototype" operation be
delegated to the global object, just as property accesses? 
This way, it's visible that there is a JSGlobalProxy, and that it's different
from the Global object.

http://codereview.chromium.org/7701023/diff/4001/src/v8natives.js
File src/v8natives.js (left):

http://codereview.chromium.org/7701023/diff/4001/src/v8natives.js#oldcode652
src/v8natives.js:652: return IS_UNDEFINED(desc) ? false : true;
Seems not to be called at all, and it was calling %GetPrototype without ensuring
that the argument was a JSObject.

[Mads Ager (chromium), 2011-08-24 18:38:40]

LGTM if the proxy code is indeed dead code. Andreas is the code Lasse is
deleting here actually dead code?

http://codereview.chromium.org/7701023/diff/4001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7701023/diff/4001/src/runtime.cc#newcode695
src/runtime.cc:695: }
On 2011/08/24 13:39:41, Lasse Reichstein wrote:
> Should we detect a JSGlobalProxy and return the global object's prototype
> instead of just the global object? I.e., should the "get prototype" operation
be
> delegated to the global object, just as property accesses? 
> This way, it's visible that there is a JSGlobalProxy, and that it's different
> from the Global object.

I don't remember the exact setup here. There are some hidden prototypes involved
in the setup of the global object in the browser, but maybe that is because of
the shadow object that is after the proxy but before the actual global. :-)

This does not seem to cause trouble and is handled consistently across the
__proto__ accessor and this code. I would leave it as is.

http://codereview.chromium.org/7701023/diff/4001/src/v8natives.js
File src/v8natives.js (left):

http://codereview.chromium.org/7701023/diff/4001/src/v8natives.js#oldcode652
src/v8natives.js:652: return IS_UNDEFINED(desc) ? false : true;
On 2011/08/24 13:39:41, Lasse Reichstein wrote:
> Seems not to be called at all, and it was calling %GetPrototype without
ensuring
> that the argument was a JSObject.

This looks proxy related. Are you sure that this is not hooked up somewhere when
running with the proxy flag enabled? You should ask Andreas on this part of the
code. (You are right about the missing checking that should be added if this is
not dead code.)

[Lasse Reichstein, 2011-08-25 07:18:07]

Running the tests with --harmony-block-scoping doesn't fail because of missing
functions (as expected, the names weren't mentioned in any JS file).

However, there are failures due to passing a JSProxy to Object.getPrototypeOf
(in mjsunit/harmony/proxies), since proxies are not JSObject's.

We can't do access checks on proxies, since our callback only supports a
JSObject as object. We should either 1) not do access checks on proxies, but
just get the prototype of the proxy, or 2) extend access checks to apply to
JSReceiver. I'll implement 1 for now, since the ObjectGetPrototypeOf function
doesn't support proxies at all with the current change.

[rossberg, 2011-08-25 13:12:07]

> We can't do access checks on proxies, since our callback only supports a
> JSObject as object. We should either 1) not do access checks on proxies, but
> just get the prototype of the proxy, or 2) extend access checks to apply to
> JSReceiver. I'll implement 1 for now, since the ObjectGetPrototypeOf function
> doesn't support proxies at all with the current change.

I think I briefly discussed access checks with Mads some time ago, and the
conclusion was that they are never needed for proxies. So (1) is consistent with
the proxy code so far.

http://codereview.chromium.org/7701023/diff/4001/src/v8natives.js
File src/v8natives.js (left):

http://codereview.chromium.org/7701023/diff/4001/src/v8natives.js#oldcode652
src/v8natives.js:652: return IS_UNDEFINED(desc) ? false : true;
On 2011/08/24 18:38:40, Mads Ager wrote:
> On 2011/08/24 13:39:41, Lasse Reichstein wrote:
> > Seems not to be called at all, and it was calling %GetPrototype without
> ensuring
> > that the argument was a JSObject.
> 
> This looks proxy related. Are you sure that this is not hooked up somewhere
when
> running with the proxy flag enabled? You should ask Andreas on this part of
the
> code. (You are right about the missing checking that should be added if this
is
> not dead code.)

This may very well be dead code. It existed before proxies, I just added the
proxy code path. I already wondered what it was used for...

[Lasse Reichstein, 2011-08-25 13:21:20]

Excellent, I'll land it then.