Skip to content

lib: add brand checks to AbortController and AbortSignal#37720

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
Closed
MattiasBuelens wants to merge 6 commits into from
Closed

lib: add brand checks to AbortController and AbortSignal #37720

MattiasBuelens wants to merge 6 commits into from

Conversation

@MattiasBuelens
Copy link
Contributor

@MattiasBuelensMattiasBuelens commented Mar 11, 2021

Web IDL specifies that both attributes and operations of IDL interfaces should throw a TypeError if this does not implement the interface. These brand checks were missing for AbortController and AbortSignal (as noticed here), so I added them.

Unfortunately, this is not straightforward to test. WPT has an IDL test for this, but it's bunched up together with other IDL tests for EventTarget, Event and CustomEvent. Node supports the first two but not the last one, so the test would still fail. 😕 Suggestions are welcome!

@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 11, 2021
@MattiasBuelensMattiasBuelens mentioned this pull request Mar 11, 2021
Closed
4 tasks
aduh95
aduh95 reviewed Mar 11, 2021
View reviewed changes
jasnell
jasnell reviewed Mar 11, 2021
View reviewed changes
Ethan-Arrowood
Ethan-Arrowood approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@Ethan-ArrowoodEthan-Arrowood left a comment

Choose a reason for hiding this comment

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

This LGTM!

@aduh95
Copy link
Contributor

aduh95 commented Mar 13, 2021

Can you add tests?

@MattiasBuelens
Copy link
ContributorAuthor

MattiasBuelens commented Mar 14, 2021•
edited
Loading

@aduh95 Ah, I see, there's a test/parallel/test-abortcontroller.js file. Sure, I'll add some tests.

They'll kind of duplicate the WPT IDL tests, but as long as we can't run those against Node yet, I suppose duplicating them makes sense. Actually, my initial assumption was wrong: the IDL tests do not test against "bad" this contexts in method calls. So we definitely need our own tests. 🙂

MattiasBuelens
MattiasBuelens commented Mar 14, 2021
View reviewed changes
aduh95
aduh95 approved these changes Mar 15, 2021
View reviewed changes
Copy link
Contributor

@aduh95aduh95 left a comment

Choose a reason for hiding this comment

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

LGTM

IDL tests do not test against "bad" this contexts in method calls

It may be worth opening a PR to add those tests upstream to make sure the behaviour stays consistent.

@MattiasBuelensMattiasBuelens mentioned this pull request Mar 16, 2021
Closed
@MattiasBuelens
Copy link
ContributorAuthor

MattiasBuelens commented Mar 16, 2021

Okay, so my correction on my initial assumption was wrong. 😅 As Domenic pointed out, the IDL harness does try to call getters and methods with a "bad" receiver to check if they throw a TypeError. So we don't need to upstream any tests, this is already covered by the WPT IDL tests. My apologies for the confusion.

Once Node can run the IDL tests, we can remove these newly added tests. But for now, they are necessary, so I'll leave them in.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 18, 2021

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

@aduh95aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 19, 2021

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

jasnell pushed a commit that referenced this pull request Mar 19, 2021
@MattiasBuelens@jasnell
lib: add brand checks to AbortController and AbortSignal
feaeb76
PR-URL: #37720 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 19, 2021

Landed in feaeb76

@jasnelljasnell closed this Mar 19, 2021
@MattiasBuelensMattiasBuelens deleted the abort-controller-brand-checks branch March 20, 2021 23:13
ruyadorno pushed a commit that referenced this pull request Mar 24, 2021
@MattiasBuelens@ruyadorno
lib: add brand checks to AbortController and AbortSignal
af7489c
PR-URL: #37720 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@ruyadornoruyadorno mentioned this pull request Mar 30, 2021
Merged
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
@MattiasBuelens@targos
lib: add brand checks to AbortController and AbortSignal
4f1aa19
PR-URL: nodejs#37720 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@targostargos mentioned this pull request Apr 24, 2021
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
@MattiasBuelens@targos
lib: add brand checks to AbortController and AbortSignal
a4045c8
PR-URL: nodejs#37720 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@MattiasBuelens@targos
lib: add brand checks to AbortController and AbortSignal
10131cd
PR-URL: nodejs#37720 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Apr 30, 2021
@MattiasBuelens@targos
lib: add brand checks to AbortController and AbortSignal
b1507c4
PR-URL: #37720 Backport-PR-URL: #38386 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
@targostargos added backported-to-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v14.x labels Apr 30, 2021
@danielleadamsdanielleadams mentioned this pull request May 3, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@aduh95aduh95aduh95 approved these changes

@Ethan-ArrowoodEthan-ArrowoodEthan-Arrowood approved these changes

Assignees

No one assigned

Labels

needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@MattiasBuelens@aduh95@nodejs-github-bot@jasnell@Ethan-Arrowood@targos