Bug 40613 - The JSObjectSetProperty doesn't overwrite property flags.
Summary: The JSObjectSetProperty doesn't overwrite property flags.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Jędrzej Nowacki
URL:
Keywords:
Depends on:
Blocks: 40903
  Show dependency treegraph
 
Reported: 2010-06-15 00:00 PDT by Jędrzej Nowacki
Modified: 2012-09-26 12:42 PDT (History)
6 users (show)

See Also:


Attachments
Fix v1 (3.03 KB, patch)
2010-06-15 00:04 PDT, Jędrzej Nowacki
oliver: review-
oliver: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jędrzej Nowacki 2010-06-15 00:00:38 PDT
It seems that the JSObjectSetProperty can't overwrite property flag. It simply ignores attributes argument while replacing property.
Comment 1 Jędrzej Nowacki 2010-06-15 00:04:48 PDT
Created attachment 58759 [details]
Fix v1
Comment 2 Oliver Hunt 2010-06-21 17:55:20 PDT
Comment on attachment 58759 [details]
Fix v1

This has bad behaviour in a number of ways -- the unconditional use of delete if the property exists will destroy the PIC and so hurt performance for cases where the attributes are not being changed.

Additionally you get weird behaviour if have:

proto = {prop: "foo"}
o = Object.create(proto);

and do:
JSObjectSetProperty(o, "prop", ..., ...);

you will delete "prop" from proto, and then add "prop" to o.  Clearly this is less than ideal.

:-(

--Oliver
Comment 3 Kent Hansen 2010-06-22 01:39:31 PDT
(In reply to comment #2)
> (From update of attachment 58759 [details])
> This has bad behaviour in a number of ways -- the unconditional use of delete if the property exists will destroy the PIC and so hurt performance for cases where the attributes are not being changed.
> 
> Additionally you get weird behaviour if have:
> 
> proto = {prop: "foo"}
> o = Object.create(proto);
> 
> and do:
> JSObjectSetProperty(o, "prop", ..., ...);
> 
> you will delete "prop" from proto, and then add "prop" to o.  Clearly this is less than ideal.

The hasProperty() call seems weird. Just because the object has some property in the prototype chain, the attributes you specify will be ignored when you create a property of the same name on your object?
Would calling hasOwnProperty() be sufficient?
Better, by calling getOwnPropertyDescriptor() one can get the attributes, and only delete the property if they differ.
Oh, looks like JSObject::defineOwnProperty() already has such logic. ;)

Still seems like a work-around, though. How about fixing JSObject::putWithAttributes() so it takes care of updating the attributes of an existing entry in case the property already exists (just like it takes care of updating the value)? Then unconditionally call putWithAttributes() from JSObjectSetProperty() when attributes != 0. (Which means there's no way to remove all attributes without recreating the property; oh well.)
Comment 4 Gavin Barraclough 2012-03-12 15:58:40 PDT
Sigh - the problem here is that it's not really clear what JSObjectSetProperty should be doing.  Does it correspond to the spec's [[Put]] functionality? [[DefineOwnProperty]]? - or a mix of the two?

Given the interface we have, if the property already exists on the object being put to, it probably does make sense to make this call defineOwnProperty so the attributes are updated accordingly.
Comment 5 Gavin Barraclough 2012-09-26 12:42:46 PDT
I don't think we're going to change this interface right now.  The API behaviour is

if ([[HasProperty]])
    [[Put]]
else
    [[DefineOwnProperty]]

It might be nice to separate put & defineOwnProperty traps in the API, but JSObjectSetProperty's behaviour isn't going to change now.