Skip to content

Conversation

@eugeneo
Copy link
Contributor

@eugeneoeugeneo commented Jun 10, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • the commit message follows commit guidelines
Affected core subsystem(s)

This change only touches inspector connection

Description of change

In some cases close callback was called twice while other codepaths ended up
not releasing the memory at all.

CC: @ofrobots

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 10, 2016
@mscdexmscdex added the inspector Issues and PRs related to the V8 inspector protocol label Jun 10, 2016
@ofrobots
Copy link
Contributor

@ofrobots
Copy link
Contributor

Relaunched CI: https://ci.nodejs.org/job/node-test-commit/3765/. Looks green.

PTAL @nodejs/diagnostics.

Copy link
Member

Choose a reason for hiding this comment

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

The ASSERT_NE is useless now (and should have been a CHECK_EQ in hindsight.)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Removed. Thanks!

In some cases close callback was called twice, while in some cases the memory was still not released at all.
@bnoordhuis
Copy link
Member

LGTM

@ofrobots
Copy link
Contributor

ofrobots pushed a commit that referenced this pull request Jun 17, 2016
In some cases close callback was called twice, while in some cases the memory was still not released at all. PR-URL: #7268 Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
@ofrobots
Copy link
Contributor

Landed as 9dfc8b9.

Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
In some cases close callback was called twice, while in some cases the memory was still not released at all. PR-URL: #7268 Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
@Fishrock123Fishrock123 mentioned this pull request Jul 5, 2016
@eugeneoeugeneo deleted the no_double_dispose branch August 2, 2016 23:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.inspectorIssues and PRs related to the V8 inspector protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@eugeneo@ofrobots@bnoordhuis@mscdex@MylesBorins@Fishrock123@nodejs-github-bot