Changing a JSFunction's prototype property should clear allocation ca…

…ches

https://bugs.webkit.org/show_bug.cgi?id=270302
rdar://121657868

Reviewed by Alexey Shvayka and Yusuke Suzuki.

Right now we only clear the allocation watchpoint if a JSFunction `mayHaveNonReifiedPrototype()` when setting
the .prototype property. This is semantically incorrect in the case of `new.target` bound functions because we will cache
the wrong value.

This patch makes it so we always file the allocation profile watchpoint when turning either of the allocation profiles.
When turning the ObjectAllocationProfile (used by op_create_this) we assert the watchpoint has already been fired as it
should've already happened when the new .prototype value was set. When turning the InternalFunctionAllocationProfile (used
by createSubclassStructure when subclassing InternalFunction/Reflect.construct) its possible to pass the same JSFunction
to two different InternalFunctions, which will turn the profile.

* JSTests/stress/bound-constructor-change-prototype-clears-cache.js: Added.
(empty):
(test1.const.newTarget):
(test1):
(test2.const.newTarget):
(test2.Opt):
(test2):
(test3.const.newTarget):
(main):
* JSTests/stress/put-prototype-to-normal-function-shouldnt-be-cached.js: Added.
(opt):
(main.target):
(main):
* Source/JavaScriptCore/bytecode/InternalFunctionAllocationProfile.h:
(JSC::InternalFunctionAllocationProfile::createAllocationStructureFromBase):
* Source/JavaScriptCore/bytecode/ObjectAllocationProfileInlines.h:
(JSC::ObjectAllocationProfileBase<Derived>::initializeProfile):
* Source/JavaScriptCore/dfg/DFGOperations.cpp:
(JSC::DFG::JSC_DEFINE_JIT_OPERATION):
* Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:
(JSC::JSC_DEFINE_COMMON_SLOW_PATH):
(JSC::createInternalFieldObject):
* Source/JavaScriptCore/runtime/FunctionRareData.h:
* Source/JavaScriptCore/runtime/InternalFunction.cpp:
(JSC::InternalFunction::createSubclassStructure):
* Source/JavaScriptCore/runtime/JSFunction.cpp:
(JSC::JSFunction::prototypeForConstruction):
(JSC::JSFunction::allocateAndInitializeRareData):
(JSC::JSFunction::initializeRareData):
(JSC::JSFunction::put):
(JSC::JSFunction::defineOwnProperty):
* Source/JavaScriptCore/runtime/JSFunction.h:
* Source/JavaScriptCore/runtime/JSFunctionInlines.h:
(JSC::JSFunction::canUseAllocationProfiles):
(JSC::JSFunction::ensureRareDataAndObjectAllocationProfile):
(JSC::JSFunction::canUseAllocationProfile): Deleted.
(JSC::JSFunction::ensureRareDataAndAllocationProfile): Deleted.

Originally-landed-as: 272448.699@safari-7618-branch (96283e8). rdar://128089585
Canonical link: https://commits.webkit.org/278869@main

JSTests/stress/bound-constructor-change-prototype-clears-cache.js

@@ -0,0 +1,73 @@
+function empty() {
+
+}
+
+function test1() {
+    const newTarget = (function () {}).bind();
+    newTarget.prototype = Object.prototype;
+
+    const a = Reflect.construct(Promise, [empty], newTarget);
+
+    newTarget.prototype = Array.prototype;
+
+    const b = Reflect.construct(Promise, [empty], newTarget);
+
+    if (a.__proto__ === b.__proto__)
+        throw new Error('They should be different.');
+}
+
+function test2() {
+    const newTarget = (function () {}).bind();
+    newTarget.prototype = Object.prototype;
+
+    const newTargetWrapper = Function.prototype.apply;
+    newTargetWrapper.prototype = newTarget;
+
+    class Opt extends Promise {
+        constructor() {
+            newTargetWrapper.prototype = new.target;
+            empty instanceof newTargetWrapper;
+
+            super(empty);
+        }
+    }
+
+    for (let i = 0; i < 200000; i++) {
+        Reflect.construct(Opt, [], newTarget);
+    }
+
+    const a = Reflect.construct(Opt, [], newTarget);
+
+    newTarget.prototype = Array.prototype;
+    Reflect.construct(Object, [], newTarget);
+
+    const b = Reflect.construct(Opt, [], newTarget);
+
+    if (a.__proto__ === b.__proto__)
+        throw new Error('They should be different.');
+}
+
+function test3() {
+    const newTarget = (function () {}).bind();
+    let prototype = Object.prototype;
+    Object.defineProperty(newTarget, "prototype", { get() { return prototype; }});
+
+    const a = Reflect.construct(Promise, [empty], newTarget);
+
+    prototype = Array.prototype;
+
+    const b = Reflect.construct(Promise, [empty], newTarget);
+
+    if (a.__proto__ == b.__proto__)
+        throw new Error('They should be different.');
+}
+
+function main() {
+    test1();
+
+    test2();
+
+    test3();
+}
+
+main();

JSTests/stress/put-prototype-to-normal-function-shouldnt-be-cached.js

@@ -0,0 +1,38 @@
+function opt(object, value) {
+    object.prototype = value;
+}
+
+function main() {
+    Function.prototype.__proto__ = new Proxy({}, {});
+
+    const object = {
+        nonConstructor() { }
+    };
+
+    function target() { }
+
+    Function.prototype.__proto__ = Object.prototype;
+
+    const nonConstructor = object.nonConstructor;
+
+    Object.defineProperty(nonConstructor, 'prototype', {
+        configurable: false,
+        enumerable: false,
+        writable: true,
+        value: {}
+    });
+
+    target.prototype = {};
+    Reflect.construct(Object, [], target);
+
+    opt(nonConstructor, {});
+    const newPrototype = {};
+    opt(target, newPrototype);
+
+    const result = Reflect.construct(Object, [], target);
+
+    if (result.__proto__ !== newPrototype)
+        throw new Error("Rare data of result was not cleared.");
+}
+
+main();

Source/JavaScriptCore/bytecode/InternalFunctionAllocationProfile.h

@@ -36,7 +36,7 @@ class InternalFunctionAllocationProfile {
     static inline ptrdiff_t offsetOfStructureID() { return OBJECT_OFFSETOF(InternalFunctionAllocationProfile, m_structureID); }
 
     Structure* structure() { return m_structureID.get(); }
-    Structure* createAllocationStructureFromBase(VM&, JSGlobalObject*, JSCell* owner, JSObject* prototype, Structure* base);
+    Structure* createAllocationStructureFromBase(VM&, JSGlobalObject*, JSCell* owner, JSObject* prototype, Structure* base, InlineWatchpointSet&);
 
     void clear() { m_structureID.clear(); }
     template<typename Visitor> void visitAggregate(Visitor& visitor) { visitor.append(m_structureID); }
@@ -45,7 +45,7 @@ class InternalFunctionAllocationProfile {
     WriteBarrierStructureID m_structureID;
 };
 
-inline Structure* InternalFunctionAllocationProfile::createAllocationStructureFromBase(VM& vm, JSGlobalObject* baseGlobalObject, JSCell* owner, JSObject* prototype, Structure* baseStructure)
+inline Structure* InternalFunctionAllocationProfile::createAllocationStructureFromBase(VM& vm, JSGlobalObject* baseGlobalObject, JSCell* owner, JSObject* prototype, Structure* baseStructure, InlineWatchpointSet& watchpointSet)
 {
     ASSERT(!m_structureID || m_structureID.get()->classInfoForCells() != baseStructure->classInfoForCells() || m_structureID->globalObject() != baseStructure->globalObject());
     ASSERT(baseStructure->hasMonoProto());
@@ -61,6 +61,13 @@ inline Structure* InternalFunctionAllocationProfile::createAllocationStructureFr
     // Ensure that if another thread sees the structure, it will see it properly created.
     WTF::storeStoreFence();
 
+    // It's possible to get here because some JSFunction got passed to two different InternalFunctions. e.g.
+    // function Foo() { }
+    // Reflect.construct(Promise, [], Foo);
+    // Reflect.construct(Int8Array, [], Foo);
+    if (UNLIKELY(m_structureID && m_structureID.value() != structure->id()))
+        watchpointSet.fireAll(vm, "InternalFunctionAllocationProfile rotated to a new structure");
+
     m_structureID.set(vm, owner, structure);
     return structure;
 }

Source/JavaScriptCore/bytecode/ObjectAllocationProfileInlines.h

@@ -135,6 +135,12 @@ ALWAYS_INLINE void ObjectAllocationProfileBase<Derived>::initializeProfile(VM& v
     // Ensure that if another thread sees the structure and prototype, it will see it properly created.
     WTF::storeStoreFence();
 
+    // The watchpoint should have been fired already but it's prudent to be safe here.
+    if (UNLIKELY(functionRareData && m_structure && m_structure.get() != structure)) {
+        ASSERT(functionRareData->allocationProfileWatchpointSet().hasBeenInvalidated());
+        functionRareData->allocationProfileWatchpointSet().fireAll(vm, "Clearing to be safe because structure has changed");
+    }
+
     m_structure.set(vm, owner, structure);
     static_cast<Derived*>(this)->setPrototype(vm, owner, prototype);
 }

Source/JavaScriptCore/dfg/DFGOperations.cpp

@@ -413,22 +413,19 @@ JSC_DEFINE_JIT_OPERATION(operationCreateThis, JSCell*, (JSGlobalObject* globalOb
     CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
     JITOperationPrologueCallFrameTracer tracer(vm, callFrame);
     auto scope = DECLARE_THROW_SCOPE(vm);
-    if (constructor->type() == JSFunctionType && jsCast<JSFunction*>(constructor)->canUseAllocationProfile()) {
-        JSObject* result;
-        {
-            DeferTermination deferScope(vm);
-            auto rareData = jsCast<JSFunction*>(constructor)->ensureRareDataAndAllocationProfile(globalObject, inlineCapacity);
-            scope.releaseAssertNoException();
-            ObjectAllocationProfileWithPrototype* allocationProfile = rareData->objectAllocationProfile();
-            Structure* structure = allocationProfile->structure();
-                result = constructEmptyObject(vm, structure);
-            if (structure->hasPolyProto()) {
-                JSObject* prototype = allocationProfile->prototype();
-                ASSERT(prototype == jsCast<JSFunction*>(constructor)->prototypeForConstruction(vm, globalObject));
-                result->putDirectOffset(vm, knownPolyProtoOffset, prototype);
-                prototype->didBecomePrototype(vm);
-                ASSERT_WITH_MESSAGE(!hasIndexedProperties(result->indexingType()), "We rely on JSFinalObject not starting out with an indexing type otherwise we would potentially need to convert to slow put storage");
-            }
+    if (constructor->type() == JSFunctionType && jsCast<JSFunction*>(constructor)->canUseAllocationProfiles()) {
+        DeferTermination deferScope(vm);
+        auto rareData = jsCast<JSFunction*>(constructor)->ensureRareDataAndObjectAllocationProfile(globalObject, inlineCapacity);
+        scope.releaseAssertNoException();
+        ObjectAllocationProfileWithPrototype* allocationProfile = rareData->objectAllocationProfile();
+        Structure* structure = allocationProfile->structure();
+        JSObject* result = constructEmptyObject(vm, structure);
+        if (structure->hasPolyProto()) {
+            JSObject* prototype = allocationProfile->prototype();
+            ASSERT(prototype == jsCast<JSFunction*>(constructor)->prototypeForConstruction(vm, globalObject));
+            result->putDirectOffset(vm, knownPolyProtoOffset, prototype);
+            prototype->didBecomePrototype(vm);
+            ASSERT_WITH_MESSAGE(!hasIndexedProperties(result->indexingType()), "We rely on JSFinalObject not starting out with an indexing type otherwise we would potentially need to convert to slow put storage");
         }
         OPERATION_RETURN(scope, result);
     }

Source/JavaScriptCore/runtime/CommonSlowPaths.cpp

@@ -152,15 +152,15 @@ JSC_DEFINE_COMMON_SLOW_PATH(slow_path_create_this)
     JSObject* result;
     JSObject* constructorAsObject = asObject(GET(bytecode.m_callee).jsValue());
     JSFunction* constructor = jsDynamicCast<JSFunction*>(constructorAsObject);
-    if (constructor && constructor->canUseAllocationProfile()) {
+    if (constructor && constructor->canUseAllocationProfiles()) {
         WriteBarrier<JSCell>& cachedCallee = bytecode.metadata(codeBlock).m_cachedCallee;
         if (!cachedCallee)
             cachedCallee.set(vm, codeBlock, constructor);
         else if (cachedCallee.unvalidatedGet() != JSCell::seenMultipleCalleeObjects() && cachedCallee.get() != constructor)
             cachedCallee.setWithoutWriteBarrier(JSCell::seenMultipleCalleeObjects());
 
         size_t inlineCapacity = bytecode.m_inlineCapacity;
-        ObjectAllocationProfileWithPrototype* allocationProfile = constructor->ensureRareDataAndAllocationProfile(globalObject, inlineCapacity)->objectAllocationProfile();
+        ObjectAllocationProfileWithPrototype* allocationProfile = constructor->ensureRareDataAndObjectAllocationProfile(globalObject, inlineCapacity)->objectAllocationProfile();
         CHECK_EXCEPTION();
         Structure* structure = allocationProfile->structure();
         result = constructEmptyObject(vm, structure);
@@ -204,7 +204,7 @@ JSC_DEFINE_COMMON_SLOW_PATH(slow_path_create_promise)
     }
 
     JSFunction* constructor = jsDynamicCast<JSFunction*>(constructorAsObject);
-    if (constructor && constructor->canUseAllocationProfile()) {
+    if (constructor && constructor->canUseAllocationProfiles()) {
         WriteBarrier<JSCell>& cachedCallee = bytecode.metadata(codeBlock).m_cachedCallee;
         if (!cachedCallee)
             cachedCallee.set(vm, codeBlock, constructor);
@@ -236,7 +236,7 @@ static JSClass* createInternalFieldObject(JSGlobalObject* globalObject, VM& vm,
     JSClass* result = JSClass::create(vm, structure);
 
     JSFunction* constructor = jsDynamicCast<JSFunction*>(constructorAsObject);
-    if (constructor && constructor->canUseAllocationProfile()) {
+    if (constructor && constructor->canUseAllocationProfiles()) {
         WriteBarrier<JSCell>& cachedCallee = bytecode.metadata(codeBlock).m_cachedCallee;
         if (!cachedCallee)
             cachedCallee.set(vm, codeBlock, constructor);

Source/JavaScriptCore/runtime/FunctionRareData.h

@@ -97,7 +97,7 @@ class FunctionRareData final : public JSCell {
     Structure* createInternalFunctionAllocationStructureFromBase(VM& vm, JSGlobalObject* baseGlobalObject, JSObject* prototype, Structure* baseStructure)
     {
         initializeAllocationProfileWatchpointSet();
-        return m_internalFunctionAllocationProfile.createAllocationStructureFromBase(vm, baseGlobalObject, this, prototype, baseStructure);
+        return m_internalFunctionAllocationProfile.createAllocationStructureFromBase(vm, baseGlobalObject, this, prototype, baseStructure, allocationProfileWatchpointSet());
     }
     void clearInternalFunctionAllocationProfile(const char* reason)
     {

Source/JavaScriptCore/runtime/InternalFunction.cpp

@@ -141,33 +141,29 @@ Structure* InternalFunction::createSubclassStructure(JSGlobalObject* globalObjec
     // newTarget may be an InternalFunction if we were called from Reflect.construct.
     JSFunction* targetFunction = jsDynamicCast<JSFunction*>(newTarget);
 
-    if (LIKELY(targetFunction)) {
-        FunctionRareData* rareData = targetFunction->ensureRareData(vm);
-        Structure* structure = rareData->internalFunctionAllocationStructure();
-        if (LIKELY(structure && structure->classInfoForCells() == baseClass->classInfoForCells() && structure->globalObject() == baseGlobalObject))
-            return structure;
-
-        // Note, Reflect.construct might cause the profile to churn but we don't care.
-        JSValue prototypeValue = targetFunction->get(globalObject, vm.propertyNames->prototype);
-        RETURN_IF_EXCEPTION(scope, nullptr);
-
-        // While very unlikely, if the profile churns becauase of the prototype getter, it would be nice to use the updated profile instead of creating a new one.
-        structure = rareData->internalFunctionAllocationStructure();
-        if (UNLIKELY(structure && structure->classInfoForCells() == baseClass->classInfoForCells() && structure->globalObject() == baseGlobalObject))
-            return structure;
-
-        if (JSObject* prototype = jsDynamicCast<JSObject*>(prototypeValue))
-            return rareData->createInternalFunctionAllocationStructureFromBase(vm, baseGlobalObject, prototype, baseClass);
-    } else {
+    if (UNLIKELY(!targetFunction || !targetFunction->canUseAllocationProfiles())) {
         JSValue prototypeValue = newTarget->get(globalObject, vm.propertyNames->prototype);
         RETURN_IF_EXCEPTION(scope, nullptr);
         if (JSObject* prototype = jsDynamicCast<JSObject*>(prototypeValue)) {
-            // This only happens if someone Reflect.constructs our builtin constructor with another builtin constructor as the new.target.
-            // Thus, we don't care about the cost of looking up the structure from our hash table every time.
+            // This only happens if someone Reflect.constructs our builtin constructor with another builtin constructor or weird .prototype property on a
+            // JSFunction as the new.target. Thus, we don't care about the cost of looking up the structure from our hash table every time.
             return baseGlobalObject->structureCache().emptyStructureForPrototypeFromBaseStructure(baseGlobalObject, prototype, baseClass);
         }
+        return baseClass;
     }
 
+    FunctionRareData* rareData = targetFunction->ensureRareData(vm);
+    Structure* structure = rareData->internalFunctionAllocationStructure();
+    if (LIKELY(structure && structure->classInfoForCells() == baseClass->classInfoForCells() && structure->globalObject() == baseGlobalObject))
+        return structure;
+
+    // .prototype can't be a getter if we canUseAllocationProfiles().
+    JSValue prototypeValue = targetFunction->get(globalObject, vm.propertyNames->prototype);
+    scope.assertNoException();
+
+    if (JSObject* prototype = jsDynamicCast<JSObject*>(prototypeValue))
+        return rareData->createInternalFunctionAllocationStructureFromBase(vm, baseGlobalObject, prototype, baseClass);
+
     return baseClass;
 }
 

Source/JavaScriptCore/runtime/JSFunction.cpp

@@ -154,7 +154,7 @@ JSObject* JSFunction::prototypeForConstruction(VM& vm, JSGlobalObject* globalObj
 {
     // This code assumes getting the prototype is not effectful. That's only
     // true when we can use the allocation profile.
-    ASSERT(canUseAllocationProfile());
+    ASSERT(canUseAllocationProfiles());
     DeferTermination deferScope(vm);
     auto scope = DECLARE_CATCH_SCOPE(vm);
     JSValue prototype = get(globalObject, vm.propertyNames->prototype);
@@ -178,7 +178,7 @@ FunctionRareData* JSFunction::allocateAndInitializeRareData(JSGlobalObject* glob
 {
     uintptr_t executableOrRareData = m_executableOrRareData;
     ASSERT(!(executableOrRareData & rareDataTag));
-    ASSERT(canUseAllocationProfile());
+    ASSERT(canUseAllocationProfiles());
     VM& vm = globalObject->vm();
     JSObject* prototype = prototypeForConstruction(vm, globalObject);
     FunctionRareData* rareData = FunctionRareData::create(vm, bitwise_cast<ExecutableBase*>(executableOrRareData));
@@ -199,7 +199,7 @@ FunctionRareData* JSFunction::initializeRareData(JSGlobalObject* globalObject, s
 {
     uintptr_t executableOrRareData = m_executableOrRareData;
     ASSERT(executableOrRareData & rareDataTag);
-    ASSERT(canUseAllocationProfile());
+    ASSERT(canUseAllocationProfiles());
     VM& vm = globalObject->vm();
     JSObject* prototype = prototypeForConstruction(vm, globalObject);
     FunctionRareData* rareData = bitwise_cast<FunctionRareData*>(executableOrRareData & ~rareDataTag);
@@ -389,19 +389,21 @@ bool JSFunction::put(JSCell* cell, JSGlobalObject* globalObject, PropertyName pr
 
     JSFunction* thisObject = jsCast<JSFunction*>(cell);
 
-    if (propertyName == vm.propertyNames->prototype && thisObject->mayHaveNonReifiedPrototype()) {
+    if (propertyName == vm.propertyNames->prototype) {
         slot.disableCaching();
         if (FunctionRareData* rareData = thisObject->rareData())
             rareData->clear("Store to prototype property of a function");
-        if (!isValidOffset(thisObject->getDirectOffset(vm, propertyName))) {
-            // For class constructors, prototype object is initialized from bytecode via defineOwnProperty().
-            ASSERT(!thisObject->jsExecutable()->isClassConstructorFunction());
-            if (UNLIKELY(slot.thisValue() != thisObject))
-                RELEASE_AND_RETURN(scope, JSObject::definePropertyOnReceiver(globalObject, propertyName, value, slot));
-            thisObject->putDirect(vm, propertyName, value, prototypeAttributesForNonClass);
-            return true;
+        if (thisObject->mayHaveNonReifiedPrototype()) {
+            if (!isValidOffset(thisObject->getDirectOffset(vm, propertyName))) {
+                // For class constructors, prototype object is initialized from bytecode via defineOwnProperty().
+                ASSERT(!thisObject->jsExecutable()->isClassConstructorFunction());
+                if (UNLIKELY(slot.thisValue() != thisObject))
+                    RELEASE_AND_RETURN(scope, JSObject::definePropertyOnReceiver(globalObject, propertyName, value, slot));
+                thisObject->putDirect(vm, propertyName, value, prototypeAttributesForNonClass);
+                return true;
+            }
+            RELEASE_AND_RETURN(scope, Base::put(thisObject, globalObject, propertyName, value, slot));
         }
-        RELEASE_AND_RETURN(scope, Base::put(thisObject, globalObject, propertyName, value, slot));
     }
 
     PropertyStatus propertyType = thisObject->reifyLazyPropertyIfNeeded<>(vm, globalObject, propertyName);
@@ -431,9 +433,12 @@ bool JSFunction::defineOwnProperty(JSObject* object, JSGlobalObject* globalObjec
     JSFunction* thisObject = jsCast<JSFunction*>(object);
 
 
-    if (propertyName == vm.propertyNames->prototype && thisObject->mayHaveNonReifiedPrototype()) {
+    if (propertyName == vm.propertyNames->prototype) {
         if (FunctionRareData* rareData = thisObject->rareData())
             rareData->clear("Store to prototype property of a function");
+    }
+
+    if (propertyName == vm.propertyNames->prototype && thisObject->mayHaveNonReifiedPrototype()) {
         if (!isValidOffset(thisObject->getDirectOffset(vm, propertyName))) {
             if (thisObject->jsExecutable()->isClassConstructorFunction()) {
                 // Fast path for prototype object initialization from bytecode that avoids calling into getOwnPropertySlot().

Source/JavaScriptCore/runtime/JSFunction.h

@@ -135,7 +135,7 @@ class JSFunction : public JSCallee {
         return bitwise_cast<FunctionRareData*>(executableOrRareData & ~rareDataTag);
     }
 
-    FunctionRareData* ensureRareDataAndAllocationProfile(JSGlobalObject*, unsigned inlineCapacity);
+    FunctionRareData* ensureRareDataAndObjectAllocationProfile(JSGlobalObject*, unsigned inlineCapacity);
 
     FunctionRareData* rareData() const
     {
@@ -156,8 +156,7 @@ class JSFunction : public JSCallee {
     // Returns the __proto__ for the |this| value if this JSFunction were to be constructed.
     JSObject* prototypeForConstruction(VM&, JSGlobalObject*);
 
-    bool canUseAllocationProfile();
-    bool canUseAllocationProfileNonInline();
+    bool canUseAllocationProfiles();
 
     enum class PropertyStatus {
         Eager,