[Bug]: Electron 21 breaks node-api stability guarantees

[Julusian]

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 a bug report that matches the one I want to file, without success.

Electron Version

21

What operating system are you using?

Ubuntu

Operating System Version

Ubuntu 20.04

What arch are you using?

x64

Last Known Working Electron version

20

Expected Behavior

The NodeJS docs, state that the Node-API is stable api, intended so that modules do not need recompiling to work with newer version of NodeJS.
While I do not see Electron being referenced on that page, as Electron wraps specific versions of NodeJS and I have not seen reference to not following that stability guarantee, I would expect that Electron would follow the same Node-API support and stability promises as the version of NodeJS being wrapped.

At the very least, the NodeJS docs should be updated to make the methods deprecated or have a compatability warning, but ideally they shouldnt be broken at all.

Actual Behavior

In the blog post about enabling of v8 memory cages, it clearly states that "native modules which rely on this functionality in V8 will need to be refactored to continue working in Electron 20 and later.", which violates the Node-API stability.

The docs do say "This is a fairly rare use case", but out of the 5 native modules I have contributed to, all of them use this technique. So from my experience, every native nodejs module is now a liability.

https://www.npmjs.com/package/sharp is a good example of an incredibly popular library that breaks because of this

Testcase Gist URL

No response

Additional Information

I am now facing the dilemma of how to handle this breaking api issue. I could:

1. always do an extra memcpy and worsen performance unnecessarily for nodejs users
2. produce separate builds for nodejs and electron like we used to with nan

Of course, this doesnt help in cases where the module author is unaware of this electron incompatability, which I expect will be a lot of them. Without proper documentation in the node-api docs, how would they be expected to know?

I expect that this api breakage will cause users of them a lot of pain in transitioning to electron 21. This could end up with long chains of users waiting for dependencies of dependencies to update before they can update electron

[MarshallOfSound]

intended so that modules do not need recompiling to work with newer version of NodeJS

Correct, and modules do not need to be rebuilt to continue linking against / being loaded in Electron 21. That is the problem that Node-API was built to solve and we don't even claim to maintain that stability 🤷 Although I think we have purely by not-doing-the-wrong-thing.

I don't think at any point the Node-API makes any claims about stability across different permutations of V8 flags (which technically could happen with third-party / different NodeJS builds too).

It is relatively common behaviour if your application depends on a V8 flag in order to function it's implementation must be behind a #ifdef to ensure that it can't be used when that code is in play. So I'd claim that this issue isn't with Electron, rather than the symbols exposed by nodejs-api did not take this into account when the API layer was designed and now they're in a bad spot.

A reasonable middle-ground is probably to expose a new symbol that returns whether the memory cage is enabled or not.

I am now facing the dilemma of how to handle this breaking api issue. I could:

I think you're missing option 3 here which his by far the most ideal, allocate the memory in the cage and then write into it instead of allocate outside the cage and pointing a nodejs buffer at the external memory.

In general I understand where you're coming from, especially as native module authors / maintainers but this decision was made considering the security of the ecosystem at large and one side of the equation would always be unbalanced. Either we'd be disabling an important new V8 security feature drastically deviating our security posture from Chromium or we'd be putting a subset (small by our math) of native modules into a non-working state.

[Julusian]

Correct, and modules do not need to be rebuilt to continue linking against / being loaded in Electron 21.

True, they do load but the behavioural differences would be classed a breaking change. Especially when it works differently to the version of node that is wrapped.
On a related note, does anyone run unit tests for npm modules against electron? It is common to see them run against all the maintained versions of node, but I've never seen electron be in that matrix.

That is the problem that Node-API was built to solve and we don't even claim to maintain that stability

Yes you don't claim to maintain that, but you don't state either way so (I expect) it was widely assumed that you would do so to the benefit of the ecosystem. In fact your docs make no reference to node-api https://www.electronjs.org/docs/latest/tutorial/using-native-node-modules

I don't think at any point the Node-API makes any claims about stability across different permutations of V8 flags (which technically could happen with third-party / different NodeJS builds too).

But they have said "allow modules compiled for one major version to run on later major versions of Node.js without recompilation" and make very little reference to v8 in their docs. The node-api is designed to isolate modules from the v8 api, so why should I care about v8 flags?

I have suspected for a while that nodejs are promising too much in this api being infinitely forwards compatible, but that is a different topic to take up with them.

I think you're missing option 3 here which his by far the most ideal, allocate the memory in the cage and then write into it instead of allocate outside the cage and pointing a nodejs buffer at the external memory.

Yes, but this is not always an option. (see below)

In general I understand where you're coming from, especially as native module authors / maintainers but this decision was made considering the security of the ecosystem at large and one side of the equation would always be unbalanced. Either we'd be disabling an important new V8 security feature drastically deviating our security posture from Chromium or we'd be putting a subset (small by our math) of native modules into a non-working state.

I do appreciate that this change is needed, my concern is that it comes as a surprise that many application developers and native module developers are going to be surprised by. It is very possible that I have missed some discussion that happened before the change was made, but I would definitely have had some input on how it should have been handled if I had seen anything sooner.

---

A good case study for this concern is sharp. That module gets over 2 million installs from npm a week, and is affected by this.
It sounds like the author was completely unaware that something would need to be done, so this could result in a load of application developers being blocked on updating to 21+.
For sharp, option 3 may not be easily possible. Remember that you can't allocate buffers when not on the event loop thread. If reading an image from file, you won't know how big of a buffer you need before you are halfway through your cpu heavy work. Or as sharp uses an external library (libvips), it may be that providing the output buffer is not an option.
So in these cases, that means copying will likely be necessary. Maybe it would be possible to refactor the work to be done as two async tasks, taking a break in the middle solely to allocate some buffers but that will not be an easy or safe change to make (that would be a semver major change to me).

At the very least we can look at the short term, where copying will likely be necessary while refactoring is done to allow for more performant options.
This hits the next issue of that copying a 8MB buffer will not be cheap. 8MB is pretty reasonable for pixel buffers, as that is only 1080p. A typical iphone photo is 12megapixels, which would be 48MB (in raw rgba).
If this copying is done for nodejs users, there will be multiple complaints about the performance hit in the new versions (for some workloads it will probably end up cheaper to do it all in js). So then the question becomes how can we detect whether to copy or not, which there isnt a clear answer for.
This 'temporary' copy could probably be shuffled off to another async task, to avoid blocking the main thread, but could that have other side effects due to changing order of execution behaviour?

---

I suppose my key takeaway from this is that there is more Electron should have done, and more that you still can do.

Was this discussed with the nodejs team at all? Perhaps this is something they should be considering doing in 19/20, and you could work together to phase out the methods from the api completely.

It would have been much better to have a deprecation period of a release or two, with a warning written to the console when the method was called.
I expect that any module which uses this method to get multiple issues opened over the next few months as users start to experience issues. Here is my first, in a library which isnt even a native module, but has a dependency on one.
This deprecation period could easily have been all versions of 19+ after it was decided to make this change. That would have been a small change, and would have made this upcoming breakage be very visible.

This needs to be documented in the node-api docs, as that is the only documentation for this api.
More could be done that this to help avoid calls to the method (a compile time flag to disable the methods in the headers?), but updating the documentation is a required start.
A way of checking for the memory cage at runtime is also necessary, for all currently supported versions of nodejs (and ideally back further). It can't require targetting a newer node-api. For now I will probably have to go with checking the value of process.versions.electron
New code will be added to new or existing modules which uses that method because it is in the docs without a warning and testing with nodejs will find it to work perfectly. Later, someone using electron will update that to that new version and discover the crash.
#26593 is a good reminder that the electron docs are lacking documentation on the differences, so how are developers supposed to know that their nodejs code may not work with electron?

I would like to question the decision to make calls to this method crash the application. I would much prefer the buffer allocation to fail, rather than an application crash. At least that gives the user a chance of continuing to use the application rather than it crashing when they do something.
Because it crashes at the time of calling the method rather than startup, it could be minutes or hours until it gets called and crashes. I expect some of crashes to be discovered by users because of this, which will make a big mess for application devs.
Perhaps the symbol should have been removed completely, so that modules which use it wont be at all loadable? (I dont know if this would work, or if that would break module which dont use it)

[Nantris]

This change could make upgrading to v21 extremely difficult for some applications to do before v20 goes out of maintenance, depending on the speed that the third-party module developers and the community can update their native module code.

The planned breaking change was only documented about 75 days before it was implemented which isn't much lead time. I was not even aware of the planned breaking change until it was live - and it's a rather substantial one.

[lovell]

A reasonable middle-ground is probably to expose a new symbol that returns whether the memory cage is enabled or not.

👍 As maintainer of sharp I would love to continue to support Electron users, so the suggestion of providing a stable API to query for this feature and therefore the need to copy memory buffers would be really helpful.

Thanks, as always, for maintaining Electron ❤️

[MarshallOfSound]

@lovell Can you clarify (I can look through sharp too) how sharp allocates the memory it then passes to napi_create_external_arraybuffer?

I'm wondering if an even better solution here (that I haven't even really explored technically but wondering what you think) would be to expose primitives for:

• malloc_in_v8_cage
• free_from_v8_cage

That have the exact same syntax as malloc and free but allocate and free memory from inside the cage. You'd be susceptible to cage size limitations, but then existing calls to napi_create_external_arraybuffer would Just Work ™️.

[lovell]

@MarshallOfSound sharp relies on various libraries to encode images and delegates almost all memory allocation to those dependencies, so sadly I don't think adding a custom memory API will help as we don't control the call sites.

It would be a bit of a hack, but preloading a shared library that provides malloc and free at runtime could potentially work, something along the lines of a LD_PRELOAD=/path/to/electron_custom_v8_cage_malloc.so.1 approach, but then providing a custom memory allocator might defeat the security improvement of introducing the V8 cage in the first place.

I think I'd still prefer a stable API to detect the cage up-front rather than having the process crash. Failing that, we can add something explicit to the sharp API that allows someone developing for Electron 21+ to opt-in to copying the Buffer (and therefore opt-in to the performance and memory fragmentation cost).

[mhdawson]

We discussed in the Node-API team meeting today and our current thought that we could:

1. add a new napi_status in https://github.com/nodejs/node/blob/main/src/js_native_api_types.h, maybe napi_no_external_buffers_allowed.
2. add code to napi_create_external_buffer and napi_create_external_arraybuffer which at compile time check if the pointer cage is enabled and if so returns that error.
3. add a define, maybe NODE_API_NO_EXTERNAL_BUFFERS_ALLOWED, which if specified by an addon author, removes the definitions of napi_create_external_buffer and napi_create_external_arraybuffer so that they cannot be used in the addon. this allows authors to avoid using the methods by accident.
4. add additional documentation that explains the new error code and define, and strategies as to how to best cope

@lovell would that provide a reasonable way for you to be able to detect up front?

[nkallen]

Hi - I'm also developing an electron app with a native component with off-heap buffers. I'm trying to figure out how/if to upgrade to Electron 21. Will the proposal

malloc_in_v8_cage
free_from_v8_cage

Be possible on any thread?

[lovell]

@mhdawson A compile-time check that results in a status return at runtime (rather than the current process crash) sounds good to me, and is certainly enough to detect and workaround this change, thank you.

At the node-addon-api level, a possible implicit solution might be adding something like Napi::Buffer::NewOrCopy() to complement the existing, explicit Napi::Buffer::New() and Napi::Buffer::Copy() methods. The signature of this would probably match ::New() and, if a copy was required, would ignore any provided Finalizer.

[mhdawson]

@nkallen from our discussion in the node-api team meeting, instead of using malloc_in_v8_cage, free_from_v8_cage you can just allocate a buffer using

napi_status napi_create_buffer(napi_env env,
                               size_t size,
                               void** data,
                               napi_value* result)

That should create the buffer in the right way and pass it back to you in the output data.

[nkallen]

@mhdawson and that needs to be done on the main js thread right? A common workflow requires doing work in a thread pool and allocating memory as a result of the computation. In my case I'm faceting CAD models into polygonal representations and need to send these 3d models to Electron/webgl. I'm thinking the best practice might be to pre-allocate a large buffer and maintain my own freelist or something.

[mhdawson]

@nkallen yes I believe it would need to be called on the main thread. I agree your suggestion of pre-allocating one or more buffers and then managing re-use would make sense to me.

[nkallen]

OK thanks. I apologize for being negative, but I do want to note that this will add a significant amount of work for my project and I really wish Electron would reconsider the memory cage.

[mhdawson]

I've created this PR to implement my suggestion above - nodejs/node#45181

[mhdawson]

I tested the PR above manually but if somebody could test in the context of electron that would be great. @MarshallOfSound, @lovell

[Julusian]

I agree that some workflows will make it impractical to preallocate a buffer.

And I wouldn't be surprised that for sharp performing a napi_create_buffer_copy might have too much performance cost for large images. If so that would mean: do computation in async worker, allocate buffer on event loop, perform copy on async worker. That doesnt feel like a nice flow to implement, and that second async step could introduce some weird quirks.

This isn't a dealbreaker, but would be good to consider

[WillAvudim]

Resubmitting my request here per Charles' (@ckerr) suggestion:

Problem Description

It's not uncommon for a desktop computer nowadays to have 64GB memory. There is a large array of desktop applications dealing with ML, image processing, rendering, e.g. displaying financial charts or annotation tools for medical images that require large datasets to be loaded in-memory and accessed for every mouse move (e.g. to display the precomputed inference of the hierachical bayesian network for the selected point in time, which is kept in a multi-gigabyte Float32Array).
Such desktop applications do not benefit from the security context, we run with nodeResolution set to true, in fact, here's the exact config we run with:

  const mainWindow = new BrowserWindow({
    width: 2800,
    height: 2000,
    webPreferences: {
      // Electron memory cage is enforced starting with version 21:
      // https://github.com/electron/electron/issues/35241
      preload: path.join(__dirname, "index_preload_njs.js"),
      // preload: "/storage/mono/out/electron/preload.js",
      contextIsolation: false,
      nodeIntegration: true,
      nodeIntegrationInWorker: true,
      nodeIntegrationInSubFrames: true,
      sandbox: false,
      webSecurity: false,
      allowRunningInsecureContent: true,
      additionalArguments: [
        // Most likely doesn't take effect and can be removed.
        "-r",
        "ts-node/register/transpile-only",
      ]
    }
  })

There is no user code to run. Obviously, we do not include any hosted 3rd party content. All .js files limited to well-known frameworks (Vue3, react, etc) and vetted before getting incorporated into the code base and referenced directly. We do not benefit from the security benefits that you currently promote, it's simply unnecessary for us, it is the memory that the Renderer can immediately access we are after.

I understand that maintaining such a build, especially with your insane pace of releases (we can only attempt to upgrade every few years, VSCode was behind for several years before they could catch up with you), would waste a lot of time unnecessarily.

Proposed Solution

Would it be possible to provide an infrequent, e.g. once in 2-3 years, build without memory caging and, if possible, without memory compression?

Alternatives Considered

There is a thread currently discussing the problem: #35241
And, apparently, the top two alternatives considered so far are to switch to the web and drop Electron entirely, or use "a native webview widget in a QT application". Neither can be done without massive rewrites of the application.

Additional Information

Well, we actually love Electron overall, and we do want to continue using it going forward. Please help us leverage the entire available memory from the Renderer, it's vital to many applications.

[zyzski]

would be nice to see a recommended way to identify problematic libraries. luckily my issue was with zeromq, which was listed as fixed in this thread

thank you @Bartel-C8

[taras-nagorny]

i've got this issue with 20.3.12 electron

[Xano]

it present in electron 20 since #36626 (20.3.9 regarding patch notes)

+#if defined(V8_ENABLE_SANDBOX)
+  return napi_set_last_error(env, napi_no_external_buffers_allowed);
+#endif

so in addons builded with electron 20.3.9+ it's impossible to use external buffers