Bug 1008753 - Don't require shape checks when calling getters/setters on common prototypes, r=efaust.

js/src/jit-test/tests/basic/redefinedGetter.js

@@ -0,0 +1,24 @@
+
+function Thing() {}
+
+function foo(x, expected) {
+  for (var i = 0; i < 10; i++)
+    assertEq(x.f, expected);
+}
+
+Object.defineProperty(Thing.prototype, "f", {get: function() { return 2; }, configurable:true});
+foo(new Thing(), 2);
+Object.defineProperty(Thing.prototype, "f", {get: function() { return 3; }});
+foo(new Thing(), 3);
+
+function OtherThing() {}
+
+function bar(x, expected) {
+  for (var i = 0; i < 10; i++)
+    assertEq(x.f, expected);
+}
+
+Object.defineProperty(OtherThing.prototype, "f", {get: function() { return 2; }, configurable:true});
+bar(new OtherThing(), 2);
+delete OtherThing.prototype.f;
+bar(new OtherThing(), undefined);

js/src/jit/BaselineInspector.cpp

@@ -474,7 +474,7 @@ BaselineInspector::templateCallObject()
 }
 
 JSObject *
-BaselineInspector::commonGetPropFunction(jsbytecode *pc, Shape **lastProperty, JSFunction **commonGetter)
+BaselineInspector::commonGetPropFunction(jsbytecode *pc, JSFunction **commonGetter)
 {
     if (!hasBaselineScript())
         return nullptr;
@@ -486,7 +486,6 @@ BaselineInspector::commonGetPropFunction(jsbytecode *pc, Shape **lastProperty, J
             stub->isGetProp_CallNativePrototype())
         {
             ICGetPropCallGetter *nstub = static_cast<ICGetPropCallGetter *>(stub);
-            *lastProperty = nstub->holderShape();
             *commonGetter = nstub->getter();
             return nstub->holder();
         }
@@ -495,7 +494,7 @@ BaselineInspector::commonGetPropFunction(jsbytecode *pc, Shape **lastProperty, J
 }
 
 JSObject *
-BaselineInspector::commonSetPropFunction(jsbytecode *pc, Shape **lastProperty, JSFunction **commonSetter)
+BaselineInspector::commonSetPropFunction(jsbytecode *pc, JSFunction **commonSetter)
 {
     if (!hasBaselineScript())
         return nullptr;
@@ -504,7 +503,6 @@ BaselineInspector::commonSetPropFunction(jsbytecode *pc, Shape **lastProperty, J
     for (ICStub *stub = entry.firstStub(); stub; stub = stub->next()) {
         if (stub->isSetProp_CallScripted() || stub->isSetProp_CallNative()) {
             ICSetPropCallSetter *nstub = static_cast<ICSetPropCallSetter *>(stub);
-            *lastProperty = nstub->holderShape();
             *commonSetter = nstub->setter();
             return nstub->holder();
         }

js/src/jit/BaselineInspector.h

@@ -117,8 +117,8 @@ class BaselineInspector
     DeclEnvObject *templateDeclEnvObject();
     CallObject *templateCallObject();
 
-    JSObject *commonGetPropFunction(jsbytecode *pc, Shape **lastProperty, JSFunction **commonGetter);
-    JSObject *commonSetPropFunction(jsbytecode *pc, Shape **lastProperty, JSFunction **commonSetter);
+    JSObject *commonGetPropFunction(jsbytecode *pc, JSFunction **commonGetter);
+    JSObject *commonSetPropFunction(jsbytecode *pc, JSFunction **commonSetter);
 };
 
 } // namespace jit

js/src/jit/IonBuilder.cpp

@@ -8366,25 +8366,33 @@ IonBuilder::freezePropertiesForCommonPrototype(types::TemporaryTypeSet *types, P
     }
 }
 
-inline MDefinition *
+inline bool
 IonBuilder::testCommonGetterSetter(types::TemporaryTypeSet *types, PropertyName *name,
-                                   bool isGetter, JSObject *foundProto, Shape *lastProperty)
+                                   bool isGetter, JSObject *foundProto, JSFunction *function)
 {
+    types::TypeObjectKey *protoType = types::TypeObjectKey::get(foundProto);
+
+    // The state change done below needs to be done on an object with singleton
+    // type, as otherwise we won't be able to tell whether it already has the
+    // correct getter/setter.
+    if (!protoType->isSingleObject() || protoType->unknownProperties())
+        return false;
+
     // Check if all objects being accessed will lookup the name through foundProto.
     if (!objectsHaveCommonPrototype(types, name, isGetter, foundProto))
-        return nullptr;
+        return false;
 
     // We can optimize the getter/setter, so freeze all involved properties to
     // ensure there isn't a lower shadowing getter or setter installed in the
     // future.
     freezePropertiesForCommonPrototype(types, name, foundProto);
 
-    // Add a shape guard on the prototype we found the property on. The rest of
-    // the prototype chain is guarded by TI freezes. Note that a shape guard is
-    // good enough here, even in the proxy case, because we have ensured there
-    // are no lookup hooks for this property.
-    MInstruction *wrapper = constant(ObjectValue(*foundProto));
-    return addShapeGuard(wrapper, lastProperty, Bailout_ShapeGuard);
+    // TI doesn't have accurate type information for properties with getters or
+    // setters, but we can use the state change machinery to watch out for the
+    // property being redefined on the prototype.
+    protoType->watchStateChangeForRedefinedProperty(constraints(), name, isGetter, function);
+
+    return true;
 }
 
 bool
@@ -8804,16 +8812,13 @@ IonBuilder::getPropTryCommonGetter(bool *emitted, MDefinition *obj, PropertyName
 {
     JS_ASSERT(*emitted == false);
 
-    Shape *lastProperty = nullptr;
     JSFunction *commonGetter = nullptr;
-    JSObject *foundProto = inspector->commonGetPropFunction(pc, &lastProperty, &commonGetter);
+    JSObject *foundProto = inspector->commonGetPropFunction(pc, &commonGetter);
     if (!foundProto)
         return true;
 
     types::TemporaryTypeSet *objTypes = obj->resultTypeSet();
-    MDefinition *guard = testCommonGetterSetter(objTypes, name, /* isGetter = */ true,
-                                                foundProto, lastProperty);
-    if (!guard)
+    if (!testCommonGetterSetter(objTypes, name, /* isGetter = */ true, foundProto, commonGetter))
         return true;
 
     bool isDOM = objTypes->isDOMClass();
@@ -8824,9 +8829,9 @@ IonBuilder::getPropTryCommonGetter(bool *emitted, MDefinition *obj, PropertyName
         if (jitinfo->isInSlot) {
             // We can't use MLoadFixedSlot here because it might not have the
             // right aliasing behavior; we want to alias DOM setters.
-            get = MGetDOMMember::New(alloc(), jitinfo, obj, guard);
+            get = MGetDOMMember::New(alloc(), jitinfo, obj);
         } else {
-            get = MGetDOMProperty::New(alloc(), jitinfo, obj, guard);
+            get = MGetDOMProperty::New(alloc(), jitinfo, obj);
         }
         current->add(get);
         current->push(get);
@@ -9225,16 +9230,13 @@ IonBuilder::setPropTryCommonSetter(bool *emitted, MDefinition *obj,
 {
     JS_ASSERT(*emitted == false);
 
-    Shape *lastProperty = nullptr;
     JSFunction *commonSetter = nullptr;
-    JSObject *foundProto = inspector->commonSetPropFunction(pc, &lastProperty, &commonSetter);
+    JSObject *foundProto = inspector->commonSetPropFunction(pc, &commonSetter);
     if (!foundProto)
         return true;
 
     types::TemporaryTypeSet *objTypes = obj->resultTypeSet();
-    MDefinition *guard = testCommonGetterSetter(objTypes, name, /* isGetter = */ false,
-                                                foundProto, lastProperty);
-    if (!guard)
+    if (!testCommonGetterSetter(objTypes, name, /* isGetter = */ false, foundProto, commonSetter))
         return true;
 
     bool isDOM = objTypes->isDOMClass();

js/src/jit/IonBuilder.h

@@ -763,8 +763,8 @@ class IonBuilder : public MIRGenerator
                                     bool isGetter, JSObject *foundProto);
     void freezePropertiesForCommonPrototype(types::TemporaryTypeSet *types, PropertyName *name,
                                             JSObject *foundProto);
-    MDefinition *testCommonGetterSetter(types::TemporaryTypeSet *types, PropertyName *name,
-                                        bool isGetter, JSObject *foundProto, Shape *lastProperty);
+    bool testCommonGetterSetter(types::TemporaryTypeSet *types, PropertyName *name,
+                                bool isGetter, JSObject *foundProto, JSFunction *function);
     bool testShouldDOMCall(types::TypeSet *inTypes,
                            JSFunction *func, JSJitInfo::OpType opType);
 

js/src/jit/MIR.h

@@ -8489,23 +8489,18 @@ class MSetDOMProperty
 };
 
 class MGetDOMProperty
-  : public MAryInstruction<2>,
+  : public MUnaryInstruction,
     public ObjectPolicy<0>
 {
     const JSJitInfo *info_;
 
   protected:
-    MGetDOMProperty(const JSJitInfo *jitinfo, MDefinition *obj, MDefinition *guard)
-      : info_(jitinfo)
+    MGetDOMProperty(const JSJitInfo *jitinfo, MDefinition *obj)
+      : MUnaryInstruction(obj), info_(jitinfo)
     {
         JS_ASSERT(jitinfo);
         JS_ASSERT(jitinfo->type() == JSJitInfo::Getter);
 
-        setOperand(0, obj);
-
-        // Pin the guard as an operand if we want to hoist later
-        setOperand(1, guard);
-
         // We are movable iff the jitinfo says we can be.
         if (isDomMovable()) {
             JS_ASSERT(jitinfo->aliasSet() != JSJitInfo::AliasEverything);
@@ -8527,10 +8522,9 @@ class MGetDOMProperty
   public:
     INSTRUCTION_HEADER(GetDOMProperty)
 
-    static MGetDOMProperty *New(TempAllocator &alloc, const JSJitInfo *info, MDefinition *obj,
-                                MDefinition *guard)
+    static MGetDOMProperty *New(TempAllocator &alloc, const JSJitInfo *info, MDefinition *obj)
     {
-        return new(alloc) MGetDOMProperty(info, obj, guard);
+        return new(alloc) MGetDOMProperty(info, obj);
     }
 
     const JSJitGetterOp fun() {
@@ -8589,18 +8583,17 @@ class MGetDOMProperty
 class MGetDOMMember : public MGetDOMProperty
 {
     // We inherit everything from MGetDOMProperty except our possiblyCalls value
-    MGetDOMMember(const JSJitInfo *jitinfo, MDefinition *obj, MDefinition *guard)
-        : MGetDOMProperty(jitinfo, obj, guard)
+    MGetDOMMember(const JSJitInfo *jitinfo, MDefinition *obj)
+        : MGetDOMProperty(jitinfo, obj)
     {
     }
 
   public:
     INSTRUCTION_HEADER(GetDOMMember)
 
-    static MGetDOMMember *New(TempAllocator &alloc, const JSJitInfo *info, MDefinition *obj,
-                              MDefinition *guard)
+    static MGetDOMMember *New(TempAllocator &alloc, const JSJitInfo *info, MDefinition *obj)
     {
-        return new(alloc) MGetDOMMember(info, obj, guard);
+        return new(alloc) MGetDOMMember(info, obj);
     }
 
     bool possiblyCalls() const {

js/src/jsinfer.cpp

@@ -1627,6 +1627,47 @@ class ConstraintDataFreezeObjectForTypedArrayBuffer
     }
 };
 
+// Constraint which triggers recompilation when a getter or setter property on
+// an object is redefined.
+class ConstraintDataFreezeObjectForGetterSetter
+{
+    PropertyName *name;
+    bool isGetter;
+    JSFunction *function;
+
+  public:
+    ConstraintDataFreezeObjectForGetterSetter(PropertyName *name, bool isGetter, JSFunction *function)
+      : name(name), isGetter(isGetter), function(function)
+    {}
+
+    const char *kind() { return "freezeObjectForGetterSetter"; }
+
+    bool invalidateOnNewType(Type type) { return false; }
+    bool invalidateOnNewPropertyState(TypeSet *property) { return false; }
+    bool invalidateOnNewObjectState(TypeObject *object) {
+        Shape *shape = object->singleton()->nativeLookupPure(name);
+        if (!shape)
+            return true;
+
+        if (isGetter)
+            return !shape->hasGetterValue() || shape->getterObject() != function;
+        return !shape->hasSetterValue() || shape->setterObject() != function;
+    }
+
+    bool constraintHolds(JSContext *cx,
+                         const HeapTypeSetKey &property, TemporaryTypeSet *expected)
+    {
+        return !invalidateOnNewObjectState(property.object()->maybeType());
+    }
+
+    bool shouldSweep() {
+        // Note: |name| may be used for looking up properties on the object,
+        // but if it becomes dead then the property will have been deleted and
+        // the associated jitcode invalidated.
+        return false;
+    }
+};
+
 } /* anonymous namespace */
 
 void
@@ -1663,6 +1704,19 @@ TypeObjectKey::watchStateChangeForTypedArrayBuffer(CompilerConstraintList *const
                                     ConstraintDataFreezeObjectForTypedArrayBuffer(viewData)));
 }
 
+void
+TypeObjectKey::watchStateChangeForRedefinedProperty(CompilerConstraintList *constraints,
+                                                    PropertyName *name, bool isGetter,
+                                                    JSFunction *function)
+{
+    HeapTypeSetKey objectProperty = property(JSID_EMPTY);
+    LifoAlloc *alloc = constraints->alloc();
+
+    typedef CompilerConstraintInstance<ConstraintDataFreezeObjectForGetterSetter> T;
+    constraints->add(alloc->new_<T>(alloc, objectProperty,
+                                    ConstraintDataFreezeObjectForGetterSetter(name, isGetter, function)));
+}
+
 static void
 ObjectStateChange(ExclusiveContext *cxArg, TypeObject *object, bool markingUnknown)
 {

js/src/jsinfer.h

@@ -1424,6 +1424,8 @@ struct TypeObjectKey
     void watchStateChangeForInlinedCall(CompilerConstraintList *constraints);
     void watchStateChangeForNewScriptTemplate(CompilerConstraintList *constraints);
     void watchStateChangeForTypedArrayBuffer(CompilerConstraintList *constraints);
+    void watchStateChangeForRedefinedProperty(CompilerConstraintList *constraints,
+                                              PropertyName *name, bool isGetter, JSFunction *function);
     HeapTypeSetKey property(jsid id);
     void ensureTrackedProperty(JSContext *cx, jsid id);
 

js/src/vm/Shape.cpp

@@ -866,6 +866,10 @@ JSObject::putProperty(typename ExecutionModeTraits<mode>::ExclusiveContextType c
 
     JS_ASSERT_IF(shape->hasSlot() && !(attrs & JSPROP_SHARED), shape->slot() == slot);
 
+    bool wasGetterOrSetter = shape->hasGetterValue() || shape->hasSetterValue();
+    if (mode == ParallelExecution && wasGetterOrSetter)
+        return nullptr;
+
     if (obj->inDictionaryMode()) {
         /*
          * Updating some property in a dictionary-mode object. Create a new
@@ -935,6 +939,10 @@ JSObject::putProperty(typename ExecutionModeTraits<mode>::ExclusiveContextType c
             ++cx->asJSContext()->runtime()->propertyRemovals;
     }
 
+    // Inform type inference about any getter or setter property being changed.
+    if (wasGetterOrSetter)
+        types::MarkObjectStateChange(cx->asExclusiveContext(), obj);
+
     obj->checkShapeConsistency();
 
     return shape;
@@ -1125,6 +1133,10 @@ JSObject::removeProperty(ExclusiveContext *cx, jsid id_)
         self->removeLastProperty(cx);
     }
 
+    // Inform type inference about any getter or setter property being deleted.
+    if (shape->hasGetterValue() || shape->hasSetterValue())
+        types::MarkObjectStateChange(cx, self);
+
     self->checkShapeConsistency();
     return true;
 }