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.

Sign up for GitHub

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

Jump to bottom

src: handle missing TracingController everywhere #33815

Closed
wants to merge 1 commit into from
Closed

src: handle missing TracingController everywhere #33815

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 9, 2020

Fixes: #33800

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 9, 2020
@addaleax addaleax mentioned this pull request Jun 9, 2020
Closed
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. embedding Issues and PRs related to embedding Node.js in another project. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Jun 10, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31811/

gireeshpunathil
gireeshpunathil reviewed Jun 10, 2020
View reviewed changes
src/tracing/trace_event.h
Comment on lines 475 to +477
v8::TracingController* controller =
node::tracing::TraceEventHelper::GetTracingController();
if (controller == nullptr) return 0;
Copy link
Member

Choose a reason for hiding this comment

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

may be move this to the beginning of the function? here and in AddMetadataEventImpl ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gireeshpunathil How would I do that without introducing a potential memory leak?

Copy link
Member

Choose a reason for hiding this comment

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

@addaleax - I am sorry, but not following: we have a nullcheck and return (with no trace action) in the middle of a function, it can be moved to the start of the function, to save some cpu cycles in cases where the tracing controller is NULL?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gireeshpunathil Yes, but the lines between the start of the function and here take ownership of memory, so moving this line would cause a memory leak?

Copy link
Member

Choose a reason for hiding this comment

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

do you mean the std::unique_ptr<v8::ConvertableToTraceFormat> objects? cant those be allocated only in the normal path? i.e., if TracingController object is not null?

Copy link
Member Author

Choose a reason for hiding this comment

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

@gireeshpunathil They are not being allocated here, though – this function only takes ownership of them.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 11, 2020 •
edited by addaleax
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/31833/ (:yellow_heart:)

addaleax added a commit that referenced this pull request Jun 11, 2020
@addaleax
src: handle missing TracingController everywhere
dfdbbd1 
Fixes: #33800

PR-URL: #33815
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member Author

Landed in dfdbbd1

@addaleax addaleax closed this Jun 11, 2020
@addaleax addaleax deleted the tracing-controller-nullptr branch June 11, 2020 19:53
@addaleax addaleax added lts-watch-v12.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 11, 2020
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@addaleax @codebytere
src: handle missing TracingController everywhere
f8dddd3 
Fixes: #33800

PR-URL: #33815
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@addaleax @codebytere
src: handle missing TracingController everywhere
92983f2 
Fixes: #33800

PR-URL: #33815
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
@addaleax @codebytere
src: handle missing TracingController everywhere
86283aa 
Fixes: #33800

PR-URL: #33815
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Merged
codebytere pushed a commit that referenced this pull request Jul 14, 2020
@addaleax @codebytere
src: handle missing TracingController everywhere
1255e37 
Fixes: #33800

PR-URL: #33815
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gireeshpunathil gireeshpunathil gireeshpunathil left review comments

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. embedding Issues and PRs related to embedding Node.js in another project. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segfault Creating Node Environments in v12.16.2
6 participants
@addaleax @nodejs-github-bot @jasnell @cjihrig @gireeshpunathil @MylesBorins