src: remove calls to deprecated v8 functions (IntegerValue) #22129

ryzokuken · 2018-08-04T18:39:45Z

Remove all calls to deprecated v8 functions (here:
Value::IntegerValue) inside the code (src directory only).

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

nodejs-github-bot · 2018-08-04T18:39:46Z

@ryzokuken build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/428/pipeline

addaleax · 2018-08-04T23:58:16Z

src/node.cc

@@ -3123,11 +3123,9 @@ void DebugProcess(const FunctionCallbackInfo<Value>& args) {
    return env->ThrowError("Invalid number of arguments.");
  }

-  pid_t pid;
-  int r;
+  pid_t pid = args[0].As<Integer>()->Value();


Can you add CHECK(args[0]->IsInteger());?

There is no v8::Value::IsInteger method.

So, I looked at V8's implementation of the Integer class and here is what I found:

Integer::Value() can be called on any Number. The value is cast to int64_t.

.As<Integer>() can also be called on any Number. The CheckCast does obj->IsNumber().

addaleax · 2018-08-04T23:58:50Z

src/node_buffer.cc

@@ -172,7 +172,7 @@ inline MUST_USE_RESULT bool ParseArrayIndex(Local<Value> arg,
    return true;
  }

-  int64_t tmp_i = arg->IntegerValue();
+  int64_t tmp_i = arg.As<Integer>()->Value();


(ditto, and everywhere else where it’s not obvious that the value already is an integer)

src/node_buffer.cc

addaleax · 2018-08-05T00:01:51Z

src/process_wrap.cc

+        int fd = static_cast<int>(stdio->Get(context, fd_key)
+                                      .ToLocalChecked()
+                                      .As<Integer>()
+                                      ->Value());


This should also be checked first before converting, I’d say.

targos · 2018-08-26T19:34:49Z

lib/buffer.js

@@ -779,6 +779,7 @@ function bidirectionalIndexOf(buffer, val, byteOffset, encoding, dir) {

 function slowIndexOf(buffer, val, byteOffset, encoding, dir) {
  var loweredCase = false;
+  byteOffset = +byteOffset;


this is already done inside of bidirectionalIndexOf

src/node_buffer.cc

targos · 2018-09-01T19:18:01Z

@addaleax I think this one is also ready.

Copying my inline response from yesterday:

So, I looked at V8's implementation of the Integer class and here is what I found:

There is no Value::IsInteger() method.
Integer::Value() can be called on any Number. The value is cast to int64_t.
.As<Integer>() can also be called on any Number. The CheckCast does obj->IsNumber().

targos · 2018-09-01T19:18:53Z

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

addaleax

LGTM but casting Number to Integer seems like relying on V8 implementation details, even if it currently works.

I would feel more comfortable if we could add this pattern to the V8 API cctest suite first?

addaleax · 2018-09-01T19:44:38Z

src/node.cc

@@ -2565,7 +2563,8 @@ static void DebugProcess(const FunctionCallbackInfo<Value>& args) {
    goto out;
  }

-  pid = (DWORD) args[0]->IntegerValue();
+  CHECK(args[0]->IsNumber());
+  DWORD pid = (DWORD) args[0].As<Integer>()->Value();


If you happen to be able to test it on Windows without much extra work – I think the cast here is unnecessary.

It compiles fine, thanks

targos · 2018-09-02T08:33:35Z

I created a CL to add cctests: https://chromium-review.googlesource.com/c/v8/v8/+/1200983

addaleax · 2018-09-02T08:39:45Z

@targos Thank you :)

jasnell · 2018-09-04T14:53:29Z

Needs a rebase now unfortunately.

Remove all calls to deprecated v8 functions (here: Value::IntegerValue) inside the code (src directory only). Co-authored-by: Michaël Zasso <targos@protonmail.com>

targos · 2018-09-04T14:57:43Z

Rebased

targos · 2018-09-05T06:46:29Z

The V8 CL landed. This is ready.

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

Edit: Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20550/

targos · 2018-09-05T11:47:52Z

Landed in f464ac3

Remove all calls to deprecated v8 functions (here: Value::IntegerValue) inside the code (src directory only). Co-authored-by: Michaël Zasso <targos@protonmail.com> PR-URL: #22129 Reviewed-By: Anna Henningsen <anna@addaleax.net>

2555cb4 introduced a crash when a non-number value was passed to `ParseArrayIndex()`. We do not always have JS typechecking for that in place, though. This returns back to the previous behavior of coercing values to integers, which is certainly questionable. Refs: nodejs#22129 Fixes: nodejs#23668

2555cb4 introduced a crash when a non-number value was passed to `ParseArrayIndex()`. We do not always have JS typechecking for that in place, though. This returns back to the previous behavior of coercing values to integers, which is certainly questionable. Refs: #22129 Fixes: #23668

2555cb4 introduced a crash when a non-number value was passed to `ParseArrayIndex()`. We do not always have JS typechecking for that in place, though. This returns back to the previous behavior of coercing values to integers, which is certainly questionable. Refs: nodejs#22129 Fixes: nodejs#23668

2555cb4 introduced a crash when a non-number value was passed to `ParseArrayIndex()`. We do not always have JS typechecking for that in place, though. This returns back to the previous behavior of coercing values to integers, which is certainly questionable. Refs: #22129 Fixes: #23668 PR-URL: #25154 Reviewed-By: James M Snell <jasnell@gmail.com>

2555cb4 introduced a crash when a non-number value was passed to `ParseArrayIndex()`. We do not always have JS typechecking for that in place, though. This returns back to the previous behavior of coercing values to integers, which is certainly questionable. Refs: nodejs#22129 Fixes: nodejs#23668 PR-URL: nodejs#25154 Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 4, 2018

addaleax reviewed Aug 5, 2018

View reviewed changes

targos reviewed Aug 26, 2018

View reviewed changes

targos force-pushed the v8-deprecation-6 branch 2 times, most recently from 2d67e9f to 778ffc7 Compare August 29, 2018 14:28

addaleax approved these changes Sep 1, 2018

View reviewed changes

src: remove calls to deprecated v8 functions (IntegerValue)

76e79b1

Remove all calls to deprecated v8 functions (here: Value::IntegerValue) inside the code (src directory only). Co-authored-by: Michaël Zasso <targos@protonmail.com>

targos force-pushed the v8-deprecation-6 branch from e59d646 to 76e79b1 Compare September 4, 2018 14:57

targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2018

targos closed this Sep 5, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

addaleax mentioned this pull request Oct 21, 2018

[v10.x] buffer: fix crash for invalid index types #23795

Closed

3 tasks

addaleax added the dont-land-on-v8.x label Oct 21, 2018

refack mentioned this pull request Oct 25, 2018

[v10.x] Revert buffer crash #23875

Closed

3 tasks

refack mentioned this pull request Nov 25, 2018

Regression v10.x node_file.cc:1713: Assertion `(off) <= (buffer_length)' failed. #24640

Closed

addaleax mentioned this pull request Dec 20, 2018

buffer: fix crash for invalid index types #25154

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: remove calls to deprecated v8 functions (IntegerValue) #22129

src: remove calls to deprecated v8 functions (IntegerValue) #22129

ryzokuken commented Aug 4, 2018

nodejs-github-bot commented Aug 4, 2018

addaleax Aug 4, 2018

targos Aug 29, 2018

targos Aug 31, 2018

addaleax Aug 4, 2018

addaleax Aug 5, 2018

targos Aug 26, 2018

targos commented Sep 1, 2018

targos commented Sep 1, 2018

addaleax left a comment

addaleax Sep 1, 2018

targos Sep 2, 2018

targos commented Sep 2, 2018

addaleax commented Sep 2, 2018

jasnell commented Sep 4, 2018

targos commented Sep 4, 2018

targos commented Sep 5, 2018 •

edited

Loading

targos commented Sep 5, 2018

src: remove calls to deprecated v8 functions (IntegerValue) #22129

src: remove calls to deprecated v8 functions (IntegerValue) #22129

Conversation

ryzokuken commented Aug 4, 2018

Checklist

nodejs-github-bot commented Aug 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Sep 1, 2018

targos commented Sep 1, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Sep 2, 2018

addaleax commented Sep 2, 2018

jasnell commented Sep 4, 2018

targos commented Sep 4, 2018

targos commented Sep 5, 2018 • edited Loading

targos commented Sep 5, 2018

targos commented Sep 5, 2018 •

edited

Loading