close
Skip to content

Fix --prof-startup never being able to profile renderer/extension host#307849

Merged
deepak1556 merged 2 commits into
microsoft:mainfrom
winstliu:fix-debug-ports
Apr 10, 2026
Merged

Fix --prof-startup never being able to profile renderer/extension host#307849
deepak1556 merged 2 commits into
microsoft:mainfrom
winstliu:fix-debug-ports

Conversation

@winstliu
Copy link
Copy Markdown
Member

@winstliu winstliu commented Apr 4, 2026

--remote-debugging-port is documented as only accepting a port.
Similarly, --inspect-brk-extensions runs through parseDebugParams, which directly tries to convert the argument into a number.

So, remove the host from both arguments.

Copilot AI review requested due to automatic review settings April 4, 2026 22:15
@vs-code-engineering
Copy link
Copy Markdown
Contributor

vs-code-engineering Bot commented Apr 4, 2026

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

@bpasero

Matched files:

  • src/vs/code/node/cli.ts

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes --prof-startup startup profiling by ensuring Electron/Node debug-related CLI switches are passed in the format they actually accept, so renderer and extension host profiling can attach successfully.

Changes:

  • Pass --remote-debugging-port as a port-only value (no host).
  • Pass --inspect-brk-extensions as a port-only value (no host), matching parseDebugParams behavior.

Copy link
Copy Markdown
Collaborator

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Thanks! can you remove profileHost on inspect-brk it is also redundant https://github.com/nodejs/node/blob/4f08c6478d6ecd073c03536b8a6c473232e16c37/src/node_options.h#L90

Also for --remote-debugging-port I believe we also need remote-allow-origins since a while back.

cc @connor4312 host parameters were added in #209142

@deepak1556 deepak1556 assigned connor4312 and unassigned alexr00 Apr 6, 2026
@deepak1556 deepak1556 added this to the 1.116.0 milestone Apr 6, 2026
@winstliu
Copy link
Copy Markdown
Member Author

winstliu commented Apr 7, 2026

Yep, let me get to that on Wednesday :)

@winstliu
Copy link
Copy Markdown
Member Author

winstliu commented Apr 9, 2026

Also for --remote-debugging-port I believe we also need remote-allow-origins since a while back.

I'm not sure this is needed. It's only checked if an Origin header is supplied, which wouldn't be the case for --prof-startup since I'd only expect browsers to send an Origin header when connecting, right? Versus what VS Code is doing with v8-inspect-profiler.

Copy link
Copy Markdown
Collaborator

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Thanks for checking, you are right it shouldn't affect non-browser clients.

@deepak1556 deepak1556 merged commit 72cfebb into microsoft:main Apr 10, 2026
22 of 23 checks passed
@deepak1556 deepak1556 self-assigned this Apr 10, 2026
@winstliu winstliu deleted the fix-debug-ports branch April 10, 2026 21:30
@vs-code-engineering vs-code-engineering Bot locked and limited conversation to collaborators May 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants