Skip to content

src: use v8::Isolate::TryGetCurrent() in DumpJavaScriptBacktrace() - #50518

Closed
joyeecheung wants to merge 3 commits into
nodejs:mainfrom
joyeecheung:try-get-current
Closed

src: use v8::Isolate::TryGetCurrent() in DumpJavaScriptBacktrace()#50518
joyeecheung wants to merge 3 commits into
nodejs:mainfrom
joyeecheung:try-get-current

Conversation

@joyeecheung

Copy link
Copy Markdown
Member

It was using Isolate::GetCurrent() which DCHECK on nullptr, even though what we wanted was to return early if it is nullptr.

Refs: #50242

It was using Isolate::GetCurrent() which DCHECK on nullptr, even
though what we wanted was to return early if it is nullptr.
@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. debugger Issues and PRs related to the debugger subsystem. needs-ci PRs that need a full CI run. labels Nov 2, 2023
H4ad
H4ad approved these changes Nov 2, 2023
@H4adH4ad added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 2, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 2, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@legendecaslegendecas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be worth adding a cctest to verify DumpJavaScriptBacktrace won't trigger DCHECK when no isolate is entered?

@juanarboljuanarbol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@joyeecheung

joyeecheung commented Nov 3, 2023

Copy link
Copy Markdown
MemberAuthor

Added a cctest

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

Copy link
Copy Markdown
Collaborator

H4ad
H4ad approved these changes Nov 3, 2023
@H4adH4ad added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@joyeecheung

Copy link
Copy Markdown
MemberAuthor

Landed in 036c89f with the linter issue fixed.

joyeecheung added a commit that referenced this pull request Nov 6, 2023
It was using Isolate::GetCurrent() which DCHECK on nullptr, even
though what we wanted was to return early if it is nullptr.
PR-URL: #50518
Refs: #50242
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
It was using Isolate::GetCurrent() which DCHECK on nullptr, even
though what we wanted was to return early if it is nullptr.
PR-URL: #50518
Refs: #50242
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@targostargos mentioned this pull request Nov 12, 2023
targos pushed a commit that referenced this pull request Nov 14, 2023
It was using Isolate::GetCurrent() which DCHECK on nullptr, even
though what we wanted was to return early if it is nullptr.
PR-URL: #50518
Refs: #50242
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
It was using Isolate::GetCurrent() which DCHECK on nullptr, even
though what we wanted was to return early if it is nullptr.
PR-URL: #50518
Refs: #50242
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@UlisesGasconUlisesGascon mentioned this pull request Dec 12, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs 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++.debuggerIssues and PRs related to the debugger subsystem.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@joyeecheung@nodejs-github-bot@legendecas@H4ad@juanarbol