FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

console: fix console.dir crash on a revoked proxy by daeyeon · Pull Request #43100 · nodejs/node · GitHub

/ node Public

console: fix console.dir crash on a revoked proxy - #43100

Merged
nodejs-github-bot merged 4 commits into
nodejs:masterfrom
daeyeon:master.console-220514.Sat.ba25
May 22, 2022
ConversationCommits (4)ChecksFiles changed
Merged

console: fix console.dir crash on a revoked proxy#43100
nodejs-github-bot merged 4 commits into
nodejs:masterfrom
daeyeon:master.console-220514.Sat.ba25

Conversation

daeyeon commented May 14, 2022

Copy link
Copy Markdown
Member

Fixes: #43095

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels May 14, 2022
daeyeon force-pushed the master.console-220514.Sat.ba25 branch 9 times, most recently from cdb0ea5 to bb9c1e7 Compare May 14, 2022 19:22
daeyeon added 2 commits May 15, 2022 04:24
console: fix console.dir crash on a revoked proxy
8253a30 
Fixes: nodejs#43095

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
console: remove spaces for the output is empty
5723aaf 
This commit removes extra spaces looking unnecessary if the
`joinedOutput` of type `Array` is empty on `reduceToSingleString`.

e.g) Proxy [  ] -> Proxy []

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
daeyeon force-pushed the master.console-220514.Sat.ba25 branch from bb9c1e7 to 5723aaf Compare May 14, 2022 19:26
BridgeAR reviewed May 14, 2022
View reviewed changes

BridgeAR left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks for the PR! I am going to ignore the implementation details for now to clarify the visualization first:

I am wondering how we want to visualize the revoked proxy with showProxy === false. It's not null but we do not know about the value either. We could just print Revoked Proxy [] or Proxy [ null, null ]. This would however expose the information that it's a proxy which is a hidden fact for not-revoked proxies.

The situation for showProxy === true is simpler: just either use Revoked Proxy [] or Proxy [ null, null ]. I personally like the Revoked Proxy [] a bit better. Any other opinions @nodejs/util?

daeyeon commented May 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Thanks for starting the clarification!

FAYI, this PR's current visualization is the following:

let r = Proxy.revocable({}, {});
console.log(1, inspect(r.proxy));
console.log(2, inspect(r.proxy, { showProxy: true }));

r.revoke();

console.log(3, inspect(r.proxy));
console.log(4, inspect(r.proxy, { showProxy: true }));
1 {}
2 Proxy [ {}, {} ]
3 Proxy []
4 Proxy [ null, null ]

(3) was the part I was most confused of.

fixup: fix util.format on a revoked proxy
161dbe5 
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
daeyeon force-pushed the master.console-220514.Sat.ba25 branch from ddf91e4 to 161dbe5 Compare May 16, 2022 17:26

BridgeAR commented May 18, 2022

Copy link
Copy Markdown
Member

@daeyeon what do you think about:

1 {}
2 Proxy [ {}, {} ]
3 <Revoked Proxy>
4 <Revoked Proxy>

3 + 4 using special for formatting (ctx.stylize('<Revoked Proxy>', 'special')).

BridgeAR reviewed May 18, 2022
View reviewed changes

BridgeAR left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It is possible to simplify the code by using this diff (line numbers must be ignored as they are not identical), if we do not care to distinguish visualization with the showProxy option:

@@ -759,6 +758,9 @@ function formatValue(ctx, value, recurseTimes, typedArray) {
   // any proxy handlers.
   const proxy = getProxyDetails(value, !!ctx.showProxy);
   if (proxy !== undefined) {
+    if (proxy === null) {
+      return ctx.stylize('<Revoked Proxy>', 'special');
+    }
     if (ctx.showProxy) {
       return formatProxy(ctx, proxy, recurseTimes);
     }
@@ -1967,6 +1960,9 @@ function hasBuiltInToString(value) {
   const getFullProxy = false;
   const proxyTarget = getProxyDetails(value, getFullProxy);
   if (proxyTarget !== undefined) {
+    if (proxyTarget === null) {
+      return true;
+    }
     value = proxyTarget;
   }

The last change moves the extra check to only be executed for proxies and no other entries (value?.toString has to run the check for all entries).

daeyeon force-pushed the master.console-220514.Sat.ba25 branch 2 times, most recently from 87a63f5 to a19e077 Compare May 19, 2022 08:59

daeyeon commented May 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

@BridgeAR Thanks for the review.

I also think that <Revoked Proxy> looks good and provides enough information for a revoked proxy. It can make the code simpler indeed. I applied your suggestion. PTAL.

The last change moves the extra check to only be executed for proxies and no other entries (value?.toString has to run the check for all entries).

Also fixed.

BridgeAR commented May 19, 2022

Copy link
Copy Markdown
Member

LGTM!

fixup: simplify the code and change visualization
13964d0 
Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com
daeyeon force-pushed the master.console-220514.Sat.ba25 branch from a19e077 to 13964d0 Compare May 19, 2022 13:08
aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 21, 2022
aduh95 added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels May 21, 2022
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 21, 2022

nodejs-github-bot commented May 21, 2022

Copy link
Copy Markdown
Collaborator

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

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 22, 2022
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 22, 2022
nodejs-github-bot merged commit f7cd3f6 into nodejs:master May 22, 2022

nodejs-github-bot commented May 22, 2022

Copy link
Copy Markdown
Collaborator

Landed in f7cd3f6

daeyeon deleted the master.console-220514.Sat.ba25 branch May 23, 2022 15:02
bengl pushed a commit that referenced this pull request May 30, 2022
console: fix console.dir crash on a revoked proxy
285ef30 
Fixes: #43095

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43100
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
bengl mentioned this pull request May 31, 2022
Merged
juanarbol pushed a commit that referenced this pull request May 31, 2022
console: fix console.dir crash on a revoked proxy
c4ff510 
Fixes: #43095

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43100
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
console: fix console.dir crash on a revoked proxy
0cd0f11 
Fixes: #43095

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43100
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 12, 2022
console: fix console.dir crash on a revoked proxy
e67c2dc 
Fixes: #43095

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43100
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
console: fix console.dir crash on a revoked proxy
35fc1c2 
Fixes: #43095

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: #43100
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
console: fix console.dir crash on a revoked proxy
236445b 
Fixes: nodejs/node#43095

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: nodejs/node#43100
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

BridgeAR BridgeAR approved these changes
aduh95 aduh95 approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Crash when calling console.dir on a revoked Proxy

5 participants

Back | FazBrowse Home | New Git URL