src: do proper error checking in AsyncWrap::MakeCallback

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);
 }
 

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/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>&);

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_)

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;
 

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

@@ -0,0 +1,49 @@
+// Flags: --experimental-worker
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+
+const { MessageChannel } = require('worker_threads');
+
+{
+  const { port1, port2 } = new MessageChannel();
+
+  // Returning a non-function in the getter should not crash.
+  Object.defineProperty(port1, 'onmessage', {
+    get() {
+      port1.unref();
+      return 42;
+    }
+  });
+
+  port2.postMessage({ foo: 'bar' });
+
+  // We need to start the port manually because .onmessage assignment tracking
+  // has been overridden.
+  port1.start();
+  port1.ref();
+}
+
+{
+  const err = new Error('eyecatcher');
+  process.on('uncaughtException', common.mustCall((exception) => {
+    port1.unref();
+    assert.strictEqual(exception, err);
+  }));
+
+  const { port1, port2 } = new MessageChannel();
+
+  // Throwing in the getter should not crash.
+  Object.defineProperty(port1, 'onmessage', {
+    get() {
+      throw err;
+    }
+  });
+
+  port2.postMessage({ foo: 'bar' });
+
+  // We need to start the port manually because .onmessage assignment tracking
+  // has been overridden.
+  port1.start();
+  port1.ref();
+}