Fix v8::External::Value/New for the new ExternalPointerTypeTag API#1015
Open
gdavidkov wants to merge 1 commit intonodejs:mainfrom
Open
Fix v8::External::Value/New for the new ExternalPointerTypeTag API#1015gdavidkov wants to merge 1 commit intonodejs:mainfrom
gdavidkov wants to merge 1 commit intonodejs:mainfrom
Conversation
V8 13 added an ExternalPointerTypeTag parameter (uint16_t pointer-tag for the
sandbox) to both v8::External::New and v8::External::Value:
static Local<External> New(Isolate*, void*, ExternalPointerTypeTag);
void* Value(ExternalPointerTypeTag) const;
The old zero-arg Value() and two-arg New(Isolate*, void*) overloads are gone,
so any nan-using addon fails to compile against Electron 42 / Node 24's V8:
nan_callbacks_12_inl.h(189): error C2664:
'void *v8::External::Value(v8::ExternalPointerTypeTag) const':
cannot convert argument 1 from 'v8::Isolate *' to 'v8::ExternalPointerTypeTag'
nan_implementation_12_inl.h(79): error C2660:
'v8::External::New': function does not take 2 arguments
Add two private inline helpers in the existing imp:: namespace:
imp::GetExternalPointer(v8::Local<v8::External>)
imp::NewExternal(v8::Isolate*, void*)
Each selects the V8 13+ signature when V8_MAJOR_VERSION >= 13 (passing
v8::kExternalPointerTypeTagDefault, the documented no-tagging default for
embedders that do not partition externals by type) and falls back to the
legacy signature otherwise. All 28 Value() trampoline callsites in
nan_callbacks_12_inl.h and all 3 New() factory callsites in
nan_implementation_12_inl.h are routed through the helpers. Both helpers
share kExternalPointerTypeTagDefault so the tag matches between create and
read. No new macros are introduced.
Tests
-----
The existing news.cpp::NewExternal and nannew.cpp::testExternal cases call
v8::External::Value() directly (consumer-side raw-V8 calls, not through nan),
so they failed to compile against V8 13 in the same way as nan's headers.
Added the same V8 13 guard inline at those two sites so the tests build and
run on V8 13 while remaining identical on older V8. Together these two
existing tests already cover the round-trip exercised by this fix:
- news.cpp::NewExternal exercises the factory path:
Nan::New<v8::External>(...) -> Factory<v8::External>::New(value)
-> imp::NewExternal(isolate, value)
-> v8::External::New(isolate, value, tag)
- The trampoline path (imp::GetExternalPointer) is exercised every time
any NAN_METHOD test is invoked from JS, including news.cpp::NewExternal
itself, since FunctionCallbackWrapper unpacks the registered callback
pointer through that helper before dispatching.
Scope and what is not changed
-----------------------------
This patch addresses the v8::External break specifically. Other unrelated
V8 13 API changes in nan, if any, are out of scope (mirroring the prior
nan_weak.h fix which was a separate, surgical commit).
nan_callbacks_pre_12_inl.h and nan_implementation_pre_12_inl.h are
intentionally untouched: they target NODE_MODULE_VERSION <=
NODE_0_12_MODULE_VERSION (Node <= 0.12, pre-V8-3.x), where neither overload
exists.
Backwards compatibility
-----------------------
Every change is gated on `defined(V8_MAJOR_VERSION) && V8_MAJOR_VERSION >= 13`.
On V8 <= 12 the else branch reproduces the original code byte-for-byte, so
the patch is a no-op for every V8 version nan currently supports.
7cfa358 to
59d874d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
V8 added an
ExternalPointerTypeTagparameter tov8::External::Newandv8::External::Valueand removed the prior overloads:The new signatures are gated by V8's external-pointer-tagging build configuration, signaled by the public macro
V8_EXTERNAL_POINTER_TAG_COUNT. AV8_MAJOR_VERSIONcheck is unreliable here because Node and Chromium ship divergent V8 builds — the feature can be enabled in Electron's V8 and not in upstream Node's V8 at the same nominal major version.nanstill calls the removed forms — 28 sites innan_callbacks_12_inl.h(theimp::*CallbackWrappertrampolines) and 3 innan_implementation_12_inl.h(theExternal/Function/FunctionTemplatefactories). Both are header-inline, so any addon that includes<nan.h>fails to build against a tag-enabled V8. Reproduced against Electron 42.0.0-beta.8 (Node 24.15.0) on Windows + MSVC 2022:Fix
Two private inline helpers in the existing
imp::namespace, one per file next to its callers:Each selects the tag-taking signature when
V8_EXTERNAL_POINTER_TAG_COUNTis defined, passingv8::kExternalPointerTypeTagDefault(V8's no-tagging default for embedders that do not partition externals by type), and falls back to the legacy form otherwise. All 31 callsites are routed through the helpers. Both use the same tag, so create/read tags match by construction.Backwards compatibility
Every change is gated on
#ifdef V8_EXTERNAL_POINTER_TAG_COUNT. The#elsebranches reproduce the prior code byte-for-byte — no-op on every V8 build that doesn't define the macro.Tests
test/cpp/news.cpp::NewExternalandtest/cpp/nannew.cpp::testExternalround-trip avoid*throughNan::New<v8::External>and read it back via rawv8::External::Value(). They hit the same compile break asnan's headers; the same gate is applied at each test site.NewExternalexercisesimp::NewExternaldirectly;imp::GetExternalPointeris exercised every time anyNAN_METHODtest runs from JS, sinceimp::FunctionCallbackWrapperunpacks the registered callback through it before dispatch.Out of scope
V8 14 work already landed (#1010, #1013); this PR addresses only the
ExternalAPI change.nan_callbacks_pre_12_inl.h/nan_implementation_pre_12_inl.htarget Node ≤ 0.12 (pre-V8-3.x), which uses a still-older single-arg form, so a tag-enabled V8 never selects those files.Verified
Builds clean against Electron 42.0.0-beta.8 (Node 24.15.0) on Windows + MSVC 2022. CI will exercise both branches across the existing matrix.