Skip to content

Commit

Permalink
fixed issue #11
Browse files Browse the repository at this point in the history
  • Loading branch information
Tom Van Cutsem committed Dec 20, 2012
1 parent 6945efa commit b4b2f7a
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions reflect.js
View file
Expand Up @@ -1601,15 +1601,19 @@ var Reflect = global.Reflect = {
}
// otherwise, isDataDescriptor(ownDesc) must be true
if (ownDesc.writable === false) return false;
if (receiver === target) {
// we found an existing writable data property on the prototype chain.
// Now update or add the data property on the receiver, depending on
// whether the receiver already defines the property or not.
var existingDesc = Object.getOwnPropertyDescriptor(receiver, name);
if (existingDesc !== undefined) {
var updateDesc =
{ value: value,
// FIXME: it should not be necessary to describe the following
// attributes. Added to circumvent a bug in tracemonkey:
// https://bugzilla.mozilla.org/show_bug.cgi?id=601329
writable: ownDesc.writable,
enumerable: ownDesc.enumerable,
configurable: ownDesc.configurable };
writable: existingDesc.writable,
enumerable: existingDesc.enumerable,
configurable: existingDesc.configurable };
Object.defineProperty(receiver, name, updateDesc);
return true;
} else {
Expand Down

1 comment on commit b4b2f7a

@tvcutsem
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Reflect.set (or [[SetP]] as the algorithm is called in the ES6 draft spec), at one point, the algorithm must decide whether to update an existing data property, or rather to add a new data property to the receiver object.

Previously, the decision was made based on whether receiver === target. If the target and receiver are the same, we know we are performing a "direct" update, and since we just found a data property (ownDesc) on target, we know the descriptor also exists on the receiver object.

However, this identity test failed in the presence of proxies where receiver could be a proxy for the target object. This caused the test to take the wrong branch in the presence of proxies, with behavior like the one reported in issue #11.

The fix was to no longer use object identity to test whether we need to add or update a property, but rather simply to query the receiver object to see if it already defines a property with the same name. This test works correctly even if receiver is a proxy for the target.

For normal objects, the new test is more expensive and a bit redundant, but inside an ES6 engine, the fast path (where both target and receiver are normal objects) will be optimized and would probably not make use of this algorithm at all.

Please sign in to comment.