src: remove calls to deprecated v8 functions (ToString) #21935

ryzokuken · 2018-07-22T19:00:10Z

Remove all calls to deprecated v8 functions (here: Value::ToString) 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-07-22T19:00:11Z

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

ryzokuken · 2018-07-22T19:16:49Z

Testing locally, but what we have:

not ok 1381 parallel/test-process-env-symbols
  ---
  duration_ms: 0.413
  severity: crashed
  exitcode: -6
  stack: |-
    FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
     1: 0x834fe0 node::Abort() [out/Release/node]
     2: 0x8351d2  [out/Release/node]
     3: 0x9dc661 v8::V8::ToLocalEmpty() [out/Release/node]
     4: 0x837aef  [out/Release/node]
     5: 0xded05e v8::internal::PropertyCallbackArguments::CallNamedSetter(v8::internal::Handle<v8::internal::InterceptorInfo>, v8::internal::Handle<v8::internal::Name>, v8::internal::Handle<v8::internal::Object>) [out/Release/node]
     6: 0xe7500a  [out/Release/node]
     7: 0xe8439f v8::internal::Object::SetPropertyInternal(v8::internal::LookupIterator*, v8::internal::Handle<v8::internal::Object>, v8::internal::LanguageMode, v8::internal::Object::StoreFromKeyed, bool*) [out/Release/node]
     8: 0xe84237 v8::internal::Object::SetProperty(v8::internal::LookupIterator*, v8::internal::Handle<v8::internal::Object>, v8::internal::LanguageMode, v8::internal::Object::StoreFromKeyed) [out/Release/node]
     9: 0xde338c v8::internal::StoreIC::Store(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Name>, v8::internal::Handle<v8::internal::Object>, v8::internal::Object::StoreFromKeyed) [out/Release/node]
    10: 0xde5365 v8::internal::KeyedStoreIC::Store(v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>) [out/Release/node]
    11: 0xde92e7 v8::internal::Runtime_KeyedStoreIC_Miss(int, v8::internal::Object**, v8::internal::Isolate*) [out/Release/node]
    12: 0x9e04e8841bd 
  ...

Looks related?

ryzokuken · 2018-07-22T19:36:02Z

Okay, one of the ToLocalChecked must have failed on me, but idk which one?

@addaleax good enough opportunity to add error checking for each MaybeLocal?

addaleax · 2018-07-22T19:58:48Z

@ryzokuken Yes, as far as I can tell, all of these can fail. We do need proper error handling here rather than using ToLocalChecked().

addaleax

LGTM with a bunch of nits

addaleax · 2018-07-28T17:46:51Z

src/node_crypto.cc

-    if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8))
+    bool result;
+    if (!decoder.Decode(env, args[0].As<String>(), args[1], UTF8).To(&result) ||
+        !result)


tiny nit: if (!decoder.Decode(…).FromMaybe(false)) also does the trick and is a bit more idiomatic (here and below)

addaleax · 2018-07-28T17:47:12Z

src/node_encoding.cc

+  if (r.IsNothing()) {
+    return -1;
+  }
+  return r.FromJust();


ditto, return StringBytes::Size(…).FromMaybe(-1) is just as fine

addaleax · 2018-07-28T17:48:20Z

src/spawn_sync.cc

+  if (maybe_r.IsNothing()) {
+    return v8::Nothing<bool>();
+  }
+  r = maybe_r.FromJust();


if (!ParseOptions(options).To(&r)) return v8::Nothing<bool>();?

addaleax · 2018-07-28T17:49:03Z

src/spawn_sync.cc

  HandleScope scope(env()->isolate());
  int r;

  if (!js_value->IsObject())
-    return UV_EINVAL;
+    return v8::Just(static_cast<int>(UV_EINVAL));


v8::Just<int>(UV_EINVAL)?

addaleax · 2018-07-28T17:49:29Z

src/spawn_sync.cc

+  if (r_file.IsNothing()) {
+    return v8::Nothing<int>();
+  }
+  r = r_file.FromJust();


ditto, using .To(&r) is a bit more succinct

addaleax · 2018-07-28T17:50:39Z

src/util.cc

+      StringBytes::StorageSize(isolate, string, UTF8);
+  if (maybe_storage.IsNothing())
+    return;
+  const size_t storage = maybe_storage.FromJust() + 1;


and finally, ditto :)

addaleax · 2018-07-28T20:04:23Z

src/spawn_sync.cc

  HandleScope scope(env()->isolate());
  int r;

  if (!js_value->IsObject())
-    return UV_EINVAL;
+    return v8::Just<int>(UV_EINVAL);

  Local<Context> context = env()->context();
  Local<Object> js_options = js_value.As<Object>();

  Local<Value> js_file =
      js_options->Get(context, env()->file_string()).ToLocalChecked();


Now, I know that you are a huuuuge fan of PR scope creep, but ... feel free to take care of other such places in the functions you're already touching as well, if you like?

targos · 2018-08-28T12:22:45Z

This needs a rebase. Apart from that, is it ready?

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

Rework all affected functions to use Maybes, thus improving error handling substantially in internal functions, API functions as well as utilities.

targos · 2018-08-29T14:10:55Z

Rebased, fixed conflicts and added using statements for v8::Just, v8::Maybe, v8::Nothing, etc.

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

targos · 2018-08-29T14:26:20Z

Note: I also recreated the commits to apply make format-cpp. I forgot to reset the author to @ryzokuken after that. This should be done before landing.

targos · 2018-08-31T06:46:10Z

Ping. CI is green

targos · 2018-09-01T18:38:42Z

@addaleax also this one :)

addaleax · 2018-09-02T12:32:42Z

Landed in 67403b3, a55c57b

Remove all calls to deprecated v8 functions (here: Value::ToString) inside the code (src directory only). PR-URL: #21935 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Rework all affected functions to use Maybes, thus improving error handling substantially in internal functions, API functions as well as utilities. Co-authored-by: Michaël Zasso <targos@protonmail.com> PR-URL: #21935 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Remove all calls to deprecated v8 functions (here: Value::ToString) inside the code (src directory only). PR-URL: #21935 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Rework all affected functions to use Maybes, thus improving error handling substantially in internal functions, API functions as well as utilities. Co-authored-by: Michaël Zasso <targos@protonmail.com> PR-URL: #21935 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Remove all calls to deprecated v8 functions (here: Value::ToString) inside the code (src directory only). PR-URL: #21935 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Rework all affected functions to use Maybes, thus improving error handling substantially in internal functions, API functions as well as utilities. Co-authored-by: Michaël Zasso <targos@protonmail.com> PR-URL: #21935 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Remove all calls to deprecated v8 functions (here: Value::ToString) inside the code (src directory only). PR-URL: #21935 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Rework all affected functions to use Maybes, thus improving error handling substantially in internal functions, API functions as well as utilities. Co-authored-by: Michaël Zasso <targos@protonmail.com> PR-URL: #21935 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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 Jul 22, 2018

jasnell approved these changes Jul 23, 2018

View reviewed changes

addaleax reviewed Jul 28, 2018

View reviewed changes

addaleax approved these changes Jul 28, 2018

View reviewed changes

targos added 3 commits August 29, 2018 15:39

src: remove calls to deprecated v8 functions (ToString)

4a2f922

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

src: rework (mostly internal) functions to use Maybes

96fd305

Rework all affected functions to use Maybes, thus improving error handling substantially in internal functions, API functions as well as utilities.

fixup! add using statements

d096c49

targos force-pushed the v8-deprecation-2 branch from eef4819 to d096c49 Compare August 29, 2018 14:07

addaleax closed this Sep 2, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

beevelop mentioned this pull request Sep 6, 2018

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

Closed

beevelop mentioned this pull request Sep 6, 2018

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: remove calls to deprecated v8 functions (ToString) #21935

src: remove calls to deprecated v8 functions (ToString) #21935

ryzokuken commented Jul 22, 2018

nodejs-github-bot commented Jul 22, 2018

ryzokuken commented Jul 22, 2018 •

edited

Loading

ryzokuken commented Jul 22, 2018

addaleax commented Jul 22, 2018

addaleax left a comment

addaleax Jul 28, 2018

addaleax Jul 28, 2018

addaleax Jul 28, 2018

addaleax Jul 28, 2018

addaleax Jul 28, 2018

addaleax Jul 28, 2018

addaleax Jul 28, 2018

targos commented Aug 28, 2018

targos commented Aug 29, 2018

targos commented Aug 29, 2018

targos commented Aug 31, 2018

targos commented Sep 1, 2018

addaleax commented Sep 2, 2018

src: remove calls to deprecated v8 functions (ToString) #21935

src: remove calls to deprecated v8 functions (ToString) #21935

Conversation

ryzokuken commented Jul 22, 2018

Checklist

nodejs-github-bot commented Jul 22, 2018

ryzokuken commented Jul 22, 2018 • edited Loading

ryzokuken commented Jul 22, 2018

addaleax commented Jul 22, 2018

addaleax left a comment

Choose a reason for hiding this comment

addaleax Jul 28, 2018

Choose a reason for hiding this comment

addaleax Jul 28, 2018

Choose a reason for hiding this comment

addaleax Jul 28, 2018

Choose a reason for hiding this comment

addaleax Jul 28, 2018

Choose a reason for hiding this comment

addaleax Jul 28, 2018

Choose a reason for hiding this comment

addaleax Jul 28, 2018

Choose a reason for hiding this comment

addaleax Jul 28, 2018

Choose a reason for hiding this comment

targos commented Aug 28, 2018

targos commented Aug 29, 2018

targos commented Aug 29, 2018

targos commented Aug 31, 2018

targos commented Sep 1, 2018

addaleax commented Sep 2, 2018

ryzokuken commented Jul 22, 2018 •

edited

Loading