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

fix: initialize tracing controller before starting platform (3-0-x) #14503

Merged
merged 2 commits into from Sep 21, 2018

Conversation

alexeykuzmin
Copy link
Contributor

@alexeykuzmin alexeykuzmin commented Sep 8, 2018

Description of Change

Fixes a crash when electron/node@19d1193 is reverted.
A PR to revert it is already created: electron/node#60

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines
Release Notes

Notes: no notes

@alexeykuzmin alexeykuzmin requested review from nornagon and a team September 8, 2018 12:42
@alexeykuzmin alexeykuzmin changed the title fix: initialize tracing controller before starting platform (#14499) fix: initialize tracing controller before starting platform Sep 8, 2018
@alexeykuzmin
Copy link
Contributor Author

alexeykuzmin commented Sep 9, 2018

Please ignore appveyor: electron-ia32-branch and appveyor: electron-x64-branch "failures", they shouldn't have been started at all.

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

LGTM, but before merge we should probably understand what the other things are that node does when initializing the tracing system: https://github.com/electron/node/blob/electron-node-v10.2.0-3.0.x/src/node.cc#L294-L297

@alexeykuzmin
Copy link
Contributor Author

before merge we should probably understand what the other things are that node does when initializing the tracing system: https://github.com/electron/node/blob/electron-node-v10.2.0-3.0.x/src/node.cc#L294-L297

@nornagon any idea who could do that?

@codebytere codebytere added this to PR in Flight in 3.0.x / 3.1.x Sep 12, 2018
@codebytere
Copy link
Member

@alexeykuzmin i merged the Node PR, once you roll deps this can be merged!

@alexeykuzmin alexeykuzmin changed the base branch from master to 3-0-x September 20, 2018 15:20
@alexeykuzmin alexeykuzmin changed the title fix: initialize tracing controller before starting platform fix: initialize tracing controller before starting platform (3-0-x) Sep 20, 2018
@alexeykuzmin
Copy link
Contributor Author

alexeykuzmin commented Sep 20, 2018

@codebytere
That change has already gotten to the master as a part of the Chromium 67 merge.
So I changed the target branch to 3-0-x and updated the vendor/node ref.

@ckerr
Copy link
Member

ckerr commented Sep 21, 2018

Merging after discussion with @alexeykuzmin. If there are other node issues as mentioned above by @nornagon, they don't block this PR and can go in a followup.

@ckerr ckerr merged commit 7eb1c3f into 3-0-x Sep 21, 2018
@release-clerk
Copy link

release-clerk bot commented Sep 21, 2018

Release Notes Persisted

no notes

@ckerr ckerr deleted the initialize-tracing-controller branch September 21, 2018 15:24
@ckerr ckerr moved this from PR in Flight to Fixed for Next Release in 3.0.x / 3.1.x Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
3.0.x / 3.1.x
Fixed (3.0.1)
Development

Successfully merging this pull request may close these issues.

None yet

5 participants