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

src: initialize inspector before RunBootstrapping() #32672

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Apr 5, 2020

This is necessary for --inspect-brk-node to work, and for the
inspector to be aware of scripts created before bootstrapping.

Fixes: #32648
Refs: #30467 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Apr 5, 2020
src/inspector_io.cc Outdated Show resolved Hide resolved
This is necessary for `--inspect-brk-node` to work, and for the
inspector to be aware of scripts created before bootstrapping.

Fixes: nodejs#32648
Refs: nodejs#30467 (comment)
@addaleax addaleax force-pushed the inspector-init-createenvironment branch from 0979bd5 to 4ce0133 Compare April 5, 2020 21:30
src/node.h Show resolved Hide resolved
@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label Apr 5, 2020
@addaleax addaleax removed the wip Issues and PRs that are still a work in progress. label Apr 5, 2020
@addaleax
Copy link
Member Author

addaleax commented Apr 5, 2020

@devsnek Sorry, had to make a few changes to get tests to pass (some of which were rather obvious in hindsight) … could you take another look?

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM. I am still skeptical about whether it's necessary to burden users with maintaining InspectorParentHandle across threads through the Environment API, but it is what it is

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 6, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 6, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30502/ (:heavy_check_mark:)

@addaleax
Copy link
Member Author

addaleax commented Apr 7, 2020

Landed in 8aa7ef7

addaleax added a commit that referenced this pull request Apr 7, 2020
This is necessary for `--inspect-brk-node` to work, and for the
inspector to be aware of scripts created before bootstrapping.

Fixes: #32648
Refs: #30467 (comment)

PR-URL: #32672
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax closed this Apr 7, 2020
@addaleax addaleax deleted the inspector-init-createenvironment branch April 7, 2020 22:05
BethGriggs pushed a commit that referenced this pull request Apr 14, 2020
This is necessary for `--inspect-brk-node` to work, and for the
inspector to be aware of scripts created before bootstrapping.

Fixes: #32648
Refs: #30467 (comment)

PR-URL: #32672
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau
Copy link
Member

It looks like this PR is making cctest crash when coverage is enabled (see #32912).

addaleax added a commit to addaleax/node that referenced this pull request Sep 23, 2020
This is necessary for `--inspect-brk-node` to work, and for the
inspector to be aware of scripts created before bootstrapping.

Fixes: nodejs#32648
Refs: nodejs#30467 (comment)

PR-URL: nodejs#32672
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 23, 2020
This is necessary for `--inspect-brk-node` to work, and for the
inspector to be aware of scripts created before bootstrapping.

Fixes: #32648
Refs: #30467 (comment)

Backport-PR-URL: #35241
PR-URL: #32672
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
codebytere added a commit to electron/electron that referenced this pull request May 20, 2021
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
codebytere added a commit to electron/electron that referenced this pull request May 31, 2021
codebytere added a commit to electron/electron that referenced this pull request Jun 8, 2021
codebytere added a commit to electron/electron that referenced this pull request Jun 9, 2021
codebytere added a commit to electron/electron that referenced this pull request Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in inspectors when entering certain frames
9 participants