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

importing powerMonitor from electron before 'ready' event causes crash #21716

Closed
3 tasks done
mtgto opened this issue Jan 9, 2020 · 6 comments · Fixed by #21927
Closed
3 tasks done

importing powerMonitor from electron before 'ready' event causes crash #21716

mtgto opened this issue Jan 9, 2020 · 6 comments · Fixed by #21927

Comments

@mtgto
Copy link
Contributor

mtgto commented Jan 9, 2020

Preflight Checklist

  • I have read the Contributing Guidelines for this project.
  • I agree to follow the Code of Conduct that this project adheres to.
  • I have searched the issue tracker for an issue that matches the one I want to file, without success.

Issue Details

Same as #18966, app crashed when launched.

import electron, { app } from "electron";

let mainWindow: electron.BrowserWindow;

app.on("ready", () => {
  mainWindow = new electron.BrowserWindow({
    width: 800,
    height: 600,
  });
});

Here is my repro sample.
https://github.com/mtgto/sample-electron-crash

Here is compiled index.ts
https://gist.github.com/mtgto/2a60ed4ce2e280c7577412884cde166e

  • Electron Version:
    • v7.1.8
  • Operating System:
    • Ubuntu 18.04.3
  • Last Known Working Electron version:
    • Don't know

Expected Behavior

Open an empty window.

Actual Behavior

$ yarn start
yarn run v1.21.1
$ electron index.js
App threw an error during load
Error: The 'powerMonitor' module can't be used before the app 'ready' event
    at Object.module.<computed> [as on] (electron/js2c/browser_init.js:6800:30)
    at Object../lib/browser/api/power-monitor.ts (electron/js2c/browser_init.js:2671:18)
    at __webpack_require__ (electron/js2c/browser_init.js:20:30)
    at loader (electron/js2c/browser_init.js:2060:41)
    at Object.powerMonitor (electron/js2c/browser_init.js:7079:17)
    at __importStar (/home/user/sample-electron-crash/index.js:5:96)
    at Object.<anonymous> (/home/user/sample-electron-crash/index.js:10:18)
    at Module._compile (internal/modules/cjs/loader.js:880:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:892:10)
    at Module.load (internal/modules/cjs/loader.js:735:32)
A JavaScript error occurred in the main process
Uncaught Exception:
Error: The 'powerMonitor' module can't be used before the app 'ready' event
    at Object.module.<computed> [as on] (electron/js2c/browser_init.js:6800:30)
    at Object../lib/browser/api/power-monitor.ts (electron/js2c/browser_init.js:2671:18)
    at __webpack_require__ (electron/js2c/browser_init.js:20:30)
    at loader (electron/js2c/browser_init.js:2060:41)
    at Object.powerMonitor (electron/js2c/browser_init.js:7079:17)
    at __importStar (/home/user/sample-electron-crash/index.js:5:96)
    at Object.<anonymous> (/home/user/sample-electron-crash/index.js:10:18)
    at Module._compile (internal/modules/cjs/loader.js:880:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:892:10)
    at Module.load (internal/modules/cjs/loader.js:735:32)

To Reproduce

$ git clone https://github.com/mtgto/sample-electron-crash.git
$ cd sample-electron-crash
$ yarn install
$ yarn build
$ yarn start

Screenshots

N/A

Additional Information

Stacktrace says __importStar calls powerMonitor. It happens when tsconfig uses esModuleInterop: true (default is true)
In macOS, app doesn't crash.

@jacobq
Copy link
Contributor

jacobq commented Jan 9, 2020

Why can't you just use const electron = require('electron') instead of import? It looks like your code is causing the powerMonitor module to be loaded right away, but the docs are pretty clear:

This module cannot be used until the ready event of the app module is emitted.

Also, does electron support TypeScript natively? If so, please specify URL that discusses this. As much as I like TS, keeping reproduction code as simple as possible is desirable, so I would've preferred it to be written directly in JS.

In macOS, app doesn't crash.

I suspect that to be due to "luck". When you use something in a way that is forbidden by the specs you should expect undefined behavior.

@vladimiry
Copy link

vladimiry commented Jan 9, 2020

Can confirm the issue is reproducible.

Although TypeScript developers normally do (no import-start helper involved in both cases)

import { app, BrowserWindow } from "electron";

let mainWindow: BrowserWindow;

or

import { app } from "electron";

let mainWindow: Electron.BrowserWindow;

instead of

import electron, { app } from "electron";

let mainWindow: electron.BrowserWindow;

Besides, in a real-world scenario, you would probably go with Webpack/Rollup/etc bundlers which often have own module loading wrappers that usually do require under the hood.

@jacobq
Copy link
Contributor

jacobq commented Jan 9, 2020

Can confirm the issue is reproducible.

Sure, but is it a bug in electron? It looked to me like this only happens when the application code attempts to load the powerMonitor module before the ready event occurs, which is forbidden by the spec.

Stacktrace says __importStar calls powerMonitor. It happens when tsconfig uses esModuleInterop: true (default is true)

Exactly. That is what is causing the problem in your code. --esModuleInterop (docs say the default is false, BTW) causes __importStar to be emitted, and that function acts like import * which would trigger loading of powerMonitor just as would import { powerMonitor }, which is what causes the exception to be thrown.

v8::Local<v8::Value> PowerMonitor::Create(v8::Isolate* isolate) {
if (!Browser::Get()->is_ready()) {
isolate->ThrowException(v8::Exception::Error(
gin::StringToV8(isolate,
"The 'powerMonitor' module can't be used before the "
"app 'ready' event")));
return v8::Null(isolate);
}
return gin::CreateHandle(isolate, new PowerMonitor(isolate)).ToV8();
}

@vladimiry
Copy link

In my opinion, loading the library and using it are different things. So from this point that would be a bug in Electron. But it's clear the Electron initializes the instance eagerly/on-module-loading

const powerMonitor = createLazyInstance(createPowerMonitor, PowerMonitor, true)

So maybe it's just documentation issue and the docs should be adjusted like you can't load or use the module.

Given the import start helper code:

var __importStar = (this && this.__importStar) || function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
    result["default"] = mod;
    return result;
};

The power monitor module and all of them listed on electron are clearly accessed here: mod[k];.

And well, it's better not to use TS import-start stuff a lot since if affects performance, you know that for (var k in mod) if stuff.

@vladimiry
Copy link

vladimiry commented Jan 9, 2020

Sure, but is it a bug in electron?

Actually I think Electron could do something like:

if (process.platform === 'linux') {
  require("electron").app.once("ready", () => {
    powerMonitor.on('newListener', (event:string) => {
      if (event === 'shutdown' && powerMonitor.listenerCount('shutdown') === 0) {
        powerMonitor.blockShutdown()
      }
    });

    powerMonitor.on('removeListener', (event: string) => {
      if (event === 'shutdown' && powerMonitor.listenerCount('shutdown') === 0) {
        powerMonitor.unblockShutdown()
      }
    })
  })
}

Though if for the instance powerMonitor module gets loded lazily, after ready event has already been fired on app, the event is not going to be replayed and so subscribing logic won't be triggered. Introducing something like app.promises.onReady would help but would be somewhat messy at the same time.

Instead of eagerly calling powerMonitor.on method in along with module loading (so Electron itself violates the module using rules described in the documentation):

if (process.platform === 'linux') {
powerMonitor.on('newListener', (event:string) => {
if (event === 'shutdown' && powerMonitor.listenerCount('shutdown') === 0) {
powerMonitor.blockShutdown()
}
})
powerMonitor.on('removeListener', (event: string) => {
if (event === 'shutdown' && powerMonitor.listenerCount('shutdown') === 0) {
powerMonitor.unblockShutdown()
}
})
}

@ckerr
Copy link
Member

ckerr commented Jan 28, 2020

Yeah, I agree this is a bug in Electron rather than an issue with ts wildstar.

For example, you can remove typescript from the equation altogether and instead try to run the sample code from the top of electron/docs/api/power-monitor.md:

This module cannot be used until the ready event of the app module is emitted.

For example:

const { app, powerMonitor } = require('electron')

app.on('ready', () => {
  powerMonitor.on('suspend', () => {
    console.log('The system is going to sleep')
  })
})

That code causes the same failure on require.

If you don't mind, I'm going to edit the issue topic to reflect that the issue doesn't require TypeScript

@ckerr ckerr changed the title Typescript powerMonitor Issues importing powerMonitor from electron before 'ready' event causes crash Jan 28, 2020
ckerr added a commit that referenced this issue Jan 28, 2020
powerMonitor can't be used until the app is ready; however, on Linux,
powerMonitor.on() was called as soon as lib/browser/api/power-monitor.ts
was loaded.

This patch takes @vladimiry's suggestion of wrapping that in an
app.on('ready') handler to prevent powerMonitor.on() from being called
prematurely.

Fixes #21716
ckerr added a commit that referenced this issue Jan 28, 2020
* fix: use powerMonitor.on() only after app is ready

powerMonitor can't be used until the app is ready; however, on Linux,
powerMonitor.on() was called as soon as lib/browser/api/power-monitor.ts
was loaded.

This patch takes @vladimiry's suggestion of wrapping that in an
app.on('ready') handler to prevent powerMonitor.on() from being called
prematurely.

Fixes #21716
trop bot pushed a commit that referenced this issue Jan 28, 2020
powerMonitor can't be used until the app is ready; however, on Linux,
powerMonitor.on() was called as soon as lib/browser/api/power-monitor.ts
was loaded.

This patch takes @vladimiry's suggestion of wrapping that in an
app.on('ready') handler to prevent powerMonitor.on() from being called
prematurely.

Fixes #21716
trop bot pushed a commit that referenced this issue Jan 28, 2020
powerMonitor can't be used until the app is ready; however, on Linux,
powerMonitor.on() was called as soon as lib/browser/api/power-monitor.ts
was loaded.

This patch takes @vladimiry's suggestion of wrapping that in an
app.on('ready') handler to prevent powerMonitor.on() from being called
prematurely.

Fixes #21716
zcbenz pushed a commit that referenced this issue Jan 29, 2020
* fix: use powerMonitor.on() only after app is ready

powerMonitor can't be used until the app is ready; however, on Linux,
powerMonitor.on() was called as soon as lib/browser/api/power-monitor.ts
was loaded.

This patch takes @vladimiry's suggestion of wrapping that in an
app.on('ready') handler to prevent powerMonitor.on() from being called
prematurely.

Fixes #21716

* refactor: handle import powerMonitor timing issue

Fix the previous commit's app-is-ready handler by checking to see if
app is already ready when power-monitor.ts is loaded.

* refactor: use app.whenReady() for simpler logic

* chore: use a consistent comment formatting style

Co-authored-by: Charles Kerr <ckerr@github.com>
@sofianguy sofianguy moved this from Unsorted Issues to Fixed for Next Release in 7.2.x Feb 15, 2020
@sofianguy sofianguy moved this from Fixed for Next Release to Fixed in 7.1.11 in 7.2.x Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
7.2.x
Fixed in 7.1.11
Development

Successfully merging a pull request may close this issue.

5 participants