v8: remove deprecated method

[gengjiawen]

Fix nodejs/node#30915

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Failing tests

[mmarchini]

Tests on nodejs/node#32116 are failing with those changes, and I don't think it helped supress any build warnings (but then again, we're getting so many warnings there it's hard to tell). Should this work on 8.1?

[mmarchini]

CI: https://ci.nodejs.org/job/node-test-pull-request/29596/

[gengjiawen]

Tests on nodejs/node#32116 are failing with those changes

The failed may not related to this change. Github action still show previous already failed case.

I don't think it helped supress any build warnings

It do fix 5 warnings, you can see warning https://github.com/nodejs/node-v8/runs/481711528 (search crbug), which I notice the bug in first place. Also @addaleax put todos in related code.

Details

../deps/v8/include/v8.h:5169:8: note: declared here
   bool IsExternal() const;
        ^~~~~~~~~~
../src/js_native_api_v8.cc:2725:50: warning: ‘void v8::ArrayBuffer::Externalize(const std::shared_ptr<v8::BackingStore>&)’ is deprecated: This will be removed together with IsExternal. [-Wdeprecated-declarations]
     buffer->Externalize(buffer->GetBackingStore());
                                                  ^
In file included from ../src/util.h:31:0,
                 from ../src/aliased_buffer.h:7,
                 from ../src/env-inl.h:27,
                 from ../src/js_native_api_v8.cc:5:
../deps/v8/include/v8.h:5206:8: note: declared here
   void Externalize(const std::shared_ptr<BackingStore>& backing_store);
        ^~~~~~~~~~~
In file included from ../src/js_native_api_v8.cc:6:0:
../src/js_native_api_v8.cc: In function ‘napi_status napi_detach_arraybuffer(napi_env, napi_value)’:
../src/js_native_api_v8.cc:3189:27: warning: ‘bool v8::ArrayBuffer::IsExternal() const’ is deprecated: With v8::BackingStore externalized ArrayBuffers are the same as ordinary ArrayBuffers. See http://crbug.com/v8/9908. [-Wdeprecated-declarations]
       env, it->IsExternal(), napi_detachable_arraybuffer_expected);
                           ^
../src/js_native_api_v8.h:141:11: note: in definition of macro ‘RETURN_STATUS_IF_FALSE’

Should this work on 8.1?

Yeap. But still @addaleax review this will more helpful (I have poor knowledge in V8).

[mmarchini]

The failed may not related to this change. Github action still show previous already failed case.

I reverted and the failure stopped. If I cherry-pick this commit I can reproduce the failure predictably on my machine. Bear in mind we've been on 8.2 on this repository for a while now.

[gengjiawen]

I reverted and the failure stopped. If I cherry-pick this commit I can reproduce the failure predictably on my machine. Bear in mind we've been on 8.2 on this repository for a while now.

I see. Let's not include this to v8 8.1 then.

[addaleax]

I’ll take a look 👍

[addaleax]

@mmarchini @gengjiawen Can you point me to the failing tests? When running the tests on this PR locally, 3 memory tests + test-inspector-multisession-ws fail, but the same tests also fail on the canary branch itself.

[addaleax]

Ah, judging from the conversation above, this only fails when applied to V8 8.1? In that case, yeah, simply leaving it out of the V8 8.1 PR makes sense to me.

[mmarchini]

Just tried to cherry-pick it on canary-base and I'm seeing this failure:

$ out/Release/node /home/mmarchini/workspace/nodejs/node-canary-base-cherrypicks/test/js-native-api/test_typedarray/test.js
assert.js:102
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at /home/mmarchini/workspace/nodejs/node-canary-base-cherrypicks/test/js-native-api/test_typedarray/test.js:81:10
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/home/mmarchini/workspace/nodejs/node-canary-base-cherrypicks/test/js-native-api/test_typedarray/test.js:79:12)
    at Module._compile (internal/modules/cjs/loader.js:1202:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1222:10)
    at Module.load (internal/modules/cjs/loader.js:1051:32)
    at Function.Module._load (internal/modules/cjs/loader.js:947:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: /A detachable arraybuffer was expected/,
  operator: 'throws'
}

[gengjiawen]

Just tried to cherry-pick it on canary-base and I'm seeing this failure:

Maybe some upstream change caused this. I will try to do a little digging.

[gengjiawen]

Also previous v8 version on node-v8 is 8.2.237, now it's 8.2.0, could this be the reason. @mmarchini

[gengjiawen]

Github action also shows the only test failure is Path: parallel/test-zlib-unused-weak.

[mmarchini]

Started another CI: https://ci.nodejs.org/job/node-test-pull-request/29638/, but FWIW, the previous CI was also failing on test_typedarray.

[mmarchini]

So the test is failing because test_typedarray/test.js#L79-L84 expects Uint8Array and family to be non-detachable (because they don't have externalized memory), but since we removed

  // TODO(addaleax): Remove the first condition once we have V8 8.0.
  RETURN_STATUS_IF_FALSE(
      env, it->IsExternal(), napi_detachable_arraybuffer_expected); 

a call to Detach() is not throwing the expected exception. I'm not sure if the test is outdated, or if we need to perform an extra check in napi_detach_arraybuffer. I assume if the array was not detachable, IsDetachable would return false, so the test is probably outdated.

diff --git a/test/js-native-api/test_typedarray/test.js b/test/js-native-api/test_typedarray/test.js
index 0d9594d929..f3efcc6e23 100644
--- a/test/js-native-api/test_typedarray/test.js
+++ b/test/js-native-api/test_typedarray/test.js
@@ -78,9 +78,7 @@ nonByteArrayTypes.forEach((currentType) => {
 // Test detaching
 arrayTypes.forEach((currentType) => {
   const buffer = Reflect.construct(currentType, [8]);
-  assert.throws(
-    () => test_typedarray.Detach(buffer),
-    /A detachable arraybuffer was expected/);
+  assert.doesNotThrow(() => test_typedarray.Detach(buffer));
 });
 {
   const buffer = test_typedarray.External();

@addaleax what do you think?

[mmarchini]

Ping @addaleax

[gengjiawen]

Close in favor of nodejs/node#32358.