Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(core,ext): Fix not to be affected by Promise.prototype.then #16326

Merged
merged 18 commits into from
Oct 29, 2022
Merged

fix(core,ext): Fix not to be affected by Promise.prototype.then #16326

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1cee8e1
Fix: Use primordials for Promise
petamoriken Oct 13, 2022
86e64e4
Fix: Avoid using PromiseAll directly
petamoriken Oct 13, 2022
e75a2b0
Merge branch 'main' into fix/promise-then
petamoriken Oct 17, 2022
f8ca1d6
Merge branch 'main' into fix/promise-then
petamoriken Oct 18, 2022
53ed4cc
Merge branch 'main' into fix/promise-then
petamoriken Oct 20, 2022
171405d
Format
petamoriken Oct 20, 2022
5885c73
Merge branch 'main' into fix/promise-then
petamoriken Oct 21, 2022
e05f54f
Update
petamoriken Oct 21, 2022
f277f29
Merge branch 'main' into fix/promise-then
petamoriken Oct 22, 2022
f6088da
Fix
petamoriken Oct 22, 2022
42e36d4
Fix
petamoriken Oct 22, 2022
ea57ca4
Merge branch 'main' into fix/promise-then
petamoriken Oct 23, 2022
b2e5f80
Stop using `SafeArrayIterator`
petamoriken Oct 23, 2022
d0f4a10
Merge branch 'main' into fix/promise-then
petamoriken Oct 25, 2022
73835d8
Add tests
petamoriken Oct 25, 2022
40fe9e9
Format
petamoriken Oct 25, 2022
956725c
Tweak
petamoriken Oct 25, 2022
1997a89
Merge branch 'main' into fix/promise-then
petamoriken Oct 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Fix: Avoid using PromiseAll directly
  • Loading branch information
@petamoriken
petamoriken committed Oct 13, 2022
commit 86e64e42ded9c007a5abd35b3628db5f0c5a10b0
20 changes: 18 additions & 2 deletions core/00_primordials.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,7 @@
ObjectFreeze,
ObjectSetPrototypeOf,
Promise,
PromiseAll,
PromisePrototypeThen,
Set,
SymbolIterator,
Expand Down Expand Up @@ -309,14 +310,17 @@
return SafeIterator;
};

primordials.SafeArrayIterator = createSafeIterator(
const SafeArrayIterator = createSafeIterator(
primordials.ArrayPrototypeSymbolIterator,
primordials.ArrayIteratorPrototypeNext,
);
primordials.SafeStringIterator = createSafeIterator(
primordials.SafeArrayIterator = SafeArrayIterator;

const SafeStringIterator = createSafeIterator(
primordials.StringPrototypeSymbolIterator,
primordials.StringIteratorPrototypeNext,
);
primordials.SafeStringIterator = SafeStringIterator;

const copyProps = (src, dest) => {
ArrayPrototypeForEach(ReflectOwnKeys(src), (key) => {
Expand Down Expand Up @@ -436,6 +440,18 @@
primordials.PromisePrototypeCatch = (thisPromise, onRejected) =>
PromisePrototypeThen(thisPromise, undefined, onRejected);

primordials.SafePromiseAll = (iterable) =>
// Wrapping on a new Promise is necessary to not expose the SafePromise
// prototype to user-land.
Comment on lines +449 to +450
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it unsafe to expose SafePromise.prototype to user-land? I think it can't be modified even if it's exposed because it's frozen. (I might be missing something...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation is referenced from SafePromisePrototypeFinally. I think it is meant to prevent the internal code from being strangely utilized in user-land.

deno/core/00_primordials.js

Lines 439 to 455 in 45ac6e6

/**
* Attaches a callback that is invoked when the Promise is settled (fulfilled or
* rejected). The resolved value cannot be modified from the callback.
* Prefer using async functions when possible.
* @param {Promise<any>} thisPromise
* @param {() => void) | undefined | null} onFinally The callback to execute
* when the Promise is settled (fulfilled or rejected).
* @returns A Promise for the completion of the callback.
*/
primordials.SafePromisePrototypeFinally = (thisPromise, onFinally) =>
// Wrapping on a new Promise is necessary to not expose the SafePromise
// prototype to user-land.
new Promise((a, b) =>
new SafePromise((a, b) => PromisePrototypeThen(thisPromise, a, b))
.finally(onFinally)
.then(a, b)
);

new Promise((a, b) =>
FunctionPrototypeCall(
PromiseAll,
SafePromise,
new SafeArrayIterator(iterable),
)
.then(a, b)
);

/**
* Attaches a callback that is invoked when the Promise is settled (fulfilled or
* rejected). The resolved value cannot be modified from the callback.
Expand Down
4 changes: 2 additions & 2 deletions ext/flash/01_http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
Function,
ObjectPrototypeIsPrototypeOf,
Promise,
PromiseAll,
SafePromiseAll,
PromisePrototypeCatch,
PromisePrototypeThen,
TypedArrayPrototypeSubarray,
Expand Down Expand Up @@ -643,7 +643,7 @@
}, 1000);
}

await PromiseAll([
await SafePromiseAll([
PromisePrototypeCatch(server.serve(), console.error),
serverPromise,
]);
Expand Down
5 changes: 3 additions & 2 deletions ext/web/06_streams.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,14 @@
ObjectPrototypeIsPrototypeOf,
ObjectSetPrototypeOf,
Promise,
PromiseAll,
PromisePrototypeCatch,
PromisePrototypeThen,
PromiseReject,
PromiseResolve,
queueMicrotask,
RangeError,
ReflectHas,
SafePromiseAll,
SharedArrayBuffer,
Symbol,
SymbolAsyncIterator,
Expand Down Expand Up @@ -2282,7 +2282,8 @@
});
}
shutdownWithAction(
() => PromiseAll(ArrayPrototypeMap(actions, (action) => action())),
() =>
SafePromiseAll(ArrayPrototypeMap(actions, (action) => action())),
true,
error,
);
Expand Down
4 changes: 2 additions & 2 deletions ext/webgpu/src/01_webgpu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@
ObjectDefineProperty,
ObjectPrototypeIsPrototypeOf,
Promise,
PromiseAll,
PromisePrototypeCatch,
PromisePrototypeThen,
PromiseReject,
PromiseResolve,
SafeArrayIterator,
SafePromiseAll,
Set,
SetPrototypeEntries,
SetPrototypeForEach,
Expand Down Expand Up @@ -1517,7 +1517,7 @@
"OperationError",
);
}
const operations = PromiseAll(scope.operations);
const operations = SafePromiseAll(scope.operations);
return PromisePrototypeThen(
operations,
() => PromiseResolve(null),
Expand Down
4 changes: 2 additions & 2 deletions runtime/js/40_spawn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
String,
TypeError,
Uint8Array,
PromiseAll,
SafePromiseAll,
PromisePrototypeThen,
SymbolFor,
} = window.__bootstrap.primordials;
Expand Down Expand Up @@ -178,7 +178,7 @@
);
}

const [status, stdout, stderr] = await PromiseAll([
const [status, stdout, stderr] = await SafePromiseAll([
this.#status,
collectOutput(this.#stdout),
collectOutput(this.#stderr),
Expand Down