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: |-

src/async_wrap-inl.h

@@ -27,6 +27,7 @@
 #include "async_wrap.h"
 #include "base_object-inl.h"
 #include "node_internals.h"
+#include "node_errors.h"
 
 namespace node {
 
@@ -81,18 +82,17 @@ 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()) {
+    // Due to V8’s error handling mechanisms, this will not show up as an error
+    // in the common case, which is that this is outside of any JS frame.
+    // So, the exception here is mostly just there to fulfill the
+    // `MaybeLocal<>` API contract.
+    THROW_ERR_MISSING_METHOD(env()->isolate(), symbol);
+    return v8::MaybeLocal<v8::Value>();
+  }
   return MakeCallback(cb_v.As<v8::Function>(), argc, argv);
 }
 

src/async_wrap.h

@@ -172,9 +172,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;

src/node_errors.h

@@ -33,6 +33,7 @@ namespace node {
   V(ERR_MEMORY_ALLOCATION_FAILED, Error)                                     \
   V(ERR_MISSING_ARGS, TypeError)                                             \
   V(ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST, TypeError)                    \
+  V(ERR_MISSING_METHOD, TypeError)                                           \
   V(ERR_MISSING_MODULE, Error)                                               \
   V(ERR_MISSING_PLATFORM_FOR_WORKER, Error)                                  \
   V(ERR_SCRIPT_EXECUTION_INTERRUPTED, Error)                                 \
@@ -52,8 +53,11 @@ namespace node {
            js_code).FromJust();                                               \
     return e;                                                                 \
   }                                                                           \
+  inline void THROW_ ## code(v8::Isolate* isolate, const char* message) {     \
+    isolate->ThrowException(code(isolate, message));                          \
+  }                                                                           \
   inline void THROW_ ## code(Environment* env, const char* message) {         \
-    env->isolate()->ThrowException(code(env->isolate(), message));            \
+    THROW_ ## code (env->isolate(), message);                                 \
   }
   ERRORS_WITH_CODE(V)
 #undef V
@@ -112,6 +116,14 @@ inline v8::Local<v8::Value> ERR_STRING_TOO_LONG(v8::Isolate* isolate) {
   return ERR_STRING_TOO_LONG(isolate, message);
 }
 
+inline void THROW_ERR_MISSING_METHOD(v8::Isolate* isolate,
+                                     v8::Local<v8::Name> name) {
+  Utf8Value name_str(isolate, name);
+  std::string message("Missing method: ");
+  message += *name_str;
+  THROW_ERR_MISSING_METHOD(isolate, message.c_str());
+}
+
 #define THROW_AND_RETURN_IF_NOT_BUFFER(env, val, prefix)                     \
   do {                                                                       \
     if (!Buffer::HasInstance(val))                                           \

src/node_messaging.cc

@@ -476,7 +476,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_)

test/parallel/test-async-wrap-missing-method.js

@@ -0,0 +1,32 @@
+// Flags: --experimental-worker
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+const { MessageChannel, MessagePort } = require('worker_threads');
+
+{
+  const { port1, port2 } = new MessageChannel();
+
+  // Throwing in the getter should not crash.
+  Object.defineProperty(port1, 'onmessage', {
+    get() {
+      throw new Error('eyecatcher');
+    }
+  });
+
+  port2.postMessage({ foo: 'bar' });
+}
+
+{
+  const { port1, port2 } = new MessageChannel();
+
+  // Returning a non-function in the getter should not crash.
+  Object.defineProperty(port1, 'onmessage', {
+    get() {
+      return 42;
+    }
+  });
+
+  port2.postMessage({ foo: 'bar' });
+}