Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Comparing changes

Choose two branches to see what's changed or to start a new pull request. If you need to, you can also compare across forks.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also compare across forks.
  • 2 commits
  • 2 files changed
  • 1 commit comment
  • 1 contributor
Showing with 86 additions and 4 deletions.
  1. +8 −4 reflect.js
  2. +78 −0 test/testRegression.js
View
12 reflect.js
@@ -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 {
View
78 test/testRegression.js
@@ -0,0 +1,78 @@
+// Copyright (C) 2012 Software Languages Lab, Vrije Universiteit Brussel
+// This code is dual-licensed under both the Apache License and the MPL
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+/* Version: MPL 1.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is a series of unit tests for the ES-harmony reflect module.
+ *
+ * The Initial Developer of the Original Code is
+ * Tom Van Cutsem, Vrije Universiteit Brussel.
+ * Portions created by the Initial Developer are Copyright (C) 2012
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *
+ */
+// for node.js
+if(typeof require === 'function') {
+ var load = require;
+ var print = function(msg) {
+ if(/^fail/.test(msg)) { console.error(msg); }
+ else { console.log(msg); }
+ }
+}
+load('../reflect.js');
+
+function assert(b, msg) {
+ print((b ? 'success: ' : 'fail: ') + msg);
+}
+
+function assertThrows(message, fn) {
+ try {
+ fn();
+ print('fail: expected exception, but succeeded. Message was: '+message);
+ } catch(e) {
+ assert(e.message === message, "assertThrows: "+e.message);
+ }
+}
+
+// the 'main' function
+function test() {
+
+ (function(){
+ // https://github.com/tvcutsem/harmony-reflect/issues/11
+ function f() {}
+ var p = Proxy(f, {});
+ var proto = {};
+ p.prototype = proto;
+ assert(p.prototype === proto, 'prototype changed via p');
+ assert(f.prototype === proto, 'prototype changed via f');
+ }());
+
+}
+
+if (typeof window === "undefined") {
+ test();
+}

Showing you all comments on commits in this comparison.

@tvcutsem
Owner

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.

Something went wrong with that request. Please try again.