Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Upgrade to Electron 3.1.10 #19419

Merged
merged 6 commits into from
May 30, 2019
Merged

Upgrade to Electron 3.1.10 #19419

merged 6 commits into from
May 30, 2019

Conversation

as-cii
Copy link
Contributor

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

Antonio Scandurra added 6 commits May 21, 2019 17:26
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
Copy link
Contributor Author

as-cii 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
@as-cii as-cii deleted the electron-3.1 branch May 30, 2019 07:35
@mfonville
Copy link
Contributor

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

@Arcanemagus Arcanemagus added the electron-3 Tasks related to upgrade to electron v3 label Jun 4, 2019
@mfonville
Copy link
Contributor

Just a heads up that Electron did release 3.1.11

@mfonville
Copy link
Contributor

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

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

rafeca 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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
electron-3 Tasks related to upgrade to electron v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in AllocateJSPromise after upgrading to Electron 3
4 participants