Skip to content

Commit

Permalink
Fix NRE in native host JSReference (#96)
Browse files Browse the repository at this point in the history
  • Loading branch information
jasongin authored Apr 14, 2023
1 parent 76ff08b commit 0b80148
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 10 deletions.
7 changes: 4 additions & 3 deletions src/NodeApi/Interop/JSRuntimeContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace Microsoft.JavaScript.NodeApi.Interop;
/// loaded, and disposed when the host is unloaded. (For AOT there is no "host" compnoent, so each
/// AOT module has a context that matches the module lifetime.) The context tracks several kinds
/// of JS references used internally by this assembly, so that the references can be re-used for
/// the lifetime of the module and disposed when the context is disposed.
/// the lifetime of the host and disposed when the context is disposed.
/// </remarks>
public sealed class JSRuntimeContext : IDisposable
{
Expand Down Expand Up @@ -102,13 +102,14 @@ public sealed class JSRuntimeContext : IDisposable

public bool IsDisposed { get; private set; }

public static explicit operator napi_env(JSRuntimeContext context) => context._env;
public static explicit operator napi_env(JSRuntimeContext context) => context?._env ??
throw new ArgumentNullException(nameof(context));
public static explicit operator JSRuntimeContext(napi_env env)
=> GetInstanceData(env) as JSRuntimeContext
?? throw new InvalidCastException("Context is not found in napi_env instance data.");

/// <summary>
/// Gets the current host context.
/// Gets the current runtime context.
/// </summary>
public static JSRuntimeContext Current => JSValueScope.Current.RuntimeContext;

Expand Down
24 changes: 17 additions & 7 deletions src/NodeApi/JSReference.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ namespace Microsoft.JavaScript.NodeApi;
/// </remarks>
public class JSReference : IDisposable
{
private readonly JSRuntimeContext _context;
private readonly JSRuntimeContext? _context;
private readonly napi_ref _handle;

public bool IsWeak { get; private set; }
Expand Down Expand Up @@ -68,7 +68,7 @@ public void MakeWeak()
ThrowIfDisposed();
if (!IsWeak)
{
napi_reference_unref((napi_env)_context, _handle, default).ThrowIfFailed();
napi_reference_unref((napi_env)JSValueScope.Current, _handle, default).ThrowIfFailed();
IsWeak = true;
}
}
Expand All @@ -77,7 +77,7 @@ public void MakeStrong()
ThrowIfDisposed();
if (IsWeak)
{
napi_reference_ref((napi_env)_context, _handle, default).ThrowIfFailed();
napi_reference_ref((napi_env)JSValueScope.Current, _handle, default).ThrowIfFailed();
IsWeak = true;
}
}
Expand All @@ -86,7 +86,7 @@ public void MakeStrong()
{
ThrowIfDisposed();
napi_get_reference_value(
(napi_env)_context, _handle, out napi_value result).ThrowIfFailed();
(napi_env)JSValueScope.Current, _handle, out napi_value result).ThrowIfFailed();
return result;
}

Expand Down Expand Up @@ -118,9 +118,19 @@ protected virtual void Dispose(bool disposing)
{
IsDisposed = true;
napi_ref handle = _handle; // To capture in lambda
_context.SynchronizationContext.Post(
() => napi_delete_reference((napi_env)_context, handle).ThrowIfFailed(),
allowSync: true);

// The context may be null if the reference was creatd from a "no-context" scope such
// as the native host. In that case the reference must be disposed from the JS thread.
if (_context == null)
{
napi_delete_reference((napi_env)JSValueScope.Current, handle).ThrowIfFailed();
}
else
{
_context?.SynchronizationContext.Post(
() => napi_delete_reference((napi_env)_context, handle).ThrowIfFailed(),
allowSync: true);
}
}
}

Expand Down

0 comments on commit 0b80148

Please sign in to comment.