Skip to content

Commit a5f67b2

Browse files
sosukesuzukisosukesuzuki
committed
[JSC] Implment @InternalPromise.@internalAll in C++
https://bugs.webkit.org/show_bug.cgi?id=303965 Reviewed by Yusuke Suzuki. This patch reverts 304888@main to reland 304331@main. The original patch used PromiseAllResolveJob which calls resolve() to settle the result promise. However, resolve() looks up the "then" property on the resolution value, which can have user-observable side effects. For example, if Object.prototype.then is set to a custom function, it would be called when resolving the result array, breaking the InternalPromise pipeline. InternalPromise requires using fulfill() instead of resolve() because fulfill() directly settles the promise without looking up "then". This is the key difference between InternalPromise and regular Promise - InternalPromise must not trigger any user-observable operations. This patch: - Adds InternalPromiseAllResolveJob which uses fulfill() instead of resolve() - Updates JSInternalPromiseConstructor to use InternalPromiseAllResolveJob - Also uses fulfill() for the empty array case - Adds a test to verify Object.prototype.then is not called through module loader --- This patch changes to implement `@InternalPromiseAll.@internalAll` in C++. TipOfTree Patched internal-promise-internal-all 7.2714+-0.6089 ^ 4.1862+-0.2744 ^ definitely 1.7370x faster * JSTests/microbenchmarks/internal-promise-internal-all.js: Added. * JSTests/modules/object-prototype-then-should-not-be-called-through-internal-promise.js: Added. (Object.prototype.then): * JSTests/modules/object-prototype-then-should-not-be-called-through-internal-promise/test.js: Added. * Source/JavaScriptCore/CMakeLists.txt: * Source/JavaScriptCore/DerivedSources-input.xcfilelist: * Source/JavaScriptCore/DerivedSources.make: * Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj: * Source/JavaScriptCore/builtins/InternalPromiseConstructor.js: Removed. * Source/JavaScriptCore/runtime/JSInternalPromiseConstructor.cpp: (JSC::JSC_DEFINE_HOST_FUNCTION): * Source/JavaScriptCore/runtime/JSMicrotask.cpp: (JSC::internalPromiseAllResolveJob): (JSC::runInternalMicrotask): * Source/JavaScriptCore/runtime/Microtask.h: Canonical link: https://commits.webkit.org/304930@main
1 parent 8a8d1c5 commit a5f67b2

11 files changed

Lines changed: 146 additions & 84 deletions

File tree

‎JSTests/microbenchmarks/internal-promise-internal-all.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
2+
const internalPromise = $vm.createBuiltin("(function (executor) { return new @InternalPromise(executor); })")
3+
const internalPromiseAll = $vm.createBuiltin("(function (promises) { return @InternalPromise.internalAll(promises); })");
4+
5+
const promises = [];
6+
for (let i = 0; i < 12; i++)
7+
promises.push(internalPromise((resolve) => resolve(i)));
8+
9+
for (var i = 0; i < 1e4; ++i)
10+
internalPromiseAll(promises);
11+
12+
drainMicrotasks();

‎JSTests/modules/object-prototype-then-should-not-be-called-through-internal-promise.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
import { shouldBe } from "./resources/assert.js";
2+
3+
var thenCalled = false;
4+
Object.prototype.then = function() {
5+
thenCalled = true;
6+
throw new Error("Object.prototype.then should not be called");
7+
};
8+
9+
var originalThen = Promise.prototype.then;
10+
originalThen.call(import("./object-prototype-then-should-not-be-called-through-internal-promise/test.js"), function (namespace) {
11+
shouldBe(namespace.result, 42);
12+
shouldBe(thenCalled, false);
13+
});
14+
drainMicrotasks();
15+
shouldBe(thenCalled, false);

‎JSTests/modules/object-prototype-then-should-not-be-called-through-internal-promise/test.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
export let result = 42;

‎Source/JavaScriptCore/CMakeLists.txt‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,6 @@ set(JavaScriptCore_BUILTINS_SOURCES
350350
${JAVASCRIPTCORE_DIR}/builtins/FunctionPrototype.js
351351
${JAVASCRIPTCORE_DIR}/builtins/GeneratorPrototype.js
352352
${JAVASCRIPTCORE_DIR}/builtins/GlobalOperations.js
353-
${JAVASCRIPTCORE_DIR}/builtins/InternalPromiseConstructor.js
354353
${JAVASCRIPTCORE_DIR}/builtins/IteratorHelpers.js
355354
${JAVASCRIPTCORE_DIR}/builtins/JSIteratorConstructor.js
356355
${JAVASCRIPTCORE_DIR}/builtins/JSIteratorHelperPrototype.js

‎Source/JavaScriptCore/DerivedSources-input.xcfilelist‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ $(PROJECT_DIR)/builtins/DisposableStackPrototype.js
5252
$(PROJECT_DIR)/builtins/FunctionPrototype.js
5353
$(PROJECT_DIR)/builtins/GeneratorPrototype.js
5454
$(PROJECT_DIR)/builtins/GlobalOperations.js
55-
$(PROJECT_DIR)/builtins/InternalPromiseConstructor.js
5655
$(PROJECT_DIR)/builtins/IteratorHelpers.js
5756
$(PROJECT_DIR)/builtins/JSIteratorConstructor.js
5857
$(PROJECT_DIR)/builtins/JSIteratorHelperPrototype.js

‎Source/JavaScriptCore/DerivedSources.make‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,6 @@ JavaScriptCore_BUILTINS_SOURCES = \
112112
$(JavaScriptCore)/builtins/FunctionPrototype.js \
113113
$(JavaScriptCore)/builtins/GeneratorPrototype.js \
114114
$(JavaScriptCore)/builtins/GlobalOperations.js \
115-
$(JavaScriptCore)/builtins/InternalPromiseConstructor.js \
116115
$(JavaScriptCore)/builtins/IteratorHelpers.js \
117116
$(JavaScriptCore)/builtins/JSIteratorConstructor.js \
118117
$(JavaScriptCore)/builtins/JSIteratorHelperPrototype.js \

‎Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5832,7 +5832,6 @@
58325832
E33F507E1B8429A400413856 /* JSInternalPromise.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSInternalPromise.cpp; sourceTree = "<group>"; };
58335833
E33F507F1B8429A400413856 /* JSInternalPromise.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSInternalPromise.h; sourceTree = "<group>"; };
58345834
E33F50861B8449EF00413856 /* JSInternalPromiseConstructor.lut.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSInternalPromiseConstructor.lut.h; sourceTree = "<group>"; };
5835-
E33F50881B844A1A00413856 /* InternalPromiseConstructor.js */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.javascript; path = InternalPromiseConstructor.js; sourceTree = "<group>"; };
58365835
E3400EC022A1CC78009DED54 /* FunctionExecutableInlines.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = FunctionExecutableInlines.h; sourceTree = "<group>"; };
58375836
E3443E3028E9711700C0020C /* ArrayPrototypeInlines.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ArrayPrototypeInlines.h; sourceTree = "<group>"; };
58385837
E348876128F516C600527E5E /* ObjectPrototypeInlines.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ObjectPrototypeInlines.h; sourceTree = "<group>"; };
@@ -10291,7 +10290,6 @@
1029110290
A7A979C418BE8D9E002C3733 /* FunctionPrototype.js */,
1029210291
70B7918F1C0244EC002481E2 /* GeneratorPrototype.js */,
1029310292
A52704851D027C8800354C37 /* GlobalOperations.js */,
10294-
E33F50881B844A1A00413856 /* InternalPromiseConstructor.js */,
1029510293
534D9BF92363C55D0054524D /* IteratorHelpers.js */,
1029610294
8CCBF0662C804EE900EEC20B /* JSIteratorConstructor.js */,
1029710295
8AA92D3D2CACA137002D872B /* JSIteratorHelperPrototype.js */,

‎Source/JavaScriptCore/builtins/InternalPromiseConstructor.js‎

Lines changed: 0 additions & 78 deletions
This file was deleted.

‎Source/JavaScriptCore/runtime/JSInternalPromiseConstructor.cpp‎

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,21 @@
2727
#include "JSInternalPromiseConstructor.h"
2828

2929
#include "JSCBuiltins.h"
30+
#include "JSCInlines.h"
31+
#include "JSInternalPromise.h"
3032
#include "JSInternalPromisePrototype.h"
3133
#include "JSObjectInlines.h"
34+
#include "JSPromiseCombinatorsContext.h"
35+
#include "JSPromiseCombinatorsGlobalContext.h"
36+
#include "Microtask.h"
3237
#include "StructureInlines.h"
3338

39+
namespace JSC {
40+
41+
static JSC_DECLARE_HOST_FUNCTION(internalPromiseConstructorFuncInternalAll);
42+
43+
}
44+
3445
#include "JSInternalPromiseConstructor.lut.h"
3546

3647
namespace JSC {
@@ -41,7 +52,7 @@ const ClassInfo JSInternalPromiseConstructor::s_info = { "Function"_s, &Base::s_
4152

4253
/* Source for JSInternalPromiseConstructor.lut.h
4354
@begin internalPromiseConstructorTable
44-
internalAll JSBuiltin DontEnum|Function 1
55+
internalAll internalPromiseConstructorFuncInternalAll DontEnum|Function 1
4556
@end
4657
*/
4758

@@ -64,4 +75,65 @@ JSInternalPromiseConstructor::JSInternalPromiseConstructor(VM& vm, FunctionExecu
6475
{
6576
}
6677

78+
// InternalPromise.internalAll(array)
79+
// This function is intended to be used in the JSC internals.
80+
// The implementation should take care not to perform the user observable / trappable operations.
81+
// 1. Don't use for-of and iterables. This function only accepts the dense array of the promises.
82+
// 2. Don't look up this.constructor / @@species. Always construct the plain InternalPromise object.
83+
JSC_DEFINE_HOST_FUNCTION(internalPromiseConstructorFuncInternalAll, (JSGlobalObject* globalObject, CallFrame* callFrame))
84+
{
85+
VM& vm = globalObject->vm();
86+
auto scope = DECLARE_THROW_SCOPE(vm);
87+
88+
auto* promise = JSInternalPromise::create(vm, globalObject->internalPromiseStructure());
89+
90+
auto callReject = [&]() -> void {
91+
Exception* exception = scope.exception();
92+
ASSERT(exception);
93+
scope.clearException();
94+
scope.release();
95+
promise->reject(vm, globalObject, exception);
96+
};
97+
98+
JSValue arrayValue = callFrame->argument(0);
99+
JSArray* array = jsDynamicCast<JSArray*>(arrayValue);
100+
ASSERT(array);
101+
102+
uint64_t length = array->length();
103+
if (!length) {
104+
JSArray* values = JSArray::tryCreate(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous), 0);
105+
if (!values) [[unlikely]] {
106+
throwOutOfMemoryError(globalObject, scope);
107+
callReject();
108+
return JSValue::encode(promise);
109+
}
110+
scope.release();
111+
// Use fulfill instead of resolve to avoid looking up the then property.
112+
promise->fulfill(vm, globalObject, values);
113+
return JSValue::encode(promise);
114+
}
115+
116+
JSArray* values = JSArray::tryCreate(vm, globalObject->arrayStructureForIndexingTypeDuringAllocation(ArrayWithContiguous), length);
117+
if (!values) [[unlikely]] {
118+
throwOutOfMemoryError(globalObject, scope);
119+
callReject();
120+
return JSValue::encode(promise);
121+
}
122+
123+
JSPromiseCombinatorsGlobalContext* globalContext = JSPromiseCombinatorsGlobalContext::create(vm, promise, values, jsNumber(length));
124+
for (unsigned index = 0; index < length; ++index) {
125+
JSValue value = array->getIndex(globalObject, index);
126+
RETURN_IF_EXCEPTION(scope, { });
127+
128+
auto* nextPromise = jsCast<JSInternalPromise*>(value);
129+
ASSERT(nextPromise);
130+
JSPromiseCombinatorsContext* context = JSPromiseCombinatorsContext::create(vm, globalContext, index);
131+
132+
nextPromise->performPromiseThenWithInternalMicrotask(vm, globalObject, InternalMicrotask::InternalPromiseAllResolveJob, promise, context);
133+
RETURN_IF_EXCEPTION(scope, { });
134+
}
135+
136+
return JSValue::encode(promise);
137+
}
138+
67139
} // namespace JSC

‎Source/JavaScriptCore/runtime/JSMicrotask.cpp‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,46 @@ static void promiseAllResolveJob(JSGlobalObject* globalObject, VM& vm, JSPromise
229229
}
230230
}
231231

232+
// This is similar to promiseAllResolveJob but uses fulfill instead of resolve.
233+
// This is used for InternalPromise.internalAll to avoid looking up the then property
234+
// which could have user-observable side effects.
235+
static void internalPromiseAllResolveJob(JSGlobalObject* globalObject, VM& vm, JSPromise* promise, JSValue resolution, JSPromiseCombinatorsContext* context, JSPromise::Status status)
236+
{
237+
auto scope = DECLARE_THROW_SCOPE(vm);
238+
239+
auto* globalContext = jsCast<JSPromiseCombinatorsGlobalContext*>(context->globalContext());
240+
switch (status) {
241+
case JSPromise::Status::Pending: {
242+
RELEASE_ASSERT_NOT_REACHED();
243+
break;
244+
}
245+
case JSPromise::Status::Fulfilled: {
246+
auto* values = jsCast<JSArray*>(globalContext->values());
247+
uint64_t index = context->index();
248+
249+
values->putDirectIndex(globalObject, index, resolution);
250+
RETURN_IF_EXCEPTION(scope, void());
251+
252+
uint64_t count = globalContext->remainingElementsCount().toIndex(globalObject, "count exceeds size"_s);
253+
RETURN_IF_EXCEPTION(scope, void());
254+
255+
--count;
256+
globalContext->setRemainingElementsCount(vm, jsNumber(count));
257+
if (!count) {
258+
scope.release();
259+
// Use fulfill instead of resolve to avoid looking up the then property.
260+
promise->fulfill(vm, globalObject, values);
261+
}
262+
break;
263+
}
264+
case JSPromise::Status::Rejected: {
265+
scope.release();
266+
promise->reject(vm, globalObject, resolution);
267+
break;
268+
}
269+
}
270+
}
271+
232272
static void promiseAllSettledResolveJob(JSGlobalObject* globalObject, VM& vm, JSPromise* promise, JSValue resolution, JSPromiseCombinatorsContext* context, JSPromise::Status status)
233273
{
234274
auto scope = DECLARE_THROW_SCOPE(vm);
@@ -427,6 +467,9 @@ void runInternalMicrotask(JSGlobalObject* globalObject, InternalMicrotask task,
427467
case InternalMicrotask::PromiseAnyResolveJob:
428468
RELEASE_AND_RETURN(scope, promiseAnyResolveJob(globalObject, vm, jsCast<JSPromise*>(arguments[0]), arguments[1], jsCast<JSPromiseCombinatorsContext*>(arguments[3]), static_cast<JSPromise::Status>(arguments[2].asInt32())));
429469

470+
case InternalMicrotask::InternalPromiseAllResolveJob:
471+
RELEASE_AND_RETURN(scope, internalPromiseAllResolveJob(globalObject, vm, jsCast<JSPromise*>(arguments[0]), arguments[1], jsCast<JSPromiseCombinatorsContext*>(arguments[3]), static_cast<JSPromise::Status>(arguments[2].asInt32())));
472+
430473
case InternalMicrotask::PromiseReactionJob: {
431474
JSValue promiseOrCapability = arguments[0];
432475
JSValue handler = arguments[1];

0 commit comments

Comments
 (0)