Skip to content

Commit

Permalink
Avoid leaking napi_ref leak when calling Wrap
Browse files Browse the repository at this point in the history
  • Loading branch information
vmoroz committed Dec 21, 2023
1 parent be6cc1e commit 4ddf72d
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 5 deletions.
4 changes: 1 addition & 3 deletions src/NodeApi/JSValue.cs
Original file line number Diff line number Diff line change
Expand Up @@ -702,9 +702,7 @@ public unsafe JSValue Wrap(object value)
handle,
(nint)valueHandle,
new napi_finalize(s_finalizeGCHandle),
_scope!.RuntimeContextHandle,
// TODO: (vmoroz) add override without out parameter.
out _).ThrowIfFailed();
_scope!.RuntimeContextHandle).ThrowIfFailed();
return this;
}

Expand Down
8 changes: 8 additions & 0 deletions src/NodeApi/Runtime/JSRuntime.cs
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,14 @@ public virtual napi_status Wrap(
napi_finalize finalize_cb,
nint finalize_hint,
out napi_ref result) => throw NS();

public virtual napi_status Wrap(
napi_env env,
napi_value js_object,
nint native_object,
napi_finalize finalize_cb,
nint finalize_hint) => throw NS();

public virtual napi_status Unwrap(napi_env env, napi_value js_object, out nint result) => throw NS();
public virtual napi_status RemoveWrap(napi_env env, napi_value js_object, out nint result) => throw NS();
public virtual napi_status SetObjectTypeTag(
Expand Down
15 changes: 13 additions & 2 deletions src/NodeApi/Runtime/NodejsRuntime.JS.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@

namespace Microsoft.JavaScript.NodeApi.Runtime;

using static JSNativeApi;

// Imports Node.js native APIs defined in js_native_api.h
public unsafe partial class NodejsRuntime
{
Expand Down Expand Up @@ -1649,6 +1647,7 @@ public override napi_status Wrap(
nint finalize_hint,
out napi_ref result)
{
// The wrapper reference must be deleted by user code.
result = default;
fixed (napi_ref* result_ptr = &result)
{
Expand All @@ -1657,6 +1656,18 @@ public override napi_status Wrap(
}
}

public override napi_status Wrap(
napi_env env,
napi_value js_object,
nint native_object,
napi_finalize finalize_cb,
nint finalize_hint)
{
// The wrapper reference is deleted by Node.js.
return Import(ref napi_wrap)(
env, js_object, native_object, finalize_cb, finalize_hint, default);
}

private delegate* unmanaged[Cdecl]<napi_env, napi_value, nint, napi_status> napi_unwrap;

public override napi_status Unwrap(napi_env env, napi_value js_object, out nint result)
Expand Down
13 changes: 13 additions & 0 deletions src/NodeApi/Runtime/TracingJSRuntime.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2104,6 +2104,19 @@ public override napi_status Wrap(
return status;
}

public override napi_status Wrap(
napi_env env,
napi_value js_object,
nint native_object,
napi_finalize finalize_cb,
nint finalize_hint)
{
napi_status status = TraceCall(
[Format(env, js_object), Format(native_object)],
() => _runtime.Wrap(env, js_object, native_object, finalize_cb, finalize_hint));
return status;
}

public override napi_status Unwrap(napi_env env, napi_value js_object, out nint result)
{
nint resultValue = default;
Expand Down

0 comments on commit 4ddf72d

Please sign in to comment.