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

lib: add brand checks to AbortController and AbortSignal #37720

Closed
wants to merge 6 commits into from
Closed

lib: add brand checks to AbortController and AbortSignal #37720

wants to merge 6 commits into from

Conversation

MattiasBuelens
Copy link
Contributor

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
lib/internal/abort_controller.js Outdated Show resolved Hide resolved
lib/internal/abort_controller.js Outdated Show resolved Hide resolved
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

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

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
Copy link
Contributor Author

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

@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

jasnell pushed a commit that referenced this pull request Mar 19, 2021
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

@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
PR-URL: #37720
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
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 26, 2021
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
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
PR-URL: #37720
Backport-PR-URL: #38386
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants