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

5 participants
@alexeykuzmin
Contributor

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 a review from nornagon Sep 8, 2018

@alexeykuzmin alexeykuzmin requested a review from electron/reviewers as a code owner Sep 8, 2018

@alexeykuzmin alexeykuzmin changed the title from fix: initialize tracing controller before starting platform (#14499) to fix: initialize tracing controller before starting platform Sep 8, 2018

@alexeykuzmin

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Sep 9, 2018

Contributor

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

Contributor

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.

@nornagon

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

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Sep 10, 2018

Contributor

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?

Contributor

alexeykuzmin commented Sep 10, 2018

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 Sep 12, 2018

@codebytere

This comment has been minimized.

Show comment
Hide comment
@codebytere

codebytere Sep 19, 2018

Member

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

Member

codebytere commented Sep 19, 2018

@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 Sep 20, 2018

@alexeykuzmin alexeykuzmin changed the title from fix: initialize tracing controller before starting platform to fix: initialize tracing controller before starting platform (3-0-x) Sep 20, 2018

@alexeykuzmin

This comment has been minimized.

Show comment
Hide comment
@alexeykuzmin

alexeykuzmin Sep 20, 2018

Contributor

@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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@ckerr

ckerr Sep 21, 2018

Member

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.

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

17 checks passed

Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Show comment
Hide comment
@release-clerk

release-clerk bot Sep 21, 2018

Release Notes Persisted

no notes

release-clerk bot commented Sep 21, 2018

Release Notes Persisted

no notes

@ckerr ckerr deleted the initialize-tracing-controller branch Sep 21, 2018

@ckerr ckerr moved this from PR in Flight to Fixed for Next Release in 3.0.x Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment