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

Upgrade to Electron 3.1.10 #19419

Merged
merged 6 commits into from May 30, 2019

Conversation

Projects
None yet
4 participants
@as-cii
Copy link
Member

commented May 30, 2019

Fixes #19372

Previously, we had merged the Electron 3 upgrade branch into master, but quickly discovered several ways in which Atom could crash. That caused us to revert the upgrade so that we could investigate where the problem was occurring.

All of the crashes could ultimately be traced back to mksnapshot, which was compiled with a set of flags that would confuse Node once the snapshot was loaded into V8 (see electron/electron#18420). This pull request builds on top of #18916 but uses Electron 3.1.10 instead, which fixes the snapshot issue.

Note that, with this pull request, we are also bumping @atom/nsfw to v1.0.23. While not technically needed for the Electron upgrade, this new version contains several fixes that had been introduced in the upstream repository but that our fork wasn't yet using. For more information, see atom/nsfw#6.

I have been using this branch for 10 days now and, after several stress tests, I couldn't observe the hard crashes anymore. It was relatively easy to reproduce them with the buggy mksnapshot, so I am reasonably confident those issues have finally been resolved.

This pull request only contains a few minor changes in addition to #18916, so I am planning to merge it to master as soon as we get a green build.

as-cii added some commits May 21, 2019

Don't create a promise when preloading package settings in snapshot
Promise creation is forbidden within `mksnapshot` (see 
electron/libchromiumcontent#363, 
nodejs/node#13242 and 
electron/electron#18420). Since preloading a 
package's settings is a synchronous action anyway, we just avoid 
instantiating a new Promise when calling `loadSettings`.

@as-cii as-cii self-assigned this May 30, 2019

@as-cii

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

Builds are green, I ended up canceling https://github.visualstudio.com/Atom/_build/results?buildId=41619 because I think AppVeyor mistakenly thought this was a production branch. Going ahead and merging this.

@as-cii as-cii merged commit 187f07d into master May 30, 2019

2 of 4 checks passed

Atom Production Branches #1.39.0-dev+41619 failed
Details
Atom Pull Requests in progress
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@as-cii as-cii deleted the electron-3.1 branch May 30, 2019

@mfonville

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2019

I can confirm that the nightly based on Electron 3 works great on Ubuntu :-)

@mfonville

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Just a heads up that Electron did release 3.1.11

@mfonville

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

With Electron v3, shouldn't this issue be fixed: electron/electron#2073 (

// (TodoElectronIssue: This got fixed in electron v3: https://github.com/electron/electron/issues/2073).
) and this TODO be resolved?
// TodoElectronIssue: Once we upgrade to electron v3 we can use `crypto.randomBytes()`

@Arcanemagus

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Reviewing all TodoElectronIssue comments is now part of our review process for new Electron versions, it simply hasn't happened yet for the Electron 4 upgrade.

Thanks for bringing it up though!

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Thanks for bringing that up @mfonville ! we need to do some verification in order to remove these two TODOs, but it's under our radar 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.