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

Cannot allocate Buffer into Chromium v8. #1346

Closed
hallahan opened this issue Oct 16, 2022 · 11 comments · Fixed by #1445
Closed

Cannot allocate Buffer into Chromium v8. #1346

hallahan opened this issue Oct 16, 2022 · 11 comments · Fixed by #1445

Comments

@hallahan
Copy link

I have a function like so:

  pub async fn tile(&self, z: u8, x: u32, y: u32) -> Result<Buffer> {
    tokio::task::spawn(async move { 
      let t = Tile::from_zxy(z, x, y);
      let buf = tile_info(t).into();
      Ok(buf)
    }).await.unwrap()
  }

However, whenever I call it from an Electron app, with sandboxing turned off, I still get the following error:

[31872:1015/170858.344491: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.

I want to essentially have a Vec<u8 / Uint8Array of data created in a tokio task, and I want to access that data in NodeJS. Am I doing this wrong, or is there a bug here?

Is there a way to tell v8 to allocate a buffer, and do it from within a tokio task?

@hallahan
Copy link
Author

Under the hood, any buffer sent over to Node should be transferred to v8, which should manage a reference and garbage collection.

What is the recommended pattern to follow to create buffers in Rust and pass a reference over to V8 without any copy?

I think this is a very common use case. I'm happy to PR to napi.rs docs an example once I figure this out.

PS - Rust noob here.

@Brooooooklyn
Copy link
Sponsor Member

See nodejs/abi-stable-node#441, electron 21 doesn't allow to use zero-copy API anymore.

@hallahan
Copy link
Author

Thank you for the clarification. Electron's security paranoia is going to drive away developers that care about performance.

I guess I will have to stick with v20.

@hallahan
Copy link
Author

hallahan commented Oct 17, 2022

Is it possible to tell v8 to allocate memory through napi-rs?

This would be cumbersome, but possible?

The problem is, it's unlikely one would know the size of the buffer to allocate before actually creating the data... However, maybe exposing an interface that adheres to Vec<u8> could be possible? Not sure, as I'm a Rust noob.

@hallahan
Copy link
Author

Would it be helpful if I added this info to the napi-rs docs? Allocating a Buffer or TypedArray is an important use case, as this is the most performant way to expose data to v8. Electron apps are very common these days, so it is likely someone else will run into this.

Note, creating Buffers and Uint8Arrays is working fine when I downgrade electron to 20.3.2.

I don't understand why the Electron project emphasizes sandboxing, with no consideration that it may be prohibitive to force the developer to copy large amounts of data, as well as require transferring data across processes unnecessarily.

For example, if someone were to build a desktop application using Qt or Cocoa, there would be no requirement or suggestion to sandbox a back-end. The fact that Electron provides JavaScript execution doesn't change anything from a security standpoint?

@hallahan hallahan reopened this Oct 17, 2022
@nashaofu
Copy link

same issue:https://github.com/nashaofu/napi-rs-issue

@hallahan
Copy link
Author

How does napi-rs allocate other things like standard JavaScript arrays?

@nashaofu
Copy link

nashaofu commented Dec 4, 2022

This is my solution: https://github.com/nashaofu/node-screenshots/blob/master/src/lib.rs#L106

  #[napi]
  pub fn capture_sync(&self, env: Env) -> Option<JsBuffer> {
    let image = self.screen.capture()?;
    let buffer = env.create_buffer_copy(image.buffer()).ok()?;
    Some(buffer.into_raw())
  }

@hallahan
Copy link
Author

hallahan commented Dec 4, 2022

Note that you're doing an extra allocation and copy.

I pinned Electron to 20.3.2 which is before they became overly paranoid.

I'm just going to stick with that version and eventually move away from Electron.

@yisibl
Copy link
Collaborator

yisibl commented Jan 9, 2023

About V8 memory cage

@Brooooooklyn Sharp v0.31.3 recently added a solution for memory cage. will napi-rs consider adding a similar solution?

@Brooooooklyn
Copy link
Sponsor Member

See Brooooooklyn/snappy#97

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 a pull request may close this issue.

4 participants