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

Fix NRE in native host JSReference #96

Merged
merged 1 commit into from
Apr 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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();
jasongin marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
_context?.SynchronizationContext.Post(
() => napi_delete_reference((napi_env)_context, handle).ThrowIfFailed(),
allowSync: true);
}
}
}

Expand Down