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.

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

Add immutable array buffer awareness to structuredClone #11033

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gibson042
Copy link
Contributor

@gibson042 gibson042 commented Feb 15, 2025

Ref tc39/proposal-immutable-arraybuffer#30

Immutable ArrayBuffers is currently at Stage 2, but seeking advancement in next week's Ecma TC39 meeting. Because immutable ArrayBuffers cannot be detached, structuredClone should perform the relevant test.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chromium: …
    • Gecko: …
    • WebKit: …
    • Deno (only for timers, structured clone, base64 utils, channel messaging, module resolution, web workers, and web storage): …
    • Node.js (only for timers, structured clone, base64 utils, channel messaging, and module resolution): …
  • Corresponding HTML AAM & ARIA in HTML issues & PRs:
  • MDN issue is filed: …
  • The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)


/infrastructure.html ( diff )
/structured-data.html ( diff )

@domenic
Copy link
Member

domenic commented Feb 16, 2025

Can you work on getting the build passing?

Also, it would be good to update the PR title and commit message to specify that this is about preventing transferring of such immutable array buffers; structured cloning them is still allowed.

source Outdated
@@ -10463,7 +10464,8 @@ o.myself = o;</code></pre>
<span>"<code>DataCloneError</code>"</span> <code>DOMException</code>.</p></li>

<li><p>If <var>transferable</var> has an [[ArrayBufferData]] internal slot and
<span>IsSharedArrayBuffer</span>(<var>transferable</var>) is true, then throw a
<span>IsSharedArrayBuffer</span>(<var>transferable</var>) is true or
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit:

Suggested change
<span>IsSharedArrayBuffer</span>(<var>transferable</var>) is true or
either <span>IsSharedArrayBuffer</span>(<var>transferable</var>) is true or

since otherwise a valid reading at least at first glance is "(transferable has an [[ArrayBufferData]] internal slot and IsSharedArrayBuffer(transferable) is true) or IsImmutableBuffer(transferable) is true)".

@bakkot
Copy link
Contributor

bakkot commented Feb 16, 2025

Cloning should preserve immutability (tc39/proposal-immutable-arraybuffer#19 (comment)), the same way it currently preserves resizability.

@gibson042 gibson042 changed the title Prevent structuredClone of immutable array buffers (which are not detachable) Prevent structuredClone transfer of immutable array buffers (which are not detachable) Feb 17, 2025
@gibson042 gibson042 force-pushed the structured-clone-immutable-arraybuffer branch from ef5df7e to 2f4d3cb Compare February 17, 2025 20:17
@gibson042
Copy link
Contributor Author

Can you work on getting the build passing?

Done.

Also, it would be good to update the PR title and commit message to specify that this is about preventing transferring of such immutable array buffers; structured cloning them is still allowed.

Done.

Cloning should preserve immutability (tc39/proposal-immutable-arraybuffer#19 (comment)), the same way it currently preserves resizability.

Done: 2f4d3cb

@gibson042 gibson042 changed the title Prevent structuredClone transfer of immutable array buffers (which are not detachable) Add immutable array buffer awareness to structuredClone Feb 17, 2025
Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

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

I'm not an HTML reviewer, but LGTM

source Outdated
Comment on lines 9752 to 9754
<li><p>Set <var>serialized</var> to { [[Type]]: "ImmutableArrayBuffer", [[ArrayBufferData]]:
<var>value</var>.[[ArrayBufferData]], [[ArrayBufferByteLength]]:
<var>value</var>.[[ArrayBufferByteLength]] }.</p></li>
Copy link
Member

@andreubotella andreubotella Feb 17, 2025

Choose a reason for hiding this comment

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

Is value.[[ArrayBufferData]] supposed to be able to be kept alive in the serialization? Unlike for SABs, serializing immutable ArrayBuffers doesn't throw if forStorage is true, meaning that the serialization could be deserialized at some future time in a different process that doesn't share any memory with the current one.

At the very least we should have a note saying that implementations should make sure that a forStorage serialization actually serializes the buffer's contents, but a non-for-storage one might serialize a pointer to the buffer data instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is value.[[ArrayBufferData]] supposed to be able to be kept alive in the serialization? Unlike for SABs, serializing immutable ArrayBuffers doesn't throw if forStorage is true, meaning that the serialization could be deserialized at some future time in a different process that doesn't share any memory with the current one.

That's a good question. I'm not very well versed in the particulars here, but my instinct is that I've got it correct—an immutable array buffer should behave like any other non-shared array buffer when forStorage is true, such that it could be deserialized into a new immutable array buffer with identical contents by a completely independent process at some point in the future.

I like your idea of a note, and have attempted to update accordingly.

source Show resolved Hide resolved
[[ArrayBufferData]] internal slot value is <var>serialized</var>.[[ArrayBufferData]], whose
[[ArrayBufferByteLength]] internal slot value is
<var>serialized</var>.[[ArrayBufferByteLength]], and whose [[ArrayBufferIsImmutable]] internal
slot is present.</p></li>
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 possible for a slot to not have a value in this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically yes, but it's obviously not great and I see now that things are moving away from it. I've updated accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But for the record, the document.all HTMLAllCollection seems to be another example—it has an [[IsHTMLDDA]] internal slot for which no value is ever specified AFAICT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And in ECMAScript, Object Internal Methods and Internal Slots defines the initial value of an internal slot to be undefined.

Copy link
Contributor

Choose a reason for hiding this comment

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

True although see also tc39/ecma262#3399

source Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants