-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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.
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
feat(ext/web): use ArrayBuffer.was_detached() #16307
Changes from all commits
713e2f4
d5e8c42
8b1eba0
e8bbc48
3574152
d361155
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,9 +26,6 @@ | |||||||||||||||||||||||||||||||||||||||||||||
SymbolFor, | ||||||||||||||||||||||||||||||||||||||||||||||
SymbolIterator, | ||||||||||||||||||||||||||||||||||||||||||||||
TypeError, | ||||||||||||||||||||||||||||||||||||||||||||||
WeakSet, | ||||||||||||||||||||||||||||||||||||||||||||||
WeakSetPrototypeAdd, | ||||||||||||||||||||||||||||||||||||||||||||||
WeakSetPrototypeHas, | ||||||||||||||||||||||||||||||||||||||||||||||
} = window.__bootstrap.primordials; | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
class MessageChannel { | ||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -239,30 +236,25 @@ | |||||||||||||||||||||||||||||||||||||||||||||
return [data, transferables]; | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
const detachedArrayBuffers = new WeakSet(); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||
* @param {any} data | ||||||||||||||||||||||||||||||||||||||||||||||
* @param {object[]} transferables | ||||||||||||||||||||||||||||||||||||||||||||||
* @returns {globalThis.__bootstrap.messagePort.MessageData} | ||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||
function serializeJsMessageData(data, transferables) { | ||||||||||||||||||||||||||||||||||||||||||||||
const transferredArrayBuffers = ArrayPrototypeFilter( | ||||||||||||||||||||||||||||||||||||||||||||||
transferables, | ||||||||||||||||||||||||||||||||||||||||||||||
(a) => ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, a), | ||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
for (const arrayBuffer of transferredArrayBuffers) { | ||||||||||||||||||||||||||||||||||||||||||||||
// This is hacky with both false positives and false negatives for | ||||||||||||||||||||||||||||||||||||||||||||||
// detecting detached array buffers. V8 needs to add a way to tell if a | ||||||||||||||||||||||||||||||||||||||||||||||
// buffer is detached or not. | ||||||||||||||||||||||||||||||||||||||||||||||
if (WeakSetPrototypeHas(detachedArrayBuffers, arrayBuffer)) { | ||||||||||||||||||||||||||||||||||||||||||||||
throw new DOMException( | ||||||||||||||||||||||||||||||||||||||||||||||
"Can not transfer detached ArrayBuffer", | ||||||||||||||||||||||||||||||||||||||||||||||
"DataCloneError", | ||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||
const transferredArrayBuffers = []; | ||||||||||||||||||||||||||||||||||||||||||||||
for (let i = 0, j = 0; i < transferables.length; i++) { | ||||||||||||||||||||||||||||||||||||||||||||||
const ab = transferables[i]; | ||||||||||||||||||||||||||||||||||||||||||||||
if (ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, ab)) { | ||||||||||||||||||||||||||||||||||||||||||||||
if (ab.byteLength === 0 && core.ops.op_arraybuffer_was_detached(ab)) { | ||||||||||||||||||||||||||||||||||||||||||||||
throw new DOMException( | ||||||||||||||||||||||||||||||||||||||||||||||
`ArrayBuffer at index ${j} is already detached`, | ||||||||||||||||||||||||||||||||||||||||||||||
"DataCloneError", | ||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
j++; | ||||||||||||||||||||||||||||||||||||||||||||||
transferredArrayBuffers.push(ab); | ||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+246
to
+256
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
The user doesn't care about the index of the AB, they care about the index in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lucacasonato Wanted to mimic Chrome error message.
Also did it that way to make JS error be the same as Rust's where we're looping through AB list, not Line 471 in d361155
Since we're not passing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I think mimicing Chrome is fine here. |
||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
WeakSetPrototypeAdd(detachedArrayBuffers, arrayBuffer); | ||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||
const serializedData = core.serialize(data, { | ||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be changed after is merged #16294 to: