Skip to content

inspector: allow opening inspector when NODE_V8_COVERAGE is set - #46113

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MoLow:fix-inspector-with-c8-coverage
Jan 29, 2023
Merged

inspector: allow opening inspector when NODE_V8_COVERAGE is set#46113
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
MoLow:fix-inspector-with-c8-coverage

Conversation

@MoLow

@MoLowMoLow commented Jan 5, 2023

Copy link
Copy Markdown
Member

Fixes: #46110

@nodejs-github-botnodejs-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 needs-ci PRs that need a full CI run. labels Jan 5, 2023
@MoLowMoLow removed the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 5, 2023
@MoLow
MoLowforce-pushed the fix-inspector-with-c8-coverage branch from 868efc8 to 44f774cCompareJanuary 5, 2023 22:27
@MoLowMoLow changed the title src: allow opening inspector when NODE_V8_COVERAGE is setinspector: allow opening inspector when NODE_V8_COVERAGE is setJan 5, 2023
@MoLow

MoLow commented Jan 5, 2023

Copy link
Copy Markdown
MemberAuthor

CC @nodejs/inspector

@MoLowMoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 6, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@bnoordhuis

Copy link
Copy Markdown
Member

I kind of feel this is more a hack or workaround than a proper fix, possibly with undesirable side effects.

isEnabled() returns true because the inspector agent thread is running. Code coverage creates its own inspector session which in turn starts the agent thread.

Maybe the io_ != nullptr check here is wrong, that's the "is the agent thread already running?" check:

boolAgent::IsActive() {
if (client_ == nullptr)
returnfalse;
return io_ != nullptr || client_->IsActive();
}

Or maybe it's sometimes needed - but in this case it gives the wrong behavior.

@cola119

cola119 commented Jan 9, 2023

Copy link
Copy Markdown
Member

CMIIW, code coverage creates its own inspector session but doesn't start the agent thread while inspector.open() starts the agent thread. That means isEnabled() should return true when the agent thread is running, the condition when the inspector io (Agent::io_) exists. I've confirmed all tests are green.

diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc
index 3595536c78..1602faf1bd 100644
--- a/src/inspector_js_api.cc+++ b/src/inspector_js_api.cc@@ -273,7 +273,7 @@ static void RegisterAsyncHookWrapper(const FunctionCallbackInfo<Value>& args) {
void IsEnabled(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
- args.GetReturnValue().Set(InspectorEnabled(env));+ args.GetReturnValue().Set(env->inspector_agent()->IsListening());
}
void Open(const FunctionCallbackInfo<Value>& args) {

@MoLow
MoLowforce-pushed the fix-inspector-with-c8-coverage branch from 44f774c to 4d2eca7CompareJanuary 28, 2023 21:39
@MoLow

Copy link
Copy Markdown
MemberAuthor

thanks @cola119! I have pushed your suggestion

@MoLowMoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 29, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@MoLowMoLow added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 29, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 29, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 7b4cc58 into nodejs:mainJan 29, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 7b4cc58

@MoLow
MoLow deleted the fix-inspector-with-c8-coverage branch January 29, 2023 21:47
ruyadorno pushed a commit that referenced this pull request Feb 1, 2023
PR-URL: #46113Fixes: #46110
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadornoruyadorno mentioned this pull request Feb 1, 2023
@juanarboljuanarbol mentioned this pull request Mar 3, 2023
juanarbol pushed a commit that referenced this pull request Mar 3, 2023
PR-URL: #46113Fixes: #46110
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Mar 5, 2023
PR-URL: #46113Fixes: #46110
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inspectorIssues and PRs related to the V8 inspector protocolneeds-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

inspector.url()/open() does not work if NODE_V8_COVERAGE is set

6 participants

@MoLow@nodejs-github-bot@bnoordhuis@cola119@jasnell@benjamingr