[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 <ruben@bridgewater.de> Fixes: nodejs#36151 PR-URL: nodejs#36178 Fixes: nodejs#35730 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> Fixes: nodejs#35730 PR-URL: nodejs#36178 Fixes: nodejs#36151 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

+function isInstanceof(object, proto) {
+  try {
+    return object instanceof proto;
+  } catch {
+    return false;
+  }
+}
+


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 <ruben@bridgewater.de> Fixes: nodejs#36151 PR-URL: nodejs#36178 Fixes: nodejs#35730 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> Fixes: nodejs#35730 PR-URL: nodejs#36178 Fixes: nodejs#36151 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <ruben@bridgewater.de> Fixes: nodejs#36151 PR-URL: nodejs#36178 Fixes: nodejs#35730 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> Fixes: nodejs#35730 PR-URL: nodejs#36178 Fixes: nodejs#36151 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <ruben@bridgewater.de> Fixes: nodejs#36151 PR-URL: nodejs#36178 Fixes: nodejs#35730 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: nodejs#37100 Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> Fixes: nodejs#35730 PR-URL: nodejs#36178 Fixes: nodejs#36151 Refs: nodejs#35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <ruben@bridgewater.de> Backport-PR-URL: #37100 PR-URL: #36178 Fixes: #35730 Fixes: #36151 Refs: #35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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

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

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de> Backport-PR-URL: #37100 PR-URL: #36178 Fixes: #35730 Fixes: #36151 Refs: #35754 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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-staging January 27, 2021 11:57

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

ljharb reviewed Jan 27, 2021

View reviewed changes

BethGriggs force-pushed the v14.x-staging branch from 98f6155 to 9ff73fc Compare 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 ce8caa4 Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Pezmc commented Jan 27, 2021 •

edited

Pezmc commented Jan 27, 2021 •

edited

nodejs-github-bot commented Jan 27, 2021

nodejs-github-bot commented Jan 27, 2021 •

edited by BethGriggs

BethGriggs commented Jan 27, 2021

ljharb left a comment

ljharb Jan 27, 2021

Pezmc Jan 28, 2021

ljharb Jan 28, 2021

Pezmc commented Jan 29, 2021 •

edited

BethGriggs commented Jan 29, 2021 •

edited

Pezmc commented Jan 29, 2021

Pezmc commented Jan 29, 2021

nodejs-github-bot commented Jan 29, 2021 •

edited by BethGriggs

Pezmc commented Feb 1, 2021

BethGriggs commented Feb 3, 2021

BethGriggs commented Feb 4, 2021

BethGriggs left a comment

BethGriggs commented Feb 5, 2021 •

edited

Pezmc commented Feb 15, 2021

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

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

Conversation

Pezmc commented Jan 27, 2021 • edited

Pezmc commented Jan 27, 2021 • edited

nodejs-github-bot commented Jan 27, 2021

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

BethGriggs commented Jan 27, 2021

ljharb left a comment

Choose a reason for hiding this comment

ljharb Jan 27, 2021

Choose a reason for hiding this comment

Pezmc Jan 28, 2021

Choose a reason for hiding this comment

ljharb Jan 28, 2021

Choose a reason for hiding this comment

Pezmc commented Jan 29, 2021 • edited

BethGriggs commented Jan 29, 2021 • edited

Pezmc commented Jan 29, 2021

Pezmc commented Jan 29, 2021

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

Pezmc commented Feb 1, 2021

BethGriggs commented Feb 3, 2021

BethGriggs commented Feb 4, 2021

BethGriggs left a comment

Choose a reason for hiding this comment

BethGriggs commented Feb 5, 2021 • edited

Pezmc commented Feb 15, 2021

Pezmc commented Jan 27, 2021 •

edited

Pezmc commented Jan 27, 2021 •

edited

nodejs-github-bot commented Jan 27, 2021 •

edited by BethGriggs

Pezmc commented Jan 29, 2021 •

edited

BethGriggs commented Jan 29, 2021 •

edited

nodejs-github-bot commented Jan 29, 2021 •

edited by BethGriggs

BethGriggs commented Feb 5, 2021 •

edited