Bug 1368626 - Avoid duplicate lookups when setting a new property. r=jandem
authorAndré Bargull <andre.bargull@gmail.com>
Fri, 02 Jun 2017 20:36:43 +0200
changeset 410776 7ad815515bd315ac0da9ed9b8c32b1f5dfeb27bf
parent 410775 f302f3481aaa3cae72fd8300892d0d66287751b7
child 410777 c474a1264de73553754f9d3570613cf2be594148
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1368626
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1368626 - Avoid duplicate lookups when setting a new property. r=jandem
js/src/jit-test/tests/baseline/bug1368626.js
js/src/shell/js.cpp
js/src/vm/NativeObject.cpp
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/baseline/bug1368626.js
@@ -0,0 +1,19 @@
+var sandbox = evalcx("lazy");
+
+// Ensure we can't change the "lazy" property of the sandbox to an accessor,
+// because that'd allow to execute arbitrary side-effects when calling the
+// resolve hook of the sandbox.
+var err;
+try {
+    Object.defineProperty(sandbox, "lazy", {
+        get() {
+            Object.defineProperty(sandbox, "foo", { value: 0 });
+        }
+    });
+} catch (e) {
+    err = e;
+}
+assertEq(err instanceof TypeError, true);
+
+// Don't assert here.
+sandbox.foo = 1;
--- a/js/src/shell/js.cpp
+++ b/js/src/shell/js.cpp
@@ -3124,17 +3124,17 @@ NewSandbox(JSContext* cx, bool lazy)
         return nullptr;
 
     {
         JSAutoCompartment ac(cx, obj);
         if (!lazy && !JS_InitStandardClasses(cx, obj))
             return nullptr;
 
         RootedValue value(cx, BooleanValue(lazy));
-        if (!JS_SetProperty(cx, obj, "lazy", value))
+        if (!JS_DefineProperty(cx, obj, "lazy", value, JSPROP_PERMANENT | JSPROP_READONLY))
             return nullptr;
 
         JS_FireOnNewGlobalObject(cx, obj);
     }
 
     if (!cx->compartment()->wrap(cx, &obj))
         return nullptr;
     return obj;
--- a/js/src/vm/NativeObject.cpp
+++ b/js/src/vm/NativeObject.cpp
@@ -1883,16 +1883,79 @@ bool
 js::NativeDefineProperty(JSContext* cx, HandleNativeObject obj, PropertyName* name,
                          HandleValue value, JSGetterOp getter, JSSetterOp setter,
                          unsigned attrs)
 {
     RootedId id(cx, NameToId(name));
     return NativeDefineProperty(cx, obj, id, value, getter, setter, attrs);
 }
 
+static bool
+DefineNonexistentProperty(JSContext* cx, HandleNativeObject obj, HandleId id,
+                          Handle<PropertyDescriptor> desc, ObjectOpResult& result)
+{
+    desc.assertComplete();
+
+    // Optimized NativeDefineProperty() version for known absent properties.
+
+    // Dispense with custom behavior of exotic native objects first.
+    if (obj->is<ArrayObject>()) {
+        // Array's length property is non-configurable, so we shouldn't
+        // encounter it in this function.
+        MOZ_ASSERT(id != NameToId(cx->names().length));
+
+        // 9.4.2.1 step 3. Don't extend a fixed-length array.
+        uint32_t index;
+        if (IdIsIndex(id, &index)) {
+            if (WouldDefinePastNonwritableLength(obj, index))
+                return result.fail(JSMSG_CANT_DEFINE_PAST_ARRAY_LENGTH);
+        }
+    } else if (obj->is<TypedArrayObject>()) {
+        // 9.4.5.3 step 3. Indexed properties of typed arrays are special.
+        uint64_t index;
+        if (IsTypedArrayIndex(id, &index)) {
+            // This method is only called for non-existent properties, which
+            // means any absent indexed property must be out of range.
+            MOZ_ASSERT(index >= obj->as<TypedArrayObject>().length());
+
+            // We (wrongly) ignore out of range defines.
+            return result.succeed();
+        }
+    } else if (obj->is<ArgumentsObject>()) {
+        // If this method is called with either |length| or |@@iterator|, the
+        // property was previously deleted and hence should already be marked
+        // as overridden.
+        MOZ_ASSERT_IF(id == NameToId(cx->names().length),
+                      obj->as<ArgumentsObject>().hasOverriddenLength());
+        MOZ_ASSERT_IF(JSID_IS_SYMBOL(id) && JSID_TO_SYMBOL(id) == cx->wellKnownSymbols().iterator,
+                      obj->as<ArgumentsObject>().hasOverriddenIterator());
+
+        // We still need to mark any element properties as overridden.
+        if (JSID_IS_INT(id))
+            obj->as<ArgumentsObject>().markElementOverridden();
+    }
+
+#ifdef DEBUG
+    Rooted<PropertyResult> prop(cx);
+    NativeLookupOwnPropertyNoResolve(cx, obj, id, &prop);
+    MOZ_ASSERT(!prop, "didn't expect to find an existing property");
+#endif
+
+    // 9.1.6.3, ValidateAndApplyPropertyDescriptor.
+    // Step 1 is a redundant assertion, step 3 and later don't apply here.
+
+    // Step 2.
+    if (!obj->nonProxyIsExtensible())
+        return result.fail(JSMSG_CANT_DEFINE_PROP_OBJECT_NOT_EXTENSIBLE);
+
+    if (!AddOrChangeProperty<IsAddOrChange::Add>(cx, obj, id, desc))
+        return false;
+    return result.succeed();
+}
+
 
 /*** [[HasProperty]] *****************************************************************************/
 
 // ES6 draft rev31 9.1.7.1 OrdinaryHasProperty
 bool
 js::NativeHasProperty(JSContext* cx, HandleNativeObject obj, HandleId id, bool* foundp)
 {
     RootedNativeObject pobj(cx, obj);
@@ -2434,16 +2497,40 @@ NativeSetExistingDataProperty(JSContext*
          obj->contains(cx, shape)))
     {
         obj->setSlot(shape->slot(), value);
     }
 
     return true;  // result is populated by CallJSSetterOp above.
 }
 
+static bool
+CallSetPropertyHookAfterDefining(JSContext* cx, HandleObject receiver, HandleId id, HandleValue v,
+                                 ObjectOpResult& result)
+{
+    MOZ_ASSERT(receiver->is<NativeObject>());
+    MOZ_ASSERT(receiver->getClass()->getSetProperty());
+
+    if (!result)
+        return true;
+
+    Rooted<NativeObject*> nativeReceiver(cx, &receiver->as<NativeObject>());
+    MOZ_ASSERT(!cx->helperThread());
+    RootedValue receiverValue(cx, ObjectValue(*receiver));
+
+    // This lookup is a bit unfortunate, but not nearly the most
+    // unfortunate thing about Class getters and setters. Since the above
+    // DefineProperty call succeeded, receiver is native, and the property
+    // has a setter (and thus can't be a dense element), this lookup is
+    // guaranteed to succeed.
+    RootedShape shape(cx, nativeReceiver->lookup(cx, id));
+    MOZ_ASSERT(shape);
+    return NativeSetExistingDataProperty(cx, nativeReceiver, shape, v, receiverValue, result);
+}
+
 /*
  * When a [[Set]] operation finds no existing property with the given id
  * or finds a writable data property on the prototype chain, we end up here.
  * Finish the [[Set]] by defining a new property on receiver.
  *
  * This implements ES6 draft rev 28, 9.1.9 [[Set]] steps 5.b-f, but it
  * is really old code and there are a few barnacles.
  */
@@ -2493,34 +2580,18 @@ js::SetPropertyByDefining(JSContext* cx,
     JSSetterOp setter = clasp->getSetProperty();
     MOZ_ASSERT(getter != JS_PropertyStub);
     MOZ_ASSERT(setter != JS_StrictPropertyStub);
     if (!DefineProperty(cx, receiver, id, v, getter, setter, attrs, result))
         return false;
 
     // If the receiver is native, there is one more legacy wrinkle: the class
     // JSSetterOp is called after defining the new property.
-    if (setter && receiver->is<NativeObject>()) {
-        if (!result)
-            return true;
-
-        Rooted<NativeObject*> nativeReceiver(cx, &receiver->as<NativeObject>());
-        MOZ_ASSERT(!cx->helperThread());
-        RootedValue receiverValue(cx, ObjectValue(*receiver));
-
-        // This lookup is a bit unfortunate, but not nearly the most
-        // unfortunate thing about Class getters and setters. Since the above
-        // DefineProperty call succeeded, receiver is native, and the property
-        // has a setter (and thus can't be a dense element), this lookup is
-        // guaranteed to succeed.
-        RootedShape shape(cx, nativeReceiver->lookup(cx, id));
-        MOZ_ASSERT(shape);
-        return NativeSetExistingDataProperty(cx, nativeReceiver, shape, v,
-                                             receiverValue, result);
-    }
+    if (setter && receiver->is<NativeObject>())
+        return CallSetPropertyHookAfterDefining(cx, receiver, id, v, result);
 
     return true;
 }
 
 // When setting |id| for |receiver| and |obj| has no property for id, continue
 // the search up the prototype chain.
 bool
 js::SetPropertyOnProto(JSContext* cx, HandleObject obj, HandleId id, HandleValue v,
@@ -2538,25 +2609,73 @@ js::SetPropertyOnProto(JSContext* cx, Ha
 /*
  * Implement "the rest of" assignment to a property when no property receiver[id]
  * was found anywhere on the prototype chain.
  *
  * FIXME: This should be updated to follow ES6 draft rev 28, section 9.1.9,
  * steps 4.d.i and 5.
  */
 static bool
-SetNonexistentProperty(JSContext* cx, HandleId id, HandleValue v, HandleValue receiver,
-                       QualifiedBool qualified, ObjectOpResult& result)
+SetNonexistentProperty(JSContext* cx, HandleNativeObject obj, HandleId id, HandleValue v,
+                       HandleValue receiver, QualifiedBool qualified, ObjectOpResult& result)
 {
     if (!qualified && receiver.isObject() && receiver.toObject().isUnqualifiedVarObj()) {
         RootedString idStr(cx, JSID_TO_STRING(id));
         if (!MaybeReportUndeclaredVarAssignment(cx, idStr))
             return false;
     }
 
+    // Pure optimization for the common case. There's no point performing the
+    // lookup in step 5.c again, as our caller just did it for us.
+    if (qualified && receiver.isObject() && obj == &receiver.toObject()) {
+        // Ensure that a custom GetOwnPropertyOp, if present, doesn't
+        // introduce additional properties which weren't previously found by
+        // LookupOwnProperty.
+#ifdef DEBUG
+        if (GetOwnPropertyOp op = obj->getOpsGetOwnPropertyDescriptor()) {
+            Rooted<PropertyDescriptor> desc(cx);
+            if (!op(cx, obj, id, &desc))
+                return false;
+
+            MOZ_ASSERT(!desc.object());
+        }
+#endif
+
+        // Step 5.e. Define the new data property.
+
+        const Class* clasp = obj->getClass();
+        JSGetterOp getter = clasp->getGetProperty();
+        JSSetterOp setter = clasp->getSetProperty();
+        MOZ_ASSERT(getter != JS_PropertyStub);
+        MOZ_ASSERT(setter != JS_StrictPropertyStub);
+
+        Rooted<PropertyDescriptor> desc(cx);
+        desc.initFields(nullptr, v, JSPROP_ENUMERATE, getter, setter);
+
+        if (DefinePropertyOp op = obj->getOpsDefineProperty()) {
+            // Purge the property cache of now-shadowed id in receiver's environment chain.
+            if (!PurgeEnvironmentChain(cx, obj, id))
+                return false;
+
+            MOZ_ASSERT(!cx->helperThread());
+            if (!op(cx, obj, id, desc, result))
+                return false;
+        } else {
+            if (!DefineNonexistentProperty(cx, obj, id, desc, result))
+                return false;
+        }
+
+        // There is one more legacy wrinkle: the class JSSetterOp is called
+        // after defining the new property.
+        if (setter)
+            return CallSetPropertyHookAfterDefining(cx, obj, id, v, result);
+
+        return true;
+    }
+
     return SetPropertyByDefining(cx, id, v, receiver, result);
 }
 
 /*
  * Set an existing own property obj[index] that's a dense element or typed
  * array element.
  */
 static bool
@@ -2704,34 +2823,34 @@ js::NativeSetProperty(JSContext* cx, Han
         //   doesn't have that many elements.
         // - We're being called from a resolve hook to assign to the property
         //   being resolved.
         // What they all have in common is we do not want to keep walking
         // the prototype chain.
         RootedObject proto(cx, done ? nullptr : pobj->staticPrototype());
         if (!proto) {
             // Step 4.d.i (and step 5).
-            return SetNonexistentProperty(cx, id, v, receiver, qualified, result);
+            return SetNonexistentProperty(cx, obj, id, v, receiver, qualified, result);
         }
 
         // Step 4.c.i. If the prototype is also native, this step is a
         // recursive tail call, and we don't need to go through all the
         // plumbing of SetProperty; the top of the loop is where we're going to
         // end up anyway. But if pobj is non-native, that optimization would be
         // incorrect.
         if (!proto->isNative()) {
             // Unqualified assignments are not specified to go through [[Set]]
             // at all, but they do go through this function. So check for
             // unqualified assignment to a nonexistent global (a strict error).
             if (!qualified) {
                 bool found;
                 if (!HasProperty(cx, proto, id, &found))
                     return false;
                 if (!found)
-                    return SetNonexistentProperty(cx, id, v, receiver, qualified, result);
+                    return SetNonexistentProperty(cx, obj, id, v, receiver, qualified, result);
             }
 
             return SetProperty(cx, proto, id, v, receiver, result);
         }
         pobj = &proto->as<NativeObject>();
     }
 }