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

Fixed crash on process exit on Windows #10365

Merged
merged 2 commits into from Aug 29, 2017
Merged

Fixed crash on process exit on Windows #10365

merged 2 commits into from Aug 29, 2017

Conversation

alespergl
Copy link
Contributor

This is a fix for #10188 which was introduced by switching to dynamically linked CRT on Windows. The problem went unnoticed because it is mostly harmless other than recording the app crash in the Windows Event Log. It is desirable to merge this to master and to the 1.7 stream as well.

Description:

Chromium has its own TLS subsystem which supports automatic destruction of thread-local data, and also depends on memory allocation routines provided by the CRT. The problem is that the auto-destruction mechanism uses a hidden feature of the OS loader which calls a callback on thread exit, but only after all loaded DLLs have been detached. Since the CRT is also a DLL, it happens that by the time Chromium's OnThreadExit function is called, the heap functions, though still in memory, no longer perform their duties, and when Chromium calls free on its buffer, it triggers an access violation error.

The workaround is to invoke Chromium's OnThreadExit in time from within the CRT's atexit facility, ensuring the heap functions are still active. The second invocation from the OS loader code will be a no-op.

@MarshallOfSound
Copy link
Member

Awesome, let's get this in asap ⭐

// their duties, and when Chromium calls `free` on its buffer, it triggers
// an access violation error.
// We work around this problem by invoking Chromium's `OnThreadExit` in time
// from within the CRT's atexit facility, ensuring the heap functions are still
Copy link
Member

Choose a reason for hiding this comment

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

It is triggering linter error:

atom/app/atom_main.cc:107:  Lines should be <= 80 characters long  [whitespace/line_length] [2]

@alespergl
Copy link
Contributor Author

alespergl commented Aug 28, 2017

Hm, seems like something changed in Chromium since 58 and the OnThreadExit function is not there anymore.

Can we get this into 1.7 as is (there's no branch for that as far as I can tell) while I figure out how to port it to 1.8?

Update: Actually I needed to apply the fix only in the release build where Chromium is statically linked. The problem doesn't exist in debug/shared build and the fix doesn't compile in that mode either.

@jkleinsc
Copy link
Contributor

@alespergl There is a 1.7.x branch here: https://github.com/electron/electron/tree/1-7-x

@alespergl alespergl changed the base branch from master to 1-7-x August 28, 2017 14:18
@jkleinsc
Copy link
Contributor

@alespergl thanks for updating the PR. It looks like the Windows 32 bit build is failing:

App threw an error during load
Error: Cannot find module 'electabul'
    at Module._resolveFilename (module.js:470:15)
    at Function.Module._resolveFilename (C:\e\workspace\electron-win-ia32\out\D\resources\electron.asar\common\reset-search-paths.js:35:12)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (C:\e\workspace\electron-win-ia32\spec\static\main.js:7:20)
    at Object.<anonymous> (C:\e\workspace\electron-win-ia32\spec\static\main.js:377:3)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)

Linux x64 build fails with the following:

not ok 111 BrowserWindow module "webPreferences" option nativeWindowOpen option loads native addons correctly after reload
  AssertionError: 'Require runas failed: /usr/lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.21\' not found (required by /var/lib/jenki == 'function'
      at ipcMain.once (/var/lib/jenkins/workspace/electron-linux-x64/spec/api-browser-window-spec.js:1359:18)
      at CallbacksRegistry.apply (/var/lib/jenkins/workspace/electron-linux-x64/out/D/resources/electron.asar/common/api/callbacks-registry.js:48:25)
      at EventEmitter.<anonymous> (/var/lib/jenkins/workspace/electron-linux-x64/out/D/resources/electron.asar/renderer/api/remote.js:299:21)
      at emitThree (events.js:116:13)
      at EventEmitter.emit (events.js:197:7)

I am going to enable CircleCI on this branch to see if running the Linux x64 build will work there.

@jkleinsc
Copy link
Contributor

@alespergl All builds are now passing, so I think this is good to go.

@jkleinsc jkleinsc merged commit 96bc46c into 1-7-x Aug 29, 2017
@jkleinsc jkleinsc deleted the fix_exit_crash branch August 29, 2017 15:55
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.

None yet

4 participants