Skip to content

src: fix missing handlescope bug#17539

Closed
bnoordhuis wants to merge 1 commit intonodejs:masterfrom
bnoordhuis:fix17496
Closed

src: fix missing handlescope bug#17539
bnoordhuis wants to merge 1 commit intonodejs:masterfrom
bnoordhuis:fix17496

Conversation

@bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Dec 7, 2017 •
edited
Loading

Fix a regression that was introduced in commit 5886e20 ("inspector:
track async stacks when necessary") and that I overlooked during review:
the persistent handle with the callback must be rematerialized after
the v8::HandleScope is created, not before.

Apparently test/sequential/test-inspector-async-call-stack.js has
no test coverage for this scenario and I'm out of good ideas on how
to create a concise and reliable test case.

Fixes: #17496
CI: https://ci.nodejs.org/job/node-test-pull-request/11962/

@bnoordhuis
src: fix missing handlescope bug
b466779 
Fix a regression that was introduced in commit 5886e20 ("inspector:
track async stacks when necessary") and that I overlooked during review:
the persistent handle with the callback must be rematerialized *after*
the `v8::HandleScope` is created, not before.

Apparently `test/sequential/test-inspector-async-call-stack.js` has
no test coverage for this scenario and I'm out of good ideas on how
to create a concise and reliable test case.

Fixes: nodejs#17496
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 7, 2017
@bnoordhuis bnoordhuis mentioned this pull request Dec 7, 2017
Closed
addaleax
addaleax approved these changes Dec 7, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

re: testing … maybe just add a SealHandleScope to maxAsyncCallStackDepthChanged() (and other methods that should have one)?

Sorry, I see that’s kind of what already was triggering the assertion in the first place. Not sure how to test it either then.

@bnoordhuis bnoordhuis closed this Dec 9, 2017
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Dec 9, 2017
@bnoordhuis
src: fix missing handlescope bug in inspector
df79b7d 
Fix a regression that was introduced in commit 5886e20 ("inspector:
track async stacks when necessary") and that I overlooked during review:
the persistent handle with the callback must be rematerialized *after*
the `v8::HandleScope` is created, not before.

Apparently `test/sequential/test-inspector-async-call-stack.js` has
no test coverage for this scenario and I'm out of good ideas on how
to create a concise and reliable test case.

Fixes: nodejs#17496
PR-URL: nodejs#17539
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@bnoordhuis @MylesBorins
src: fix missing handlescope bug in inspector
b3cb627 
Fix a regression that was introduced in commit 5886e20 ("inspector:
track async stacks when necessary") and that I overlooked during review:
the persistent handle with the callback must be rematerialized *after*
the `v8::HandleScope` is created, not before.

Apparently `test/sequential/test-inspector-async-call-stack.js` has
no test coverage for this scenario and I'm out of good ideas on how
to create a concise and reliable test case.

Fixes: #17496
PR-URL: #17539
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@bnoordhuis @MylesBorins
src: fix missing handlescope bug in inspector
fb3ea4c 
Fix a regression that was introduced in commit 5886e20 ("inspector:
track async stacks when necessary") and that I overlooked during review:
the persistent handle with the callback must be rematerialized *after*
the `v8::HandleScope` is created, not before.

Apparently `test/sequential/test-inspector-async-call-stack.js` has
no test coverage for this scenario and I'm out of good ideas on how
to create a concise and reliable test case.

Fixes: #17496
PR-URL: #17539
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
Merged
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Missing landed in

Landed in df79b7d821

gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@bnoordhuis @gibfahn
src: fix missing handlescope bug in inspector
94d9f20 
Fix a regression that was introduced in commit 5886e20 ("inspector:
track async stacks when necessary") and that I overlooked during review:
the persistent handle with the callback must be rematerialized *after*
the `v8::HandleScope` is created, not before.

Apparently `test/sequential/test-inspector-async-call-stack.js` has
no test coverage for this scenario and I'm out of good ideas on how
to create a concise and reliable test case.

Fixes: #17496
PR-URL: #17539
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
@bnoordhuis @gibfahn
src: fix missing handlescope bug in inspector
28a81ce 
Fix a regression that was introduced in commit 5886e20 ("inspector:
track async stacks when necessary") and that I overlooked during review:
the persistent handle with the callback must be rematerialized *after*
the `v8::HandleScope` is created, not before.

Apparently `test/sequential/test-inspector-async-call-stack.js` has
no test coverage for this scenario and I'm out of good ideas on how
to create a concise and reliable test case.

Fixes: #17496
PR-URL: #17539
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@gibfahn gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

+2 more reviewers

@TimothyGu TimothyGu TimothyGu approved these changes

@XadillaX XadillaX XadillaX approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@bnoordhuis @gibfahn @addaleax @TimothyGu @cjihrig @XadillaX @nodejs-github-bot