diff --git a/src/jsc/bindings/napi.cpp b/src/jsc/bindings/napi.cpp index 8b52fa20f90a..a9ed59a0c484 100644 --- a/src/jsc/bindings/napi.cpp +++ b/src/jsc/bindings/napi.cpp @@ -1280,11 +1280,10 @@ extern "C" napi_status napi_is_detached_arraybuffer(napi_env env, NAPI_CHECK_ARG(env, arraybuffer); NAPI_CHECK_ARG(env, result); + // Node computes IsArrayBuffer() && WasDetached() and always returns + // napi_ok; a non-ArrayBuffer (including SharedArrayBuffer) yields false. JSC::JSArrayBuffer* jsArrayBuffer = dynamicDowncast(toJS(arraybuffer)); - NAPI_RETURN_EARLY_IF_FALSE(env, jsArrayBuffer, napi_arraybuffer_expected); - - auto* arrayBuffer = jsArrayBuffer->impl(); - *result = arrayBuffer->isDetached(); + *result = jsArrayBuffer && !jsArrayBuffer->isShared() && jsArrayBuffer->impl()->isDetached(); NAPI_RETURN_SUCCESS(env); } @@ -1297,10 +1296,16 @@ extern "C" napi_status napi_detach_arraybuffer(napi_env env, JSC::VM& vm = JSC::getVM(globalObject); JSC::JSArrayBuffer* jsArrayBuffer = dynamicDowncast(toJS(arraybuffer)); - NAPI_RETURN_EARLY_IF_FALSE(env, jsArrayBuffer, napi_arraybuffer_expected); + // V8's IsArrayBuffer() is false for SharedArrayBuffer; JSC uses the same + // cell type for both, so reject shared buffers here to match Node instead + // of returning napi_ok for a buffer that was never neutralized. + NAPI_RETURN_EARLY_IF_FALSE(env, jsArrayBuffer && !jsArrayBuffer->isShared(), napi_arraybuffer_expected); auto* arrayBuffer = jsArrayBuffer->impl(); - if (!arrayBuffer->isDetached() && arrayBuffer->isDetachable()) { + // Node then requires IsDetachable(). Detaching an already-detached buffer + // is a no-op in both engines, so treat that as success. + NAPI_RETURN_EARLY_IF_FALSE(env, arrayBuffer->isDetached() || arrayBuffer->isDetachable(), napi_detachable_arraybuffer_expected); + if (!arrayBuffer->isDetached()) { arrayBuffer->detach(vm); } NAPI_RETURN_SUCCESS(env); diff --git a/test/napi/napi-app/js_test_helpers.cpp b/test/napi/napi-app/js_test_helpers.cpp index 7bc9c25befc8..d1783e4ad8cc 100644 --- a/test/napi/napi-app/js_test_helpers.cpp +++ b/test/napi/napi-app/js_test_helpers.cpp @@ -303,22 +303,6 @@ static napi_value get_property_names(const Napi::CallbackInfo &info) { return result; } -// get_all_property_names(object, key_mode, key_filter, key_conversion) -> array -static napi_value get_all_property_names(const Napi::CallbackInfo &info) { - napi_env env = info.Env(); - uint32_t key_mode, key_filter, key_conversion; - NODE_API_CALL(env, napi_get_value_uint32(env, info[1], &key_mode)); - NODE_API_CALL(env, napi_get_value_uint32(env, info[2], &key_filter)); - NODE_API_CALL(env, napi_get_value_uint32(env, info[3], &key_conversion)); - napi_value result; - NODE_API_CALL(env, - napi_get_all_property_names( - env, info[0], (napi_key_collection_mode)key_mode, - (napi_key_filter)key_filter, - (napi_key_conversion)key_conversion, &result)); - return result; -} - // add_tag(object, lower, upper) static napi_value add_tag(const Napi::CallbackInfo &info) { Napi::Env env = info.Env(); @@ -512,7 +496,6 @@ void register_js_test_helpers(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, make_empty_array); REGISTER_FUNCTION(env, exports, make_empty_object); REGISTER_FUNCTION(env, exports, get_property_names); - REGISTER_FUNCTION(env, exports, get_all_property_names); REGISTER_FUNCTION(env, exports, add_tag); REGISTER_FUNCTION(env, exports, try_add_tag); REGISTER_FUNCTION(env, exports, check_tag); diff --git a/test/napi/napi-app/module.js b/test/napi/napi-app/module.js index 6d5c409a412e..6a84a59e9bb1 100644 --- a/test/napi/napi-app/module.js +++ b/test/napi/napi-app/module.js @@ -266,7 +266,7 @@ nativeTests.test_property_names_cache_poisoning = () => { console.log("Reflect.ownKeys after get_all_property_names(own_only):", Reflect.ownKeys(mkD()).join(",")); // The napi result itself should still include inherited keys. - const apnResult = nativeTests.get_all_property_names(mkA(), 0, 0, 0); + const apnResult = nativeTests.get_all_property_names(mkA(), 0, 0, 0).keys; console.log("napi include_prototypes result has own a,b:", apnResult.includes("a") && apnResult.includes("b")); console.log("napi include_prototypes result has inherited toString:", apnResult.includes("toString")); const gpnResult = nativeTests.get_property_names(mkB()); diff --git a/test/napi/napi-app/standalone_tests.cpp b/test/napi/napi-app/standalone_tests.cpp index 24e6a120f9a2..4cbeb244484b 100644 --- a/test/napi/napi-app/standalone_tests.cpp +++ b/test/napi/napi-app/standalone_tests.cpp @@ -757,6 +757,30 @@ static napi_value test_is_arraybuffer(const Napi::CallbackInfo &info) { return ok(env); } +static napi_value test_detach_arraybuffer(const Napi::CallbackInfo &info) { + napi_env env = info.Env(); + for (size_t i = 1; i < info.Length(); i++) { + napi_value value = info[i]; + + napi_status detach_status = napi_detach_arraybuffer(env, value); + + bool is_detached = false; + napi_status is_detached_status = + napi_is_detached_arraybuffer(env, value, &is_detached); + + size_t length = 0; + napi_status info_status = + napi_get_arraybuffer_info(env, value, nullptr, &length); + + printf("napi_detach_arraybuffer=%d napi_is_detached_arraybuffer=%d " + "is_detached=%s napi_get_arraybuffer_info=%d length=%zu\n", + static_cast(detach_status), + static_cast(is_detached_status), is_detached ? "true" : "false", + static_cast(info_status), length); + } + return ok(env); +} + static napi_value test_napi_get_default_values(const Napi::CallbackInfo &info) { napi_env env = info.Env(); @@ -2707,6 +2731,7 @@ void register_standalone_tests(Napi::Env env, Napi::Object exports) { REGISTER_FUNCTION(env, exports, test_is_buffer); REGISTER_FUNCTION(env, exports, test_is_typedarray); REGISTER_FUNCTION(env, exports, test_is_arraybuffer); + REGISTER_FUNCTION(env, exports, test_detach_arraybuffer); REGISTER_FUNCTION(env, exports, test_napi_get_default_values); REGISTER_FUNCTION(env, exports, test_napi_numeric_string_keys); REGISTER_FUNCTION(env, exports, test_deferred_exceptions); diff --git a/test/napi/napi.test.ts b/test/napi/napi.test.ts index ff8b4b01e943..3d7f8223fceb 100644 --- a/test/napi/napi.test.ts +++ b/test/napi/napi.test.ts @@ -1323,6 +1323,27 @@ describe.skipIf(!canBuildNodeAddons())("cleanup hooks", () => { }); }); + describe("napi_detach_arraybuffer", () => { + it("rejects SharedArrayBuffer instead of returning napi_ok for a no-op detach", async () => { + // napi_ok on a SharedArrayBuffer is a memory-lifetime lie: the addon + // believes the backing store is neutralized while JS (and other threads) + // still read and write it. Node rejects a SharedArrayBuffer with + // napi_arraybuffer_expected (19) because V8's IsArrayBuffer() is false + // for a SharedArrayBuffer. The same ArrayBuffer is passed twice so the + // third row covers a second detach on an already-detached buffer. + const output = await checkSameOutput( + "test_detach_arraybuffer", + "(() => { const ab = new ArrayBuffer(8); return [new SharedArrayBuffer(8), ab, ab, new Uint8Array(8)]; })()", + ); + expect(output.split(/\r?\n/)).toEqual([ + "napi_detach_arraybuffer=19 napi_is_detached_arraybuffer=0 is_detached=false napi_get_arraybuffer_info=0 length=8", + "napi_detach_arraybuffer=0 napi_is_detached_arraybuffer=0 is_detached=true napi_get_arraybuffer_info=0 length=0", + "napi_detach_arraybuffer=0 napi_is_detached_arraybuffer=0 is_detached=true napi_get_arraybuffer_info=0 length=0", + "napi_detach_arraybuffer=19 napi_is_detached_arraybuffer=0 is_detached=false napi_get_arraybuffer_info=1 length=0", + ]); + }); + }); + describe("error handling", () => { it("removing non-existent env cleanup hook should not crash", async () => { // Test that removing non-existent hooks doesn't crash the process