Enhancement: compatibility with Electron 21 will require copying all memory Buffers, will have performance cost

[Julusian]

Possible bug

Is this a possible bug in a feature of sharp, unrelated to installation?

• Running npm install sharp completes without error.
• Running node -e "require('sharp')" completes without error.

Are you using the latest version of sharp?

• I am using the latest version of sharp as reported by npm view sharp dist-tags.latest.

What is the output of running npx envinfo --binaries --system --npmPackages=sharp --npmGlobalPackages=sharp?

  System:
    OS: Linux 5.15 Ubuntu 22.04.1 LTS 22.04.1 LTS (Jammy Jellyfish)
    CPU: (8) x64 Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz
    Memory: 9.20 GB / 31.23 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 16.17.0 - /run/user/1000/fnm_multishells/799191_1664035140689/bin/node
    Yarn: 1.22.19 - /run/user/1000/fnm_multishells/799191_1664035140689/bin/yarn
    npm: 8.15.0 - /run/user/1000/fnm_multishells/799191_1664035140689/bin/npm
  npmPackages:
    sharp: ^0.31.0 => 0.31.0 

What are the steps to reproduce?

Run sharp with electron 21

What is the expected behaviour?

Sharp should be compatible

Please provide a minimal, standalone code sample, without other dependencies, that demonstrates this problem

{
  "dependencies": {
    "electron": "^21.0.0-beta.8",
    "sharp": "^0.31.0"
  },
  "scripts": {
    "test": "electron main.js"
  }
}

const sharp = require('sharp')

const semiTransparentRedPng =  sharp({
  create: {
    width: 48,
    height: 48,
    channels: 4,
    background: { r: 255, g: 0, b: 0, alpha: 0.5 }
  }
})
  .png()
  .toBuffer();

It is important to run the script through electron, and it has to be v21+

Please provide sample image(s) that help explain this problem

This is because of a change electron have made, which breaks compatability for some native modules https://www.electronjs.org/blog/v8-memory-cage

Electron 21 was released on the 27th September, and I have opened an issue to complain about their breakage of the guaranteed stable node-api that electron are doing here. electron/electron#35801

[826378:0924/183534.268000:ERROR:node_bindings.cc(146)] Fatal error in V8: v8_ArrayBuffer_NewBackingStore When the V8 Sandbox is enabled, ArrayBuffer backing stores must be allocated inside the sandbox address space. Please use an appropriate ArrayBuffer::Allocator to allocate these buffers.

[lovell]

A "more secure" feature that "will crash at runtime" sounds like a bit of an oxymoron.

The only workaround would be to take a copy of all memory before returning it to JS. This will affect Buffer and Stream-based output, as well as calls to metadata(). There will be an associated performance cost, so this will have to be optional.

Is there a way of querying the Electron runtime to determine if this "memory cage" feature is enabled, ideally via an ABI-stable napi_* call?

[Julusian]

yeah, I am really not impressed. Out of the 7 native modules I use, 6 will break from this change which to me makes it far from 'rare'.

Yeah, performance is my second concern here too. Especially not wanting to penalise nodejs users because of electron. I havent looked yet, but from a skim of the original PR I dont see anything.
I feel like for now it might need to be checking the value of the process.versions.electron global? I have asked if there is a way on the original PR electron/electron#34724

---

To clarify, I don't personally need this yet (Im very bad at keeping electron updated). But I've spent the last couple of hours looking into this and things around this, and thought you could do with a heads up to expect 'bug' reports users about this

[lovell]

I did a quick search of Node.js for previous discussion around a "memory cage" and the only result is a closed-and-not-merged PR that has some interesting observations about the performance impact.

nodejs/node#43537

Electron patches its Node.js with what appears to be a similar change, so it's possible there'll be a performance impact on anyone using Buffers in Electron.

https://github.com/electron/electron/blob/main/patches/node/support_v8_sandboxed_pointers.patch

[Nantris]

I just read the Electron 21 blog post and headed straight here and saw my fear confirmed.

Is there a way of querying the Electron runtime to determine if this "memory cage" feature is enabled, ideally via an ABI-stable napi_* call?

I don't know of any direct way to query for this feature, but the Electron version could be queried at least?

[Nantris]

@lovell the implementation they actually went with can be found by following the link in this comment: nodejs/node#43537 (comment) - not sure if it's relevant, but in the interest of gathering the necessary data.

[Julusian]

the implementation they actually went with can be found by following the link in this comment: nodejs/node#43537 (comment) - not sure if it's relevant, but in the interest of gathering the necessary data.

I dont think that really teaches us anything. All I see there is changing some node internals to always use a different memory allocator, which is essentially what they have said we need to do in that blog post

[arnhrwd]

I see a related warning when using Electron 21:

(node:228244) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead. (Use electron --trace-deprecation ... to show where the warning was created)

As well, use of metadata() causes a FATAL crash. I have had to revert to a slower parser to parse EXIF data.

[lovell]

Please see nodejs/node#45181 for the required changes to Node.js itself and nodejs/node-addon-api#1227 for follow-up changes to the node-addon-api wrapper. Once these are made available in published releases, there will also be changes required to sharp to use the new API they provide.

[lovell]

@arnhrwd That Buffer deprecation warning is not from sharp.

[KevinEady]

@lovell ,

FYI nodejs/node#45181 has landed.

[Julusian]

@lovell In case you aren't following along, the upstream change has been released in node 19.1.0, but not any other versions of nodejs yet. And I don't think it is in any release of electron yet

[lovell]

There's a work-in-progress branch for this at v8-memory-cage, which is dependent on Electron updating its various internal versions of Node.js to include the new napi_no_external_buffers_allowed status.

[lovell]

Commit 584807b adds this and it will be part of v0.31.3.

Please remember you'll need a version of Electron that provides the patched version of Node.js

[lovell]

v0.31.3 now available with a workaround for the V8 memory cage.

Please remember you'll need a future version of Electron that provides the patched version of Node.js to take advantage of this.

https://github.com/electron/electron/search?q=09ae62b&type=issues

[Y2zz]

It worked fine after I downgraded to electron 19.1.8

[Jasonnor]

Works smoothly after upgrade to Electron 22.0.2 & Sharp v0.31.3, thanks!

[lovell]

Based on the release notes, the following versions of Electron are all patched and should work with sharp v0.31.3+

• v20.3.9
• v21.3.4
• v22.0.1
• v23.0.0-beta.1

[KillerCodeMonkey]

@lovell just a question if my problem is related to this all new memory handling with electron + node.

I have an electron app and everything runs wonderful with v23. but when using v24 i get random: GSlice: assertion failed: sinfo->n_allocated > 0 errors when doing things with sharp especially when doing things in parallel.
I have a graphql and express api which also generates thumbnails for images on the fly and so on.
Now i do not know if the error is coming from sharp or electron.

PS: i found a possible root cause in another github issue/solution with a similar issues in
libvmi/libvmi#927

my system is ubuntu 22.04, node 18, sharp 0.33, electron 24 (breaks), electron 23 (works)

Thanks you very much - your support and project is amazing :)

[lovell]

@KillerCodeMonkey I'm unsure which version "sharp 0.33" refers to, but the latest sharp v0.32.0 does not rely on the GSlice allocator at all, as it has been removed from the glib dependency, so I would guess this error is more likely to be from Electron. The G_SLICE=always-malloc environment variable can be used to switch it off, but my advice (to Electron) would be to upgrade to a more recent version of glib without it.

[KillerCodeMonkey]

thanks for the quick replay and you are right.. sorry for the typo, i meant sharp v0.32.0