[v14.x backport] util: fix module inspection & instanceof check during inspect#37100

Pezmc · 2021-01-27T11:56:57Z

Backports #36178 from @BridgeAR, tagged as semver‑patch, to 14.x as per comment from @ExE-Boss

14.x is currently impacted by the bug fixed in #36178, which was originally released to 15.x in https://github.com/nodejs/node/releases/tag/v15.5.0

I followed the guide from https://github.com/nodejs/node/blob/master/doc/guides/backporting-to-release-lines.md

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <[email protected]> Fixes: nodejs#36151 PR-URL: nodejs#36178Fixes: nodejs#35730 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <[email protected]> Fixes: nodejs#35730 PR-URL: nodejs#36178Fixes: nodejs#36151 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

Pezmc · 2021-01-27T12:01:03Z

I don't have the Jekins permissions to schedule a node-test-pull-request CI job for this PR (37100)

Pezmc is missing the Job/Build permission

nodejs-github-bot · 2021-01-27T13:28:13Z

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

nodejs-github-bot · 2021-01-27T17:23:08Z

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

BethGriggs · 2021-01-27T19:57:04Z

Would appreciate a review from someone more familiar with this area (cc @nodejs/modules?)

ljharb

LGTM; i'll make similar changes in object-inspect.

ljharb · 2021-01-27T20:37:20Z

lib/internal/util/inspect.js

+functionisInstanceof(object,proto){
+try{
+returnobjectinstanceofproto;
+}catch{
+returnfalse;
+}
+}
+


why is the try/catch needed? are we worried about someone defining a Symbol.hasInstance method that can throw?

Based on cb9eec9#r527685177, instanceof can throw

ah, didn't realize this was a backport PR. turns out instanceof can indeed throw when the RHS is not a function, or when it's a non-constructible function (like an arrow function or a concise method)

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <[email protected]> Fixes: nodejs#36151 PR-URL: nodejs#36178Fixes: nodejs#35730 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <[email protected]> Fixes: nodejs#35730 PR-URL: nodejs#36178Fixes: nodejs#36151 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

Pezmc · 2021-01-29T18:20:52Z

@BethGriggs I tried rebasing onto the latest nodejs:v14.x-staging to clean this PR after your force push and ended up with 89402af in my diff. Do you know where that came from?

$ git fetch upstream v14.x-staging From https://github.com/nodejs/node * branch v14.x-staging -> FETCH_HEAD $ git checkout --track upstream/v14.x-staging Updating files: 100% (14760/14760), done. Branch 'v14.x-staging'set up to track remote branch 'v14.x-staging' from 'upstream'. Switched to a new branch 'v14.x-staging' $ git pull Already up to date. $ git checkout backport-36178-to-v14.x Switched to branch 'backport-36178-to-v14.x' $ git rebase v14.x-staging Successfully rebased and updated refs/heads/backport-36178-to-v14.x. $ git checkout -B backport-36178-to-v14.x-test Switched to a new branch 'backport-36178-to-v14.x-test' $ git push

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <[email protected]> Fixes: nodejs#36151 PR-URL: nodejs#36178Fixes: nodejs#35730 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <[email protected]> Fixes: nodejs#35730 PR-URL: nodejs#36178Fixes: nodejs#36151 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

BethGriggs · 2021-01-29T20:19:56Z

@BethGriggs I tried rebasing onto the latest nodejs:v14.x-staging to clean this PR after your force push and ended up with 89402af in my diff. Do you know where that came from?

Ah yes, that commit was backed out of staging as there was an npm version mismatch (ref: #37107 (comment)). We need to reland the npm update in a new PR.

For this PR, I think you could either rebase against upstream/v14.x-staging and then drop the npm commit. Or I can just cherry-pick your commits from this PR.

The good news is that the delay in fixing the npm update means we can probably squeeze this into v14.15.5 (#37074)

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <[email protected]> Fixes: nodejs#36151 PR-URL: nodejs#36178Fixes: nodejs#35730 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <[email protected]> Fixes: nodejs#35730 PR-URL: nodejs#36178Fixes: nodejs#36151 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

Pezmc · 2021-01-29T21:44:49Z

Re-rebased and manually dropped that commit, this PR should be good to go @BethGriggs!

Pezmc · 2021-01-29T21:55:52Z

Accidental close, please ignore!

nodejs-github-bot · 2021-01-29T23:16:05Z

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

Pezmc · 2021-02-01T20:09:06Z

@BethGriggs Anything I can do to help get this into v14.15.5?

BethGriggs · 2021-02-03T15:48:11Z

@Pezmc, I'll aim to integrate this into the v14.15.5 proposal - i'm still waiting on some other patches so haven't started updating the proposal yet. Nothing more to do on your side 🙂

BethGriggs · 2021-02-04T13:39:07Z

Would appreciate a review on this before landing (maybe @nodejs/modules)?

BethGriggs

Backport LGTM comparing with #36178

Signed-off-by: Ruben Bridgewater <[email protected]> Backport-PR-URL: #37100 PR-URL: #36178Fixes: #35730Fixes: #36151 Refs: #35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

BethGriggs · 2021-02-05T00:36:49Z

Landed in 408c7a65f3...f206505 (v14.x-staging). Thanks @Pezmc

Signed-off-by: Ruben Bridgewater <[email protected]> Backport-PR-URL: #37100 PR-URL: #36178Fixes: #35730Fixes: #36151 Refs: #35754 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: James M Snell <[email protected]>

Pezmc · 2021-02-15T11:27:55Z

Landed in https://github.com/nodejs/node/releases/tag/v14.15.5! 💃

nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Jan 27, 2021

Pezmc changed the base branch from master to v14.x-stagingJanuary 27, 2021 11:57

Pezmc force-pushed the backport-36178-to-v14.x branch from 54da1e1 to a38c661Compare January 27, 2021 11:58

ljharb reviewed Jan 27, 2021
View reviewed changes

BethGriggs force-pushed the v14.x-staging branch from 98f6155 to 9ff73fcCompare January 28, 2021 18:10

BridgeAR added 2 commits January 29, 2021 22:41

Pezmc force-pushed the backport-36178-to-v14.x branch from a38c661 to ce8caa4Compare January 29, 2021 21:43

Pezmc closed this Jan 29, 2021

Pezmc deleted the backport-36178-to-v14.x branch January 29, 2021 21:54

Pezmc restored the backport-36178-to-v14.x branch January 29, 2021 21:54

Pezmc reopened this Jan 29, 2021

BethGriggs added the review wanted PRs that need reviews. label Feb 3, 2021

BethGriggs approved these changes Feb 4, 2021
View reviewed changes

richardlau approved these changes Feb 4, 2021
View reviewed changes

BethGriggs closed this Feb 5, 2021

Pezmc deleted the backport-36178-to-v14.x branch February 15, 2021 11:27

Uh oh!

[v14.x backport] util: fix module inspection & instanceof check during inspect#37100

[v14.x backport] util: fix module inspection & instanceof check during inspect #37100

Uh oh!

Conversation

Pezmc commented Jan 27, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pezmc commented Jan 27, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 27, 2021

Uh oh!

nodejs-github-bot commented Jan 27, 2021• edited by BethGriggs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BethGriggs commented Jan 27, 2021

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

ljharbJan 27, 2021

Choose a reason for hiding this comment

Uh oh!

PezmcJan 28, 2021

Choose a reason for hiding this comment

Uh oh!

ljharbJan 28, 2021

Choose a reason for hiding this comment

Uh oh!

Pezmc commented Jan 29, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BethGriggs commented Jan 29, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pezmc commented Jan 29, 2021

Uh oh!

Pezmc commented Jan 29, 2021

Uh oh!

nodejs-github-bot commented Jan 29, 2021• edited by BethGriggs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pezmc commented Feb 1, 2021

Uh oh!

BethGriggs commented Feb 3, 2021

Uh oh!

BethGriggs commented Feb 4, 2021

Uh oh!

BethGriggs left a comment

Choose a reason for hiding this comment

Uh oh!

BethGriggs commented Feb 5, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pezmc commented Feb 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Pezmc commented Jan 27, 2021•
edited
Loading

Pezmc commented Jan 27, 2021•
edited
Loading

nodejs-github-bot commented Jan 27, 2021•
edited by BethGriggs
Loading

Pezmc commented Jan 29, 2021•
edited
Loading

BethGriggs commented Jan 29, 2021•
edited
Loading

nodejs-github-bot commented Jan 29, 2021•
edited by BethGriggs
Loading

BethGriggs commented Feb 5, 2021•
edited
Loading