This was accidentally added in 2d1e4144b3.
The second cfg attribute is essentially a no-op since the first one is
still valid and has more conditions, but we don't need it.
* fix: prevent crashing when napi_register_module_v1 is called twice
Currently napi-rs addons can lead to the Node.js process aborting with
the following error when initialising the addon on Windows:
```
c:\ws\src\cleanup_queue-inl.h:32: Assertion `(insertion_info.second)
== (true)' failed.
```
This happens because `napi_add_env_cleanup_hook` must not be called
with the same arguments multiple times unless the previously scheduled
cleanup hook with the same arguments was already executed. However,
the cleanup hook added by `napi_register_module_v1` in napi-rs on
Windows was always created with `ptr::null_mut()` as an argument.
One case where this causes a problem is when using the addon from
multiple contexts (e.g. Node.js worker threads) at the same
time. However, Node.js doesn't provide any guarantees that the N-API
addon initialisation code will run only once even per thread and
context. In fact, it's totally valid to run `process.dlopen()`
multiple times from JavaScript land in Node.js, and this will lead to
the initialisation code being run multiple times as different
`exports` objects may need to be populated. This may happen in
numerous cases, e.g.:
- When it's not possible or not desirable to use `require()` and users
must resort to using `process.dlopen()` (one use case is passing
non-default flags to `dlopen(3)`, another is ES modules). Caching
the results of `process.dlopen()` to avoid running it more than once
may not always be possible reliably in all cases (for example,
because of Jest sandbox).
- When the `require` cache is cleared.
- On Windows: `require("./addon.node")` and then
`require(path.toNamespacedPath("./addon.node"))`.
Another issue is fixed inside `napi::tokio_runtime::drop_runtime`:
there's no need to call `napi_remove_env_cleanup_hook` (it's only
useful to cancel the hooks that haven't been executed yet). Null
pointer retrieved from `arg` was being passed as the `env` argument of
that function, so it didn't do anything and just returned
`napi_invalid_arg`.
This patch makes `napi_register_module_v1` use a counter as the
cleanup hook argument, so that the value is always different. An
alternative might have been to use a higher-level abstraction around
`sys::napi_env_cleanup_hook` that would take ownership of a boxed
closure, if there is something like this in the API already. Another
alternative could have been to heap-allocate a value so that we would
have a unique valid memory address.
The patch also contains a minor code cleanup related to
`RT_REFERENCE_COUNT` along the way: the counter is encapsulated inside
its module and `ensure_runtime` takes care of incrementing it, and
less strict memory ordering is now used as there's no need for
`SeqCst` here. If desired, it can be further optimised to
`Ordering::Release` and a separate acquire fence inside the if
statement in `drop_runtime`, as `AcqRel` for every decrement is also a
bit stricter than necessary (although simpler). These changes are not
necessary to fix the issue and can be extracted to a separate patch.
At first it was tempting to use the loaded value of
`RT_REFERENCE_COUNT` as the argument for the cleanup hook but it would
have been wrong: a simple counterexample is the following sequence:
1. init in the first context (queue: 0)
2. init in the second context (queue: 0, 1)
3. destroy the first context (queue: 1)
4. init in the third context (queue: 1, 1)
* test(napi): unload test was excluded unexpected
---------
Co-authored-by: LongYinan <lynweklm@gmail.com>
And recreate it if it does not exist.
In windows, electron renderer process will crash if:
1. Import some NAPI module that enable `tokio-rt` flag in renderer process.
2. Reload the page.
3. Call a function imported from that NAPI module.
Because the tokio runtime will be dropped when reloading the page, and won't create again, but currently we assume that the runtime must exist in tokio-based `within_runtime_if_available`.
This will cause some panic like this:
```
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', napi-rs\crates\napi\src\tokio_runtime.rs:72:42
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Error: Renderer process crashed: crashed, exitCode: -529697949
at EventEmitter.<anonymous> (napi-rs\examples\napi\electron.js:33:9)
at EventEmitter.emit (node:events:525:35)
```
* fix(napi): big numbers losing precision on serde_json::Value
* fix(napi): add feature flags for bigint on number conversion
* chore(napi): change MAX_SAFE_INT to constant and convert big numbers to string when bigint is not available
* chore(napi): change how the check for safe integer range is made
Notice from the n-api docs that the data returned from
`napi_get_typedarray_info` is already adjusted by the byte offset.
https://nodejs.org/api/n-api.html#napi_get_typedarray_info
This means that when `as_ref`/`as_mut` apply the byte offset, the
offset is in practice applied twice.
This wasn't caught in tests because no test tried to modify a typed
array with a byte offset, and the test didn't us the typed array
structs, only `JsTypedArray`. If you want, I can modify the rest of the
functions in examples/napi-compt-mode/src/arraybuffers.rs
and the matching tests, to test all typed arrays.
IMO the `byte_offset` field can be removed entirely from the struct,
but I wanted to submit a minimal PR.
* fix leaked napi refcount in `Buffer` when cloning
Cloning unconditionally increased the refcount in `Buffer::clone`, but only called `napi_reference_unref` on dropping the last Buffer (the one with `strong_count == 1`). This means that the refcount will never drop back to zero after cloning, leaking the Buffer.
This commit changes it to also unconditionally unref the buffer.
* fix multiple sources of UB in `Buffer`
- `slice::from_raw_parts` may never be created with a null pointer, but `napi_get_buffer_info` was not sufficiently checked → UB when passing an empty Buffer
- `&'static mut [u8],` is invalid, as it certainly doesn't live for `'static`
Switching to `NonNull<u8>` and a `len` field fixes both of these.
- I also don't really understand how the `impl ToNapiValue for &mut Buffer` could have been sound. It creates an entirely new `Arc`, but reuses the same `Vec` allocation, leading to... a double free of the `Vec` on drop? I have replaced it with a simple call to `clone` instead.
* remove overcomplicated bool and drop impl
As far as I can tell, by just removing the bool and letting the drop code do its thing we clean up correctly in all cases. Because `napi_create_external_buffer` gets an owned `Buffer` attached to it via the Box, we can rely on `from_raw` retrieving it in the `drop_buffer` function.
