Skip to content

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

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
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
Copy link
Collaborator

nodejs-github-bot commented Dec 3, 2018

@eugeneo build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1812/pipeline
@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

CI: https://ci.nodejs.org/job/node-test-pull-request/19170/
@eugeneo
Copy link
Contributor Author

eugeneo commented Dec 4, 2018

Looks like there's a race condition that surfaces on Windows.
This was referenced Dec 5, 2018
Closed
Closed
@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 •
edited
Loading

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

CI: https://ci.nodejs.org/job/node-test-pull-request/20449/
@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.
@eugeneo eugeneo closed this Jan 30, 2019
@eugeneo eugeneo merged commit 80a18ca into nodejs:master Jan 30, 2019
@eugeneo eugeneo deleted the 23185-tracing-crash branch January 30, 2019 19:49
addaleax pushed a commit that referenced this pull request Jan 30, 2019
@addaleax
inspector, trace_events: make sure messages are sent on a main thread
6967407 
Fixes: #23185 PR-URL: #24814 Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Feb 14, 2019
Merged
This was referenced Feb 15, 2019
Closed
Closed
Closed
This was referenced Feb 15, 2019
Closed
Closed
Closed
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.

6 participants

@eugeneo @nodejs-github-bot @ofrobots @Trott @vmarchaud @jasnell