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

Update to Electron 11 to pick up Node fixes #436

Merged
merged 19 commits into from
Jan 13, 2021
Merged

Conversation

calebmshafer
Copy link
Member

@calebmshafer calebmshafer commented Dec 15, 2020

The update to Electron 11 picks up a fix in Node (included in Node 12.18.3) for a crash during the destructor of ObjectWrap from napi.

The Electron update looks to have fixed the issue @ramanujam-raman ran into during the Electron 10 update. However, we need to do a bit more testing to make sure. That's still a work in progress.

Update: This did not fix the issue with exiting the app on Windows in Certa. Reverting those changes. However, it still fixes the other issue so is worth updating.

tl;dr

While @wgoehrig and I were debugging into a MacOS hang in the CI builds, we discovered it is due to a crash in the core-full-stack-tests when running in Electron. After the initial crash, and the crash report pops up, a subsequent run of Electron opens a dialog that asks if you want to restart from the previous state. This dialog is silly for a CI build since you don't want to usually use the previous state, we're working on fixing that in the CI infrastructure to avoid it hanging for that reason ever again.

After we figured that out, we started to dig into the crash itself. From the callstack (let me know if you want the full callstack), we found it was due to the destructor of the ObjectWrap causing the issue,

1   com.github.Electron.framework 	0x000000010d763fde v8::internal::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*) + 862
2   com.github.Electron.framework 	0x000000010d8d2f5f v8::internal::HandleScope::Extend(v8::internal::Isolate*) + 399
3   com.github.Electron.framework 	0x000000010d7663b8 v8::HandleScope::CreateHandle(v8::internal::Isolate*, unsigned long) + 56
4   com.github.Electron.framework 	0x0000000112d09e51 napi_get_reference_value + 65
5   imodeljs.node                 	0x0000000121fb5c68 Napi::ObjectWrap<IModelJsNative::NativeDgnDb>::~ObjectWrap() + 48
...

The following trace of GitHub issues led us to find that this was an issue directly in Node and not napi. The fix has been back ported to both >=12.18.3 and 14.x. In order to pick this fix up in Electron, we have to update to Electron 11 which includes 12.18.3 as its Node version.

All of this said, we do have another crash in the native code from these tests which doesn't appear to be related. It happens less often than the current one. I'll follow up on that separately.

@calebmshafer calebmshafer marked this pull request as ready for review January 5, 2021 23:18
@calebmshafer
Copy link
Member Author

@pmconne @swwilson-bsi mind reviewing?

@pmconne
Copy link
Member

pmconne commented Jan 11, 2021

@calebmshafer Before upgrading Node:

--[ FAILURE: @bentley/build-tools ]--------------------------[ 2.16 seconds ]--

../../common/temp/node_modules/.pnpm/registry.npmjs.org/tsutils/3.19.1_typescript@3.7.5/node_modules/tsutils/util/util.d.ts(256,65): error TS1110: Type expected.

After upgrading to Node 14.15.4:

[j2 10:47:43.88] d:\j2\s\imodeljs>rush build --to display-test-app
Your version of Node.js (14.15.4) has not been tested with this release of Rush. Please consider installing a newer version of the "@microsoft/rush" package, or downgrading Node.js.

Rush version 5.34.2 is not currently installed. Installing...
Trying to acquire lock for rush-5.34.2
Deleting old files from C:\Users\paul.connelly\.rush\node-v14.15.4\rush-5.34.2
Copying d:\j2\s\imodeljs\common\config\rush\.npmrc --> C:\Users\paul.connelly\.rush\node-v14.15.4\rush-5.34.2\.npmrc

Running "npm install" in C:\Users\paul.connelly\.rush\node-v14.15.4\rush-5.34.2
Successfully installed Rush version 5.34.2 in C:\Users\paul.connelly\.rush\node-v14.15.4\rush-5.34.2.

Rush Multi-Project Build Tool 5.34.2 - https://rushjs.io
Node.js version is 14.15.4 (LTS)



ERROR: Your dev environment is running Node.js version v14.15.4 which does not meet the requirements for building this repository.  (The rush.json configuration requires
nodeSupportedVersionRange=">=10.17.0 <13.0.0")

[j2 10:52:11.84] d:\j2\s\imodeljs>rush clean
Your version of Node.js (14.15.4) has not been tested with this release of Rush. Please consider installing a newer version of the "@microsoft/rush" package, or downgrading Node.js.


Rush Multi-Project Build Tool 5.34.2 - https://rushjs.io
Node.js version is 14.15.4 (LTS)



ERROR: Your dev environment is running Node.js version v14.15.4 which does not meet the requirements for building this repository.  (The rush.json configuration requires
nodeSupportedVersionRange=">=10.17.0 <13.0.0")

[j2 10:52:17.50] d:\j2\s\imodeljs>rush build --to display-test-app
[j2 10:52:20.05] d:\j2\s\imodeljs>
[j2 10:53:44.87] d:\j2\s\imodeljs>rush install
Your version of Node.js (14.15.4) has not been tested with this release of Rush. Please consider installing a newer version of the "@microsoft/rush" package, or downgrading Node.js.


Rush Multi-Project Build Tool 5.34.2 - https://rushjs.io
Node.js version is 14.15.4 (LTS)



ERROR: Your dev environment is running Node.js version v14.15.4 which does not meet the requirements for building this repository.  (The rush.json configuration requires
nodeSupportedVersionRange=">=10.17.0 <13.0.0")

@calebmshafer
Copy link
Member Author

@pmconne yes, this doesn't yet support Node 14.x. That will be a separate PR.

Fixing the tsutils issue now. I'll push an update in a minute.

Copy link
Member

@pmconne pmconne left a comment

Choose a reason for hiding this comment

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

I couldn't find any regressions in display-test-app. OIDC sign-in still works.

@calebmshafer calebmshafer enabled auto-merge (squash) January 11, 2021 17:20
auto-merge was automatically disabled January 13, 2021 03:27

Pull request was closed

@calebmshafer calebmshafer reopened this Jan 13, 2021
@calebmshafer calebmshafer enabled auto-merge (squash) January 13, 2021 03:31
@calebmshafer calebmshafer merged commit 6028a0f into master Jan 13, 2021
@calebmshafer calebmshafer deleted the update-electron-11 branch January 13, 2021 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants