diff --git a/src/async_wrap-inl.h b/src/async_wrap-inl.h index 5763b17aa08bc4..4405bb3a9baa52 100644 --- a/src/async_wrap-inl.h +++ b/src/async_wrap-inl.h @@ -81,18 +81,14 @@ inline v8::MaybeLocal AsyncWrap::MakeCallback( const v8::Local symbol, int argc, v8::Local* argv) { - v8::Local cb_v = object()->Get(symbol); - CHECK(cb_v->IsFunction()); - return MakeCallback(cb_v.As(), argc, argv); -} - - -inline v8::MaybeLocal AsyncWrap::MakeCallback( - uint32_t index, - int argc, - v8::Local* argv) { - v8::Local cb_v = object()->Get(index); - CHECK(cb_v->IsFunction()); + v8::Local cb_v; + if (!object()->Get(env()->context(), symbol).ToLocal(&cb_v)) + return v8::MaybeLocal(); + if (!cb_v->IsFunction()) { + // TODO(addaleax): We should throw an error here to fulfill the + // `MaybeLocal<>` API contract. + return v8::MaybeLocal(); + } return MakeCallback(cb_v.As(), argc, argv); } diff --git a/src/async_wrap.h b/src/async_wrap.h index f696facd485188..82c57910925be9 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -173,9 +173,6 @@ class AsyncWrap : public BaseObject { const v8::Local symbol, int argc, v8::Local* argv); - inline v8::MaybeLocal MakeCallback(uint32_t index, - int argc, - v8::Local* 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 4e177d249f28b5..bd7ef4000bad6f 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&); diff --git a/src/node_messaging.cc b/src/node_messaging.cc index 352749ea48f483..407557d4f4ae13 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 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& 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 9a13437d19a331..28122c526cccea 100644 --- a/src/node_messaging.h +++ b/src/node_messaging.h @@ -154,6 +154,8 @@ class MessagePort : public HandleWrap { std::unique_ptr Detach(); bool IsSiblingClosed() const; + void Close( + v8::Local close_callback = v8::Local()) override; size_t self_size() const override; diff --git a/test/parallel/test-async-wrap-missing-method.js b/test/parallel/test-async-wrap-missing-method.js new file mode 100644 index 00000000000000..72d6d9fdf3fac4 --- /dev/null +++ b/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(); +}