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

lib: fix TypeError when converting a detached buffer source #44020

Merged
merged 2 commits into from
Nov 1, 2022
Merged

lib: fix TypeError when converting a detached buffer source #44020

merged 2 commits into from
Nov 1, 2022

Conversation

cola119
Copy link
Member

@cola119 cola119 commented Jul 28, 2022

Currently TextDecoder.decode will throw TypeError when a detached buffer is given since it will try to convert a detached buffer into new buffer. This PR fixed TypeError by checking if a buffer is detached.

const buffer = new ArrayBuffer(1);
new MessageChannel().port1.postMessage(buffer, [buffer]); // buffer is detached
const decoder = new TextDecoder();
decoder.decode(buffer);
// node: TypeError: Cannot perform Construct on a detached ArrayBuffer
// chrome/safari/deno: ""

@nodejs-github-bot nodejs-github-bot added encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. needs-ci PRs that need a full CI run. labels Jul 28, 2022
@cola119 cola119 force-pushed the fix-type-error-of-detached-buffer branch from bbb15af to ad6a134 Compare July 28, 2022 09:08
BridgeAR
BridgeAR reviewed Jul 28, 2022
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

As a user, I would rather have the error than an empty buffer. The reason is that with the empty buffer, I just do not realize that something does not work as I expected.

@cola119
Copy link
Member Author

cola119 commented Jul 28, 2022

I personally would like Node.js to have the same behavior as Web browser, especially in encoding APIs because many times I want to share the encoding logic between Web and server. Throwing TypeError feels confusing for me.

@LiviaMedeiros
Copy link
Contributor

Doesn't it look more like issue with Buffer.from()? new Uint8Array(ab) on detached buffer should throw but Uint8Array.from(ab) should return exactly what's expected in TextDecoder.decode().

@cola119 cola119 force-pushed the fix-type-error-of-detached-buffer branch from ad6a134 to 6639bec Compare July 30, 2022 04:08
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 28, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47523/

@github-actions github-actions bot mentioned this pull request Oct 29, 2022
Open
15 tasks
@aduh95
Copy link
Contributor

aduh95 commented Oct 29, 2022

Error: Found 1 unexpected passes. Consider updating test/wpt/status/encoding.json for these files: streams/decode-utf8.any.js @cola119 could you take a look at this please?

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 29, 2022
@github-actions github-actions bot mentioned this pull request Oct 30, 2022
Open
15 tasks
@cola119 cola119 force-pushed the fix-type-error-of-detached-buffer branch from 6639bec to 89329e6 Compare October 30, 2022 04:43
@cola119
Copy link
Member Author

cola119 commented Oct 30, 2022 •
edited
Loading

@aduh95 I updated the status accordingly. 89329e6 PTAL.
(encoding.json was updated in #44101 and at that time streams/decode-utf8.any.js was expected to fail. This PR fixes that issue and I needed to update the status.)

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 30, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47580/

@cola119 cola119 force-pushed the fix-type-error-of-detached-buffer branch 2 times, most recently from 684f18a to a154022 Compare October 30, 2022 23:38
@cola119
Copy link
Member Author

cola119 commented Oct 30, 2022

Rebased and squashed

@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47581/

@github-actions github-actions bot mentioned this pull request Oct 31, 2022
Open
19 tasks
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47585/

@cola119 cola119 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 31, 2022
@cola119 cola119 force-pushed the fix-type-error-of-detached-buffer branch from a154022 to 2007eb5 Compare October 31, 2022 13:32
@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/47591/

@github-actions github-actions bot mentioned this pull request Nov 1, 2022
Open
17 tasks
@cola119
Copy link
Member Author

cola119 commented Nov 1, 2022

@aduh95 PTAL 2007eb5

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2022
@nodejs-github-bot nodejs-github-bot merged commit 86088ab into nodejs:main Nov 1, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 86088ab

lucshi pushed a commit to lucshi/node that referenced this pull request Nov 9, 2022
@cola119
lib: fix TypeError when converting a detached buffer source
a9c6442 
PR-URL: nodejs#44020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
@cola119 @RafaelGSS
lib: fix TypeError when converting a detached buffer source
d172669 
PR-URL: #44020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Nov 10, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@cola119 @danielleadams
lib: fix TypeError when converting a detached buffer source
031900d 
PR-URL: #44020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 30, 2022
Merged
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
@cola119 @danielleadams
lib: fix TypeError when converting a detached buffer source
68690ef 
PR-URL: #44020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
@cola119 @danielleadams
lib: fix TypeError when converting a detached buffer source
57897f8 
PR-URL: #44020
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@aduh95 aduh95 aduh95 approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
encoding Issues and PRs related to the TextEncoder and TextDecoder APIs. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cola119 @LiviaMedeiros @nodejs-github-bot @aduh95 @jasnell @BridgeAR