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, tracing: Make sure messages are sent on a main thread #24814

Merged
merged 0 commits into from Jan 30, 2019
Merged

inspector, tracing: Make sure messages are sent on a main thread #24814

merged 0 commits into from Jan 30, 2019

Conversation

eugeneo
Copy link
Contributor

@eugeneo eugeneo commented Dec 3, 2018

Fixes: #23185

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 3, 2018
@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 3, 2018

@ofrobots
Unfortunately, I do not have access to environment where I could run untrusted code at this moment so I was not able to reproduce the issue and confirm it was fixed.

@ofrobots
Copy link
Contributor

ofrobots commented Dec 3, 2018

Thanks for the fix @eugeneo. The original test case from #23185 no longer crashes for me with the fix.

@Trott
Copy link
Member

Trott commented Dec 4, 2018

@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 4, 2018

Looks like there's a race condition that surfaces on Windows.

@eugeneo eugeneo added inspector Issues and PRs related to the V8 inspector protocol worker Issues and PRs related to Worker support. and removed investigating labels Jan 6, 2019
@eugeneo
Copy link
Contributor Author

eugeneo commented Jan 6, 2019

CI is now passing: https://ci.nodejs.org/job/node-test-pull-request/19946/

I will update the commit title and will rebase the change. I also have a follow-up CL that fixes the problem that the bad test case identified.

@eugeneo eugeneo requested a review from ofrobots January 6, 2019 00:29
@Trott
Copy link
Member

Trott commented Jan 6, 2019

@nodejs/v8-inspector @nodejs/trace-events

@eugeneo eugeneo added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jan 14, 2019
@vmarchaud
Copy link
Contributor

vmarchaud commented Jan 26, 2019

Any news on when this PR could be merged (as well was #24945) ? Currently the inspector based trace_events reporter is broken :(

@eugeneo eugeneo added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 28, 2019
@eugeneo
Copy link
Contributor Author

eugeneo commented Jan 30, 2019

@eugeneo
Copy link
Contributor Author

eugeneo commented Jan 30, 2019

The only CI failure is coming from a flaky test that only fails on a single bot.

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 lib / src Issues and PRs related to general changes in the lib or src directory. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

trace_events: when enabling via inspector after startup, process abort
6 participants