Skip to content

lib: add brand checks to AbortController and AbortSignal#37720

Closed
MattiasBuelens wants to merge 6 commits intonodejs:masterfrom
MattiasBuelens:abort-controller-brand-checks
Closed

lib: add brand checks to AbortController and AbortSignal#37720
MattiasBuelens wants to merge 6 commits intonodejs:masterfrom
MattiasBuelens:abort-controller-brand-checks

Conversation

@MattiasBuelens
Copy link
Contributor

@MattiasBuelens MattiasBuelens 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-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 11, 2021
@MattiasBuelens MattiasBuelens 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-Arrowood Ethan-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
Contributor Author

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

@aduh95 aduh95 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.

@MattiasBuelens MattiasBuelens mentioned this pull request Mar 16, 2021
Closed
@MattiasBuelens
Copy link
Contributor Author

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/

@aduh95 aduh95 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 <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 19, 2021

Landed in feaeb76

@jasnell jasnell closed this Mar 19, 2021
@MattiasBuelens MattiasBuelens 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 <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ruyadorno ruyadorno 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 <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos 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 <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos 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
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@aduh95 aduh95 aduh95 approved these changes

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood approved these changes

Assignees

No one assigned

Labels

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

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