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

Fix assertion failure in RespondInternal after transferring array buffer in RespondWithNewView #1174

Merged
merged 10 commits into from
Oct 12, 2021

Conversation

nidhijaju
Copy link
Contributor

@nidhijaju nidhijaju commented Oct 12, 2021

While implementing #1123 in Chrome, I realised that calling RespondInternal(controller, view.[[viewByteLength]] after calling TransferArrayBuffer(view.[[ViewedArrayBuffer]]) makes the assertion in RespondInternal(), specifically Assert: bytesWritten > 0, fail. This is because view.[[viewByteLength]] becomes 0 after transferring the array buffer.

This can either be solved by not using TransferArrayBuffer() (which was the case before) or by storing the view.[[viewByteLength]] first before transferring the buffer. In this PR, I've taken the latter approach.

@MattiasBuelens I'm wondering if you have any context on why we changed the standard to use TransferArrayBuffer() in RespondWithNewView()?


Preview | Diff

@MattiasBuelens
Copy link
Collaborator

MattiasBuelens commented Oct 12, 2021

Good catch! 👍 The reference implementation doesn't correctly implement TransferArrayBuffer, so it's hard to catch these kinds of errors before browsers start implementing. (self.structuredClone() would help here, but #1233571 is still open. 😛)

Could you update the reference implementation to reflect the spec text changes?

@MattiasBuelens I'm wondering if you have any context on why we changed the standard to use TransferArrayBuffer() in RespondWithNewView()?

When you respond to a BYOB request, we must immediately take back control of the buffer so we can (later on) give it back to the initiating read-into request. If we didn't transfer the buffer immediately, then you could do something like:

const view = controller.byobRequest.view;
view[0] = 12;
controller.byobRequest.respondWithNewView(view.subarray(0, 1));
view[0] = 34; // shouldn't be possible!

@nidhijaju
Copy link
Contributor Author

Could you update the reference implementation to reflect the spec text changes?

Done :)

Thank you for the explanation!

Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm

@ricea ricea merged commit c079d53 into whatwg:main Oct 12, 2021
@nidhijaju nidhijaju deleted the respond-with-new-view-bug branch October 27, 2021 03:05
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.

3 participants