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

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

(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

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

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

lib/buffer.js

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

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 <[email protected]>

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 <[email protected]> PR-URL: #22129 Reviewed-By: Anna Henningsen <[email protected]>

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#22129Fixes: 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: #22129Fixes: #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#22129Fixes: 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: #22129Fixes: #23668 PR-URL: #25154 Reviewed-By: James M Snell <[email protected]>

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#22129Fixes: nodejs#23668 PR-URL: nodejs#25154 Reviewed-By: James M Snell <[email protected]>

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 778ffc7Compare 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 <[email protected]>

targos force-pushed the v8-deprecation-6 branch from e59d646 to 76e79b1Compare 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

Uh oh!

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

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

Uh oh!

Conversation

ryzokuken commented Aug 4, 2018

Checklist

Uh oh!

nodejs-github-bot commented Aug 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

targos commented Sep 1, 2018

Uh oh!

targos commented Sep 1, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

targos commented Sep 2, 2018

Uh oh!

addaleax commented Sep 2, 2018

Uh oh!

jasnell commented Sep 4, 2018

Uh oh!

targos commented Sep 4, 2018

Uh oh!

targos commented Sep 5, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Sep 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

targos commented Sep 5, 2018•
edited
Loading