Skip to content

Commit

Permalink
Reflect.construct can churn cached internalFunctionAllocationStructure
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=263945
rdar://117556294

Reviewed by Keith Miller.
Reviewed by Mark Lam.

Reflect.construct can churn the cached internalFunctionAllocationStructure
when calling the target's prototype getter, causing us to fail a debug assert.
This isn't really a problem though, since accidentally making a second structure
shouldn't break anything (like our watchpoints or structure transition logic).

We just add an extra check to silence the debug assert and be slightly more optimal.

* JSTests/stress/reflect-construct-reenter-prototype-get-different-global.js: Added.
(newTarget):
(get let):
* JSTests/stress/reflect-construct-reenter-prototype-get.js: Added.
(newTarget):
(get let):
* Source/JavaScriptCore/runtime/InternalFunction.cpp:
(JSC::InternalFunction::createSubclassStructure):

Canonical link: https://commits.webkit.org/270084@main
  • Loading branch information
Justin Michaud committed Nov 1, 2023
1 parent 50b8e0f commit ae0b70e
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
var buffer = new ArrayBuffer(3);

var newTarget = function () {
}.bind(null)

let i = 0

let newGlobal = $vm.createGlobalObject()

Object.defineProperty(newTarget, 'prototype', {
get: function () {
let iter = i
++i;
// print(iter)
if (iter >= 1)
return { }
// print("Construct B", iter)
Reflect.construct(newGlobal.Object, [], newTarget);
// print("Construct B Done: ", iter)
return { };
}
});

// print("Construct A")
var result = Reflect.construct(Object, [], newTarget);
23 changes: 23 additions & 0 deletions JSTests/stress/reflect-construct-reenter-prototype-get.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
var buffer = new ArrayBuffer(3);

var newTarget = function () {
}.bind(null)

let i = 0

Object.defineProperty(newTarget, 'prototype', {
get: function () {
let iter = i
++i;
// print(iter)
if (iter >= 1)
return { }
// print("Construct B", iter)
Reflect.construct(Object, [], newTarget);
// print("Construct B Done: ", iter)
return { };
}
});

// print("Construct A")
var result = Reflect.construct(Object, [], newTarget);
8 changes: 7 additions & 1 deletion Source/JavaScriptCore/runtime/InternalFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,12 @@ Structure* InternalFunction::createSubclassStructure(JSGlobalObject* globalObjec
// 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 {
Expand All @@ -161,7 +167,7 @@ Structure* InternalFunction::createSubclassStructure(JSGlobalObject* globalObjec
return baseGlobalObject->structureCache().emptyStructureForPrototypeFromBaseStructure(baseGlobalObject, prototype, baseClass);
}
}

return baseClass;
}

Expand Down

0 comments on commit ae0b70e

Please sign in to comment.