Fix: FunctionTemplate::SetPrototypeAttributes broke prototype object

Fix: FunctionTemplate::SetPrototypeAttributes broke prototype object


BUG=v8:1539
TEST=cctest test-api/SetPrototypeAttributes

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

[Jakob Kummerow, 2011-07-08 09:55:24]

PTAL.

Happy to discuss other solutions -- I've looked into changing
JSObject::SetLocalPropertyIgnoreAttributes() instead, but a quick fix there
broke other tests, and doing it right would probably be messy.

[Jakob Kummerow, 2011-07-11 09:04:45]

Mads, can you take a look?

[Mads Ager (chromium), 2011-07-11 10:12:54]

What happened if you used IgnoreAttributesAndSetProperty? Did it not call the
FunctionSetPrototype setter? Was the assignment just ignored?

http://codereview.chromium.org/7324027/diff/1/src/apinatives.js
File src/apinatives.js (right):

http://codereview.chromium.org/7324027/diff/1/src/apinatives.js#newcode81
src/apinatives.js:81: %IgnoreAttributesAndSetProperty(fun, "prototype", {},
attributes);
Let's create a runtime function %SetPropertyAttributes that changes the
attributes of a property after the fact instead of using this one.

The issue here is that the prototype property is not a normal property but an
accessor that extracts the value from a field in the map. How does the code you
had before fail?

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

http://codereview.chromium.org/7324027/diff/1/src/objects.cc#newcode2409
src/objects.cc:2409: // Note that this method cannot be used to set the
prototype of a function (it
If you want a note here I think it should explain exactly why it fails instead
of just stating that it fails.

[Jakob Kummerow, 2011-07-25 11:10:32]

[Mads to CC since he's on vacation.]

Rico: PTAL. I've discussed this new approach with Mads, but he hasn't seen the
final result yet. The key idea is that since the ReadOnly flag is the only
prototype attribute we can safely modify without breaking assumptions elsewhere,
we should provide a specific method that does precisely this and nothing else.

[Rico, 2011-07-25 13:05:42]

LGTM, are there some chromium guys that needs to change some bindings code?

http://codereview.chromium.org/7324027/diff/6001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7324027/diff/6001/src/objects.cc#newcode2439
src/objects.cc:2439: // will silently fail).
I can't see where we actually silently fail in this method, and it is not
immediately clear which of the method calls would do that.

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode1966
src/runtime.cc:1966: CONVERT_CHECKED(JSFunction, object, args[0]);
it's a function, maybe call it fun, function or f instead of object

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode1973
src/runtime.cc:1973: if (object->HasFastProperties()) {
If it is not performance critical to keep fast properties consider just
converting to dictionary mode instead of the descriptor array juggling

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode1976
src/runtime.cc:1976: int index = instance_desc->Search(name);
assert that index != kNotFound

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode2004
src/runtime.cc:2004: int entry = object->property_dictionary()->FindEntry(name);
assert that entry != kNotFound

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode2008
src/runtime.cc:2008: details.type(), details.index());
nit, put last argument on seperate line

http://codereview.chromium.org/7324027/diff/6001/src/runtime.h
File src/runtime.h (right):

http://codereview.chromium.org/7324027/diff/6001/src/runtime.h#newcode213
src/runtime.h:213: F(FunctionReadOnlyPrototype, 1, 1) \
maybe rename to FunctionSetReadOnlyPrototype

http://codereview.chromium.org/7324027/diff/6001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/7324027/diff/6001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:6997: CHECK_EQ(42,
CompileRun("func2.prototype.x")->Int32Value());
We could also add a check that when we actually write to the prototype it is not
allowed (i.e., to prototype remains the same)

[Jakob Kummerow, 2011-07-25 14:57:05]

Thanks for the review, comments addressed, landing.

http://codereview.chromium.org/7324027/diff/6001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7324027/diff/6001/src/objects.cc#newcode2439
src/objects.cc:2439: // will silently fail).
On 2011/07/25 13:05:42, Rico wrote:
> I can't see where we actually silently fail in this method, and it is not
> immediately clear which of the method calls would do that.

I've made the comment a bit more verbose.

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode1966
src/runtime.cc:1966: CONVERT_CHECKED(JSFunction, object, args[0]);
On 2011/07/25 13:05:42, Rico wrote:
> it's a function, maybe call it fun, function or f instead of object

Done.

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode1973
src/runtime.cc:1973: if (object->HasFastProperties()) {
On 2011/07/25 13:05:42, Rico wrote:
> If it is not performance critical to keep fast properties consider just
> converting to dictionary mode instead of the descriptor array juggling 

I'd prefer to keep performance as it is since this code can be called by
embedders at will.

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode1976
src/runtime.cc:1976: int index = instance_desc->Search(name);
On 2011/07/25 13:05:42, Rico wrote:
> assert that index != kNotFound

Done.

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode2004
src/runtime.cc:2004: int entry = object->property_dictionary()->FindEntry(name);
On 2011/07/25 13:05:42, Rico wrote:
> assert that entry != kNotFound

Done.

http://codereview.chromium.org/7324027/diff/6001/src/runtime.cc#newcode2008
src/runtime.cc:2008: details.type(), details.index());
On 2011/07/25 13:05:42, Rico wrote:
> nit, put last argument on seperate line

Done.

http://codereview.chromium.org/7324027/diff/6001/src/runtime.h
File src/runtime.h (right):

http://codereview.chromium.org/7324027/diff/6001/src/runtime.h#newcode213
src/runtime.h:213: F(FunctionReadOnlyPrototype, 1, 1) \
On 2011/07/25 13:05:42, Rico wrote:
> maybe rename to FunctionSetReadOnlyPrototype

Done.

http://codereview.chromium.org/7324027/diff/6001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/7324027/diff/6001/test/cctest/test-api.cc#newc...
test/cctest/test-api.cc:6997: CHECK_EQ(42,
CompileRun("func2.prototype.x")->Int32Value());
On 2011/07/25 13:05:42, Rico wrote:
> We could also add a check that when we actually write to the prototype it is
not
> allowed (i.e., to prototype remains the same)

Done.