Skip to content

fix(napi): add test coverage and fix escape_handle/throw bugs#32960

Merged
bartlomieju merged 3 commits intomainfrom
test/napi-coverage
Mar 24, 2026
Merged

fix(napi): add test coverage and fix escape_handle/throw bugs#32960
bartlomieju merged 3 commits intomainfrom
test/napi-coverage

Conversation

@bartlomieju
Copy link
Copy Markdown
Member

@bartlomieju bartlomieju commented Mar 24, 2026

Summary

  • Add NAPI test coverage for handle scopes, references, and exception handling
  • Fix two bugs discovered by the new tests

New tests

Handle scopes (handle_scope.rs + handle_scope_test.js):

  • napi_open/close_handle_scope
  • napi_open/close_escapable_handle_scope + napi_escape_handle
  • Nested handle scopes
  • Double escape detection

References (reference.rs + reference_test.js):

  • napi_create_reference with strong refcount
  • napi_get_reference_value / napi_delete_reference
  • napi_reference_ref / napi_reference_unref counting
  • napi_create_external / napi_get_value_external roundtrip
  • External values accessed through references

Exceptions (exception.rs + exception_test.js):

  • napi_is_exception_pending
  • napi_get_and_clear_last_exception
  • Exception propagation through napi_call_function

Bug fixes

napi_escape_handle double-call panic: The escapable handle scope was
a complete no-op with no state tracking. Calling napi_escape_handle
twice would silently succeed instead of returning
napi_escape_called_twice. Fixed by tracking escape state via a
heap-allocated bool created in napi_open_escapable_handle_scope.

napi_throw + napi_get_and_clear_last_exception cycle broken:
napi_throw was both storing the exception in env.last_exception AND
calling scope.throw_exception(). This made it impossible to clear the
exception with napi_get_and_clear_last_exception since V8 still had a
pending exception. Fixed by only storing in env.last_exception; the
callback wrapper (call_fn) already handles the V8 throw after the
callback returns.

Test plan

  • ./x test-napi passes: 72 passed, 0 failed

🤖 Generated with Claude Code

bartlomieju and others added 2 commits March 24, 2026 19:19
… exceptions

Add NAPI test coverage for three fundamental areas that were previously
untested:

Handle scopes (handle_scope.rs + handle_scope_test.js):
- napi_open/close_handle_scope
- napi_open/close_escapable_handle_scope + napi_escape_handle
- Nested handle scopes
- Double escape (ignored: Deno panics instead of returning error)

References (reference.rs + reference_test.js):
- napi_create_reference with strong refcount
- napi_get_reference_value
- napi_delete_reference
- napi_reference_ref / napi_reference_unref counting
- napi_create_external / napi_get_value_external roundtrip
- External values accessed through references

Exceptions (exception.rs + exception_test.js):
- napi_is_exception_pending
- napi_get_and_clear_last_exception
- Exception propagation through napi_call_function
- Throw + clear cycle (ignored: napi_throw with raw string value)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Two fixes for NAPI bugs found by the new test coverage:

1. napi_escape_handle: Track whether escape has been called via a
   heap-allocated bool in napi_open_escapable_handle_scope. Return
   napi_escape_called_twice on second call instead of silently
   succeeding. Previously the escapable handle scope was entirely a
   no-op with no state tracking.

2. napi_throw: Only store the exception in env.last_exception, do not
   also call scope.throw_exception(). The NAPI callback wrapper
   (call_fn) already checks last_exception after each callback and
   throws via V8 then. Throwing in both places made it impossible to
   clear the exception with napi_get_and_clear_last_exception since
   V8 would still have a pending exception after clearing last_exception.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@bartlomieju bartlomieju changed the title test(napi): add coverage tests for handle scopes, references, exceptions fix(napi): add test coverage and fix escape_handle/throw bugs Mar 24, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@bartlomieju bartlomieju merged commit dd5c9cb into main Mar 24, 2026
112 checks passed
@bartlomieju bartlomieju deleted the test/napi-coverage branch March 24, 2026 21:20
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 this pull request may close these issues.

1 participant