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

inspector: fix session.disconnect crash #46942

Merged

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Mar 3, 2023

fix session.disconnect crash.

When worker calls session.disconnect(), JSBindingsSessionDelegate will be destroyed asynchronously, so we need to make sure that JSBindingsConnection is destroyed after JSBindingsSessionDelegate.

The test is as follows.

const { Session } = require('inspector');
const { Worker, isMainThread } = require('worker_threads'); 

if (isMainThread) {
  new Worker(__filename);
} else {
  const session = new Session();
  session.connectToMainThread();
  session.disconnect();
}

The error message is as follows.

../src/base_object-inl.h:52:virtual node::BaseObject::~BaseObject(): Assertion `(metadata->strong_ptr_count) == (0)' failed.
 1: 0x1029e5de8 node::Abort() [/usr/local/bin/node]
 2: 0x1029e5c36 node::AppendExceptionLine(node::Environment*, v8::Local<v8::Value>, v8::Local<v8::Message>, node::ErrorHandlingMode) [/usr/local/bin/node]
 3: 0x10294c6ec node::AsyncWrap::~AsyncWrap() [/usr/local/bin/node]
 4: 0x102aa77be node::inspector::(anonymous namespace)::JSBindingsConnection<node::inspector::(anonymous namespace)::MainThreadConnection>::~JSBindingsConnection() [/usr/local/bin/node]
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • 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 needs-ci PRs that need a full CI run. labels Mar 3, 2023
@theanarkh theanarkh force-pushed the fix_session_disconnect_crash branch from c38daf7 to 626e3ce Compare March 3, 2023 20:07
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

LGTM

@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@theanarkh theanarkh force-pushed the fix_session_disconnect_crash branch from 626e3ce to ee09149 Compare March 4, 2023 02:01
@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@theanarkh
Copy link
Contributor Author

@addaleax Hi, can you help review this PR ? thanks !

src/inspector_js_api.cc Outdated Show resolved Hide resolved
src/inspector_js_api.cc Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the fix_session_disconnect_crash branch from ee09149 to 20c3a37 Compare March 14, 2023 16:28
@theanarkh theanarkh force-pushed the fix_session_disconnect_crash branch from 20c3a37 to eefabde Compare March 14, 2023 16:32
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 14, 2023
@nodejs-github-bot
Copy link
Collaborator

@theanarkh theanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 14, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 14, 2023
@nodejs-github-bot nodejs-github-bot merged commit f543c05 into nodejs:main Mar 14, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in f543c05

targos pushed a commit that referenced this pull request Mar 18, 2023
PR-URL: #46942
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
PR-URL: #46942
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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 needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants