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

[Discussion] Requiring Native Modules in the Renderer Process to be NAPI or Context Aware #18397

Closed
MarshallOfSound opened this issue May 22, 2019 · 68 comments
Assignees

Comments

@MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented May 22, 2019

Native modules in Electron

Loading native modules in Electron has always been tricky. It's a pain to rebuild, to link to the right node.lib/dll, and to ensure your NODE_MODULE_VERSION is correct. And there's another problem behind the scenes that most users have never had to deal with: Node doesn't let you load multiple instances of a native module in the same process, even in different v8 contexts. Imagine this scenario:

  1. Load https://google.com in BrowserWindow
    • Process is launched.
    • Preload script runs and loads native module X
  2. Load https://google.com/foo in BrowserWindow
    • Chromium decides to reuse the process
    • Preload script runs and loads native module X
    • FAILURE - native module X loaded twice in the same process

We address this in Electron by patching Chromium to create a new process for every navigation. This mostly works but still has a few edge cases (e.g. #4025, #12045, #17576) and a few side effects:

  • We lose all the performance gains from the Chromium plz-navigate changes 😢
  • We have strange behavior with window.open and window.opener as we force child windows into separate processes sometimes. 😢
  • Our patches occasionally have regressions because this isn't a process model that Chromium accepts / tests. 😢

We've always thought "wouldn't it be cool if we could change this". Now, with Node 12's Worker Threads, we see a way to do that.

Worker Threads

Node ran into the same issue when they added worker threads: native modules could not be loaded in multiple workers. Node solved this by introducing the concept of "Context Aware" native modules. This is how native modules tell Node that they are safe to be loaded in multiple v8::Contexts. Nan has a handy helper (NAN_MODULE_WORKER_ENABLED) for doing this.

Context Aware modules and NAPI modules can be instantated multiple times in Node even without our patched process model. As they become more common, our patches will bring less benefit and be more redundant.

What does this mean for Electron?

Right now, nothing. However, at some point in the future -- probably in a few major versions -- we intend to remove our process model patches. At that point, any native Node modules loaded in the renderer process must be either NAPI or Context Aware.

This won't happen tomorrow. It probably won't happen this year. But it will happen, so if you're using or maintaining a native module, you should start looking at the work to mark it as Context Aware. For lots of modules it's as easy as replacing NODE_MODULE with NAN_MODULE_WORKER_ENABLED. For modules that require cleanup, it can be more involved.

You may be wondering why we're changing something that (mostly) works? The main reasons are:

  • Performance. If we don't restart the renderer for every navigation, certain same-site navigations will be much faster than before. 😄
  • Security. When we follow Chromium's process model, we benefit from all the the optimizations and decisions they have made around Site Isolation and related initiatives. 😄
  • Maintenance. Our patches are not easy to keep working, especially as Chromium makes more and more process model performance improvements and optimizations. Removing the patches removes a large maintenance burden. 😄

What does this look like for Native Module X?

The amount of work to become "Context Aware" will be different for every module. For native modules that act as proxies for native getters and setters -- i.e. they just expose JS apis to acccess native APIs -- it's normally as easy as adding a NAN_MODULE_WORKER_ENABLED declaration. You can find an example of that in this pull request.

Some modules will need to do cleanup when the context is destroyed to ensure the module doesn't leaking memory or to cause crashes. You can add hooks to clean up after yourself with node::AddEnvironmentCleanupHook A minimal example of this can be found in this pull request.

Timeline

  • Land the app.allowRendererProcessReuse option in Electron 6
  • Have the first deprecation warning land in Electron 7 for non-context aware native modules
  • Have a deprecation warning land in Electron 8 for the default value of app.allowRendererProcessReuse to switch
  • Switch the default value of app.allowRendererProcessReuse to true in Electron 9
  • Deprecate the ability to change app.allowRendererProcessReuse in Electron 10
  • Remove the ability to change app.allowRendererProcessReuse in Electron 14
@orange4glace
Copy link

@orange4glace orange4glace commented May 23, 2019

This is cool. I just encountered such situation that needs multiple native module instances, while it is little sad that it only supports node with V8, not N-API.

@bughit
Copy link
Contributor

@bughit bughit commented May 30, 2019

Are native modules the only reason for the "new process for every navigation"? The following explanation from @zcbenz seems to suggest that node can't fully tear down its one js environment per process, so a new one can take its place.

This is because Node is currently designed to have only one instance in one process, and it doesn't cleanup all the resources one JavaScript environment is destroyed.

@MarshallOfSound
Copy link
Member Author

@MarshallOfSound MarshallOfSound commented May 30, 2019

@bughit That situation has changed in recent times, as mentioned in the original issue text with the introduction of worker threads node now supports multiple instances of node per-process along with proper teardown support (this is what making your addon context aware supports). We already teardown node environments when the nodeIntegrationInSubFrames flag is enabled

@bughit
Copy link
Contributor

@bughit bughit commented May 30, 2019

Would you consider adding an experimental option that would deactivate the "process for every navigation" mode even before you completely drop you process model patches?

Assuming that's less work than the full job.

@MarshallOfSound
Copy link
Member Author

@MarshallOfSound MarshallOfSound commented May 30, 2019

@bughit See the PR linked --> #18396 in particular the timeline outlined in that PR, what you suggest is already what's being done 😄

@AnalyzerTech

This comment was marked as off-topic.

@ghost

This comment was marked as off-topic.

@cynx
Copy link

@cynx cynx commented Sep 1, 2019

Can the decision to remove affinity be reviewed ? I currently don't see a way to open multiple BrowserWindows quickly enough without affinity. This would impact many developers as discussed here - #16319

@jacobq
Copy link
Contributor

@jacobq jacobq commented Nov 25, 2019

Is there a way to suppress the deprecation warning? I am upgrading to electron 7.x and plan to update my native modules soon, but right now this message floods the log when I run my test suite. (I tried explicitly setting app.allowRendererProcessReuse = false; at the beginning of the main thread, but this didn't seem to make the warning disappear.)

@alba-ado
Copy link

@alba-ado alba-ado commented Dec 14, 2021

Why is this issue closed? What are we going to do with the libraries that are not N-API compatible in the latest electron versions now?

@TanninOne
Copy link
Contributor

@TanninOne TanninOne commented Dec 15, 2021

Why is this issue closed? What are we going to do with the libraries that are not N-API compatible in the latest electron versions now?

Either you port the module, find an alternative or you only use the module from the browser process and use ipc to delegate all uses of the module to it.
EDIT: or you stick with an older version of electron but that may become a security issue with time.

@alba-ado
Copy link

@alba-ado alba-ado commented Dec 16, 2021

By doing this electron is not pushing developers to use a safer code, they are pushing us to be stuck using older versions of the electron because the projects are too big to modify and use too many libraries.

@TanninOne
Copy link
Contributor

@TanninOne TanninOne commented Dec 16, 2021

By doing this electron is not pushing developers to use a safer code, they are pushing us to be stuck using older versions of the electron because the projects are too big to modify and use too many libraries.

That is unfortunately my experience with the entire js/node/npm/electron environment: You need to constantly invest time on code maintenance because the entire environment is constantly changing, modules that stop being maintained commonly stop working after months or a few years, absolutely everything is built on sand.
I've worked with C++/Qt, C# and Java before, all waaaaaay more productive once a project reaches a certain size because stuff that works will continue to work and you can use your time to build new functionality instead of having to redo the same stuff all the bloody time.

If this frustrates you: Don't use electron, get out while you can. The fact that they're now doing a major release every 8 weeks and genuinely think that's a good thing tells you all you need to know.

@jerrygreen
Copy link

@jerrygreen jerrygreen commented Dec 16, 2021

js/node/npm/electron environment: You need to constantly invest time on code maintenance because the entire environment is constantly changing, modules that stop being maintained commonly stop working after months or a few years, absolutely everything is built on sand

I feel what you’re saying but it’s not the problem with ecosystem. Ecosystem gives you an ability to easily update dependencies. It’s up to you to deal with according problems. If you’re not having enough time/desire to resolve that, just don’t update modules, never ever.

Additionally to that I’d mention, using ^ or ~ or ~> (^ in npm) in listing your dependencies is where your problem also comes from (this is related to all those ecosystems with package managers, not only node, but ruby, etc). Those special symbols tell the package managers to use the latest patch (0.0.X) version which are commonly considered safe to update to since they «don’t involve breaking changes, nor new functionalities», - but in practice it’s often a lie, because it can still break some code they didn’t think of (sometimes your code). Unfortunately though, npm uses caret symbol (^) by default when you «npm i» or «yarn add» something, which involves all those problems and makes many people think «js/node/npm» is flawful and built on sand.

NOTICE: Yes, the version is of course written to «lock» file, which means it’s kinda safe anyway because it’s used primarily, rather than package.json, so versions shouldn’t be updated just by itself without a notice. In practice, those lock files sometimes have conflicts, people have to completely replace lock file sometimes, and nobody really analyzes this newly generated list in PRs. Especially when order of those items in lock files is slightly shuffled, and git will show you a lot of fake changes (because the items were misplaced, but not actually changed). So people normally analyze people code, not automatically generated something, - which is understandable... By all those reasons, it’s unreliable to rely on lock files either, and only list strict versions in your package.json instead.

Once an OS developer recommended me to never use this caret symbol for node and setup my config to change the default, - because it introduces all those mentioned problems. He knew something about resilience, about reliability, he was an OS developer, so I listened to his advice. And guess what? I never had this problem again (except in the projects where this practice wasn’t yet applied). So that is what I can recommend to you, and to everybody who faces this problem. You can do this by simply:

npm config set save-prefix=""

Or, for yarn:

yarn config set save-prefix ""

@alba-ado
Copy link

@alba-ado alba-ado commented Dec 16, 2021

Yeah, I fix the versions when I need to. That's why I say I'm stuck with an older electron version. However, it is nice to be able to update from time to time for improved security and bug fixes.

@TanninOne
Copy link
Contributor

@TanninOne TanninOne commented Dec 16, 2021

I feel what you’re saying but it’s not the problem with ecosystem. Ecosystem gives you an ability to easily update dependencies. It’s up to you to deal with according problems. If you’re not having enough time/desire to resolve that, just don’t update modules, never ever.

I would be very careful with this advice, it depends on the software whether this is a good idea. Security issues become known in packages you use (directly and indirectly) and from the point they are known publicly, every script kiddy under the sun will start looking for unpatched software.
Based on recent news, would you say "stick with your log4j version and never update it" is a good idea?

This is an ecosystem problem because in other ecosystems you can update the foundations to stay up-to-date with security fixes without them breaking compatibility. A library like Qt you can do updates for 10 years with no or minimal upkeep while with electron you'll be lucky to go longer than 8 weeks.
It is an ecosystem problem with npm because every package has dozens of dependencies for the most miniscule pieces of functionality (I've seen npm modules, plural, actually being used that contained a single line of code) to the point where no one is capable of monitoring the changelogs for the stuff they're using.
It's an ecosystem problem because with "professional" software if they do semantic versioning you can trust that at the very least you can install patch releases without having to expect functional changes, in the npm ecosystem all bets are off. This: "but in practice it’s often a lie," is true for the npm ecosystem, not in general.

@m0059
Copy link

@m0059 m0059 commented Dec 20, 2021

Native modules in Electron

Loading native modules in Electron has always been tricky. It's a pain to rebuild, to link to the right node.lib/dll, and to ensure your NODE_MODULE_VERSION is correct. And there's another problem behind the scenes that most users have never had to deal with: Node doesn't let you load multiple instances of a native module in the same process, even in different v8 contexts. Imagine this scenario:

  1. Load https://google.com in BrowserWindow

    • Process is launched.
    • Preload script runs and loads native module X
  2. Load https://google.com/foo in BrowserWindow

    • Chromium decides to reuse the process
    • Preload script runs and loads native module X
    • FAILURE - native module X loaded twice in the same process

We address this in Electron by patching Chromium to create a new process for every navigation. This mostly works but still has a few edge cases (e.g. #4025, #12045, #17576) and a few side effects:

  • We lose all the performance gains from the Chromium plz-navigate changes 😢
  • We have strange behavior with window.open and window.opener as we force child windows into separate processes sometimes. 😢
  • Our patches occasionally have regressions because this isn't a process model that Chromium accepts / tests. 😢

We've always thought "wouldn't it be cool if we could change this". Now, with Node 12's Worker Threads, we see a way to do that.

Worker Threads

Node ran into the same issue when they added worker threads: native modules could not be loaded in multiple workers. Node solved this by introducing the concept of "Context Aware" native modules. This is how native modules tell Node that they are safe to be loaded in multiple v8::Contexts. Nan has a handy helper (NAN_MODULE_WORKER_ENABLED) for doing this.

Context Aware modules and NAPI modules can be instantated multiple times in Node even without our patched process model. As they become more common, our patches will bring less benefit and be more redundant.

What does this mean for Electron?

Right now, nothing. However, at some point in the future -- probably in a few major versions -- we intend to remove our process model patches. At that point, any native Node modules loaded in the renderer process must be either NAPI or Context Aware.

This won't happen tomorrow. It probably won't happen this year. But it will happen, so if you're using or maintaining a native module, you should start looking at the work to mark it as Context Aware. For lots of modules it's as easy as replacing NODE_MODULE with NAN_MODULE_WORKER_ENABLED. For modules that require cleanup, it can be more involved.

You may be wondering why we're changing something that (mostly) works? The main reasons are:

  • Performance. If we don't restart the renderer for every navigation, certain same-site navigations will be much faster than before. 😄
  • Security. When we follow Chromium's process model, we benefit from all the the optimizations and decisions they have made around Site Isolation and related initiatives. 😄
  • Maintenance. Our patches are not easy to keep working, especially as Chromium makes more and more process model performance improvements and optimizations. Removing the patches removes a large maintenance burden. 😄

What does this look like for Native Module X?

The amount of work to become "Context Aware" will be different for every module. For native modules that act as proxies for native getters and setters -- i.e. they just expose JS apis to acccess native APIs -- it's normally as easy as adding a NAN_MODULE_WORKER_ENABLED declaration. You can find an example of that in this pull request.

Some modules will need to do cleanup when the context is destroyed to ensure the module doesn't leaking memory or to cause crashes. You can add hooks to clean up after yourself with node::AddEnvironmentCleanupHook A minimal example of this can be found in this pull request.

Timeline

  • Land the app.allowRendererProcessReuse option in Electron 6
  • Have the first deprecation warning land in Electron 7 for non-context aware native modules
  • Have a deprecation warning land in Electron 8 for the default value of app.allowRendererProcessReuse to switch
  • Switch the default value of app.allowRendererProcessReuse to true in Electron 9
  • Deprecate the ability to change app.allowRendererProcessReuse in Electron 10
  • Remove the ability to change app.allowRendererProcessReuse in Electron 14

winksaville added a commit to winksaville/referral-analyzer that referenced this issue Jan 11, 2022
Made the tweaks below to get it to run but it still crashes with:

 Adding Moment timestamps and Decimals...
 Status: Parsing dates and amounts...
 TypeError: Cannot read property 'replace' of undefined
    at addMomentsAndDecimals (/home/wink/prgs/rust/myrepos/referral-analyzer/analyzeIncome.js:124:53)
    at Array.forEach (<anonymous>)
    at analyzeIncome (/home/wink/prgs/rust/myrepos/referral-analyzer/analyzeIncome.js:969:34)
    at async IpcMainImpl.<anonymous> (/home/wink/prgs/rust/myrepos/referral-analyzer/main.js:74:3)
 Status: Hmm... there was some kind of error.  Program stopped.
 Status: TypeError: Cannot read property 'replace' of undefined
    at addMomentsAndDecimals (/home/wink/prgs/rust/myrepos/referral-analyzer/analyzeIncome.js:124:53)
    at Array.forEach (<anonymous>)
    at analyzeIncome (/home/wink/prgs/rust/myrepos/referral-analyzer/analyzeIncome.js:969:34)
    at async IpcMainImpl.<anonymous> (/home/wink/prgs/rust/myrepos/referral-analyzer/main.js:74:3)


README.md
 - Add minimal info so I know how to "run" the app

main.js:
 - Was getting error:
   wink@3900x:~/prgs/rust/myrepos/referral-analyzer (master)
   $ electron .
   Electron could not be found. No hard resets for you!

   Changed the require('electron-reload')(__dirname) which "fixes" that
   error. But there is still a warning:

   (electron) The default value of app.allowRendererProcessReuse is deprecated, it is currently "false".  It will change to be "true" in Electron 9.  For more information please check electron/electron#18397
@ckerr ckerr unpinned this issue Jan 12, 2022
@ckerr ckerr pinned this issue Jan 12, 2022
@toghrulgasimov
Copy link

@toghrulgasimov toghrulgasimov commented Jan 18, 2022

same problem with electron >= 16.
setting value of app.allowRendererProcessReuse doesn't help.

@jcwii
Copy link

@jcwii jcwii commented Jan 19, 2022

Considering the complexity of this thread it's seemingly lacking an ELI5 — is this currently without a workaround?

@gpetrov
Copy link

@gpetrov gpetrov commented Jan 20, 2022

It is not about complexity is about that many open source node modules haven't switched yet to NAPI and are not Context Aware.

So now that Electron removed the app.allowRendererProcessReuse switch in Electron 14, we are stuck to Electron 13 and begging the writers of the native node modules to update - which is not very effective as it is huge undertaking. See all the linked topics ....

So I really wish that Electron left the app.allowRendererProcessReuse in place for at least Electron 14 and 15 - this will give the authors more time as it will take at least one year to switch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet