src: do proper error checking in `AsyncWrap::MakeCallback`

At least one method on a native object is added as a getter,
namely `MessagePort.prototype.onmessage`. When a MessagePort
attempts to call this method from C++ in response to receiving
data, it will first invoke that getter and then call the function.

Since `worker.terminate()` interrupts execution, this means
that the getter may fail (without being faulty code on its own).
This means that at least one test exercising these methods in
combination has been flaky and could have crashed, because
we did not actually check that the getter returns a value
so far, resulting in dereferencing an empty `Local`.

The proper fix for this is to use the non-deprecated overload
of `Get()` and check the result like we should be doing.
Also, as a (related) fix, don’t crash if the method
is not a function but rather something else, like a getter
could provide.

Example test failure: https://ci.nodejs.org/job/node-test-commit-linux-containered/4976/nodes=ubuntu1604_sharedlibs_zlib_x64/console

    17:56:56 not ok 1955 parallel/test-worker-dns-terminate
    17:56:56   ---
    17:56:56   duration_ms: 1.237
    17:56:56   severity: crashed
    17:56:56   exitcode: -11
    17:56:56   stack: |-

PR-URL: https://github.com/nodejs/node/pull/21189
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

5 files changed, 26 insertions, 16 deletions

diff --git a/src/async_wrap-inl.h b/src/async_wrap-inl.h
index 5763b17aa0..4405bb3a9b 100644
--- a/src/async_wrap-inl.h
+++ b/src/async_wrap-inl.h
@@ -81,18 +81,14 @@ inline v8::MaybeLocal<v8::Value> AsyncWrap::MakeCallback(
     const v8::Local<v8::Name> symbol,
     int argc,
     v8::Local<v8::Value>* argv) {
-  v8::Local<v8::Value> cb_v = object()->Get(symbol);
-  CHECK(cb_v->IsFunction());
-  return MakeCallback(cb_v.As<v8::Function>(), argc, argv);
-}
-
-
-inline v8::MaybeLocal<v8::Value> AsyncWrap::MakeCallback(
-    uint32_t index,
-    int argc,
-    v8::Local<v8::Value>* argv) {
-  v8::Local<v8::Value> cb_v = object()->Get(index);
-  CHECK(cb_v->IsFunction());
+  v8::Local<v8::Value> cb_v;
+  if (!object()->Get(env()->context(), symbol).ToLocal(&cb_v))
+    return v8::MaybeLocal<v8::Value>();
+  if (!cb_v->IsFunction()) {
+    // TODO(addaleax): We should throw an error here to fulfill the
+    // `MaybeLocal<>` API contract.
+    return v8::MaybeLocal<v8::Value>();
+  }
   return MakeCallback(cb_v.As<v8::Function>(), argc, argv);
 }
 
diff --git a/src/async_wrap.h b/src/async_wrap.h
index f696facd48..82c5791092 100644
--- a/src/async_wrap.h
+++ b/src/async_wrap.h
@@ -173,9 +173,6 @@ class AsyncWrap : public BaseObject {
       const v8::Local<v8::Name> symbol,
       int argc,
       v8::Local<v8::Value>* argv);
-  inline v8::MaybeLocal<v8::Value> MakeCallback(uint32_t index,
-                                                int argc,
-                                                v8::Local<v8::Value>* argv);
 
   virtual size_t self_size() const = 0;
   virtual std::string diagnostic_name() const;
diff --git a/src/handle_wrap.h b/src/handle_wrap.h
index 4e177d249f..bd7ef4000b 100644
--- a/src/handle_wrap.h
+++ b/src/handle_wrap.h
@@ -83,6 +83,10 @@ class HandleWrap : public AsyncWrap {
   void MarkAsInitialized();
   void MarkAsUninitialized();
 
+  inline bool IsHandleClosing() const {
+    return state_ == kClosing || state_ == kClosed;
+  }
+
  private:
   friend class Environment;
   friend void GetActiveHandles(const v8::FunctionCallbackInfo<v8::Value>&);
diff --git a/src/node_messaging.cc b/src/node_messaging.cc
index 352749ea48..407557d4f4 100644
--- a/src/node_messaging.cc
+++ b/src/node_messaging.cc
@@ -391,9 +391,21 @@ uv_async_t* MessagePort::async() {
 }
 
 void MessagePort::TriggerAsync() {
+  if (IsHandleClosing()) return;
   CHECK_EQ(uv_async_send(async()), 0);
 }
 
+void MessagePort::Close(v8::Local<v8::Value> close_callback) {
+  if (data_) {
+    // Wrap this call with accessing the mutex, so that TriggerAsync()
+    // can check IsHandleClosing() without race conditions.
+    Mutex::ScopedLock sibling_lock(data_->mutex_);
+    HandleWrap::Close(close_callback);
+  } else {
+    HandleWrap::Close(close_callback);
+  }
+}
+
 void MessagePort::New(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   if (!args.IsConstructCall()) {
@@ -476,7 +488,6 @@ void MessagePort::OnMessage() {
       };
 
       if (args[0].IsEmpty() ||
-          !object()->Has(context, env()->onmessage_string()).FromMaybe(false) ||
           MakeCallback(env()->onmessage_string(), 1, args).IsEmpty()) {
         // Re-schedule OnMessage() execution in case of failure.
         if (data_)
diff --git a/src/node_messaging.h b/src/node_messaging.h
index 9a13437d19..28122c526c 100644
--- a/src/node_messaging.h
+++ b/src/node_messaging.h
@@ -154,6 +154,8 @@ class MessagePort : public HandleWrap {
   std::unique_ptr<MessagePortData> Detach();
 
   bool IsSiblingClosed() const;
+  void Close(
+      v8::Local<v8::Value> close_callback = v8::Local<v8::Value>()) override;
 
   size_t self_size() const override;