MessagePort: keep hasRef() true until the 'close' event fires, and false inside it

src/jsc/bindings/webcore/JSMessagePort.cpp

@@ -208,7 +208,7 @@ static inline bool setJSMessagePort_onmessageSetter(JSGlobalObject& lexicalGloba
     if (value.isCallable())
         thisObject.wrapped().jsRef(&lexicalGlobalObject);
     else
-        thisObject.wrapped().jsUnref(&lexicalGlobalObject);
+        thisObject.wrapped().jsUnref();
 
     return true;
 }
@@ -354,7 +354,6 @@ static inline JSC::EncodedJSValue jsMessagePortPrototypeFunction_closeBody(JSC::
     UNUSED_PARAM(throwScope);
     UNUSED_PARAM(callFrame);
     auto& impl = castedThis->wrapped();
-    impl.jsUnref(lexicalGlobalObject);
     RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.close(); })));
 }
 
@@ -385,7 +384,7 @@ static inline JSC::EncodedJSValue jsMessagePortPrototypeFunction_unrefBody(JSC::
     UNUSED_PARAM(throwScope);
     UNUSED_PARAM(callFrame);
     auto& impl = castedThis->wrapped();
-    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.jsUnref(lexicalGlobalObject); })));
+    RELEASE_AND_RETURN(throwScope, JSValue::encode(toJS<IDLUndefined>(*lexicalGlobalObject, throwScope, [&]() -> decltype(auto) { return impl.jsUnref(); })));
 }
 
 JSC_DEFINE_HOST_FUNCTION(jsMessagePortPrototypeFunction_unref, (JSGlobalObject * lexicalGlobalObject, CallFrame* callFrame))

src/jsc/bindings/webcore/MessagePort.cpp

@@ -224,31 +224,20 @@ void MessagePort::close()
     // it in hasPendingActivity()); marking our side Closed is sufficient.
     m_pipe->close(m_side, MessagePortPipe::CloseKind::Explicit);
 
-    // Release the self-reference taken by jsRef() (set when .onmessage is
-    // assigned or .ref() is called from JS). The JS .close() binding calls
-    // jsUnref() first; stop() and contextDestroyed() do not.
-    if (m_hasRef) {
-        m_hasRef = false;
-        if (auto* context = scriptExecutionContext())
-            context->unrefEventLoop();
-        deref();
-    }
-
-    // close() can run without a prior jsUnref() (warn-and-close, contextDestroyed());
-    // clear the listener keepalive so a later listener add can't re-ref the loop.
-    if (m_isRefd) {
-        m_isRefd = false;
-        updateListenerEventLoopRef();
-    }
-
-    // Defer 'close' to a task (node fires it at uv close-callback timing, i.e.
-    // after sync code and microtasks), so a listener added after close() still
-    // observes it and close(cb) interleaves with other listeners.
     if (isContextStopped()) {
+        jsUnref();
         removeAllEventListeners();
         return;
     }
+    // Defer 'close' to a task (node fires it at uv close-callback timing, i.e.
+    // after sync code and microtasks), so a listener added after close() still
+    // observes it and close(cb) interleaves with other listeners. The loop refs
+    // are released by that task, not here: node's handle stays ref'd (hasRef()
+    // true) until its close callback, which marks it closed right before 'close'
+    // (https://github.com/nodejs/node/blob/v26.3.0/src/handle_wrap.cc#L135-L156).
+    // stop() drops them instead if teardown discards the task unrun.
     queueTaskKeepingObjectAlive(*this, TaskSource::PostedMessageQueue, [](MessagePort& port) {
+        port.jsUnref();
         port.dispatchCloseEvent();
         port.removeAllEventListeners();
     });
@@ -282,13 +271,12 @@ void MessagePort::peerClosed()
     // drain is scheduled -- e.g. on('close') registered before on('message').
     if (m_started && hasMessageEventListener())
         flushQueuedMessagesBeforeClose();
-    // Fire 'close' (guarded against a double dispatch) and release this side's loop refs
-    // so the loop can idle, matching node.
+    // Node closes this side's handle when the peer's close arrives, so its 'close'
+    // handler already sees hasRef() false: release this side's loop refs (both the
+    // listener one and the onmessage/ref() one, so a listening transferred port stops
+    // pinning the loop), then fire 'close' (guarded against a double dispatch).
+    jsUnref();
     dispatchCloseEvent();
-    // jsUnref() clears both the listener loop-ref (m_isRefd) and the onmessage/ref()
-    // keepalive (m_hasRef), so a listening transferred port stops pinning the loop.
-    auto* globalObject = defaultGlobalObject(context->globalObject());
-    jsUnref(globalObject);
 }
 
 TransferredMessagePort MessagePort::disentangle()
@@ -301,24 +289,12 @@ TransferredMessagePort MessagePort::disentangle()
     removeAllEventListeners();
     m_hasMessageEventListener = false;
 
-    // Release the self-reference taken by jsRef() on the sending side. After
-    // transfer this object is inert (the receiving side gets a fresh
+    // After transfer this object is inert (the receiving side gets a fresh
     // MessagePort for the same pipe endpoint) and is no longer a destruction
-    // observer, so nothing else will ever release a ref taken here.
-    // The caller (disentanglePorts) holds a RefPtr, so deref() is safe.
-    if (m_hasRef) {
-        m_hasRef = false;
-        if (auto* context = scriptExecutionContext())
-            context->unrefEventLoop();
-        deref();
-    }
-
-    // A transferred port is inert; clear the listener keepalive too so hasRef()
-    // reports false (the disentangle analogue of the close() reset above).
-    if (m_isRefd) {
-        m_isRefd = false;
-        updateListenerEventLoopRef();
-    }
+    // observer, so nothing else would ever release jsRef()'s self-ref and
+    // loop ref, or the listener keepalive. The caller (disentanglePorts)
+    // holds a RefPtr, so the deref() is safe.
+    jsUnref();
 
     // Hand the pipe endpoint to its next owner. Messages that arrive while
     // in transit buffer in the pipe; the receiving context's entangle()
@@ -396,11 +372,22 @@ void MessagePort::dispatchEvent(Event& event)
     EventTarget::dispatchEvent(event);
 }
 
+void MessagePort::stop()
+{
+    close();
+    // A close() that ran earlier left the refs for its 'close' task to release, and
+    // teardown discards queued tasks unrun.
+    jsUnref();
+}
+
 void MessagePort::contextDestroyed()
 {
     ASSERT(scriptExecutionContext());
 
-    close();
+    // A context destroyed without a stop phase still has to release the refs, and
+    // jsRef()'s self-ref may be the last reference to this port.
+    Ref protectedThis { *this };
+    stop();
     ActiveDOMObject::contextDestroyed();
 }
 
@@ -553,11 +540,13 @@ WebCoreOpaqueRoot root(MessagePort* port)
 void MessagePort::jsRef(JSGlobalObject* lexicalGlobalObject)
 {
     // A closed or transferred-away port can never receive messages again, so
-    // taking a self-ref (and an event-loop ref) here would only leak:
-    // close()/disentangle() have already run and nothing will ever release a
-    // ref taken afterwards. Same once the peer has closed: peerClosed() already
-    // ran jsUnref(), and nothing releases a ref re-taken after it, so `.ref()`
-    // or a late `onmessage =` would pin the loop forever. Node no-ops both.
+    // taking a self-ref (and an event-loop ref) here would only leak: once
+    // close()'s 'close' task (or disentangle()) has released the refs, nothing
+    // will ever release one taken afterwards. (Node still honours ref() in the
+    // window before its close callback; it is pointless there, so we don't.)
+    // Same once the peer has closed: peerClosed() already ran jsUnref(), and
+    // nothing releases a ref re-taken after it, so `.ref()` or a late
+    // `onmessage =` would pin the loop forever. Node no-ops both.
     // Only an explicit peer close counts: node never closes a channel because a
     // port was collected, so keying on Closed alone made this GC-dependent.
     if (!isEntangled() || m_pipe->isOtherSideClosedByRequest(m_side))
@@ -576,18 +565,21 @@ void MessagePort::jsRef(JSGlobalObject* lexicalGlobalObject)
     }
 }
 
-void MessagePort::jsUnref(JSGlobalObject* lexicalGlobalObject)
+void MessagePort::jsUnref()
 {
     // Also release the listener loop-ref; otherwise an always-listening transferred
     // port (a postMessageToThread control port) would pin the event loop forever.
     if (m_isRefd) {
         m_isRefd = false;
         updateListenerEventLoopRef();
     }
+    // The context's unrefEventLoop() balances jsRef()'s refKeepAlive: both address the
+    // thread's one bun VM. Every caller holds a reference across the deref().
     if (m_hasRef) {
         m_hasRef = false;
+        if (auto* context = scriptExecutionContext())
+            context->unrefEventLoop();
         deref();
-        Bun__eventLoop__refKeepAlive(WebCore::clientData(lexicalGlobalObject->vm())->bunVM, -1);
     }
 }
 

src/jsc/bindings/webcore/MessagePort.h

@@ -79,8 +79,8 @@ class MessagePort final : public ActiveDOMObject, public EventTarget, public Thr
     // The worker's entry module finished evaluating: a start() requested before that takes effect now.
     void entrySettled();
     void close();
-    // Called on the entangled peer when this side closes: dispatches a
-    // 'close' event and releases the event-loop ref so the loop can idle.
+    // Called on the entangled peer when this side closes: releases the peer's
+    // event-loop refs so the loop can idle, then dispatches its 'close' event.
     void peerClosed();
     void dispatchCloseEvent();
 
@@ -114,8 +114,12 @@ class MessagePort final : public ActiveDOMObject, public EventTarget, public Thr
     JSValue tryTakeMessage(JSGlobalObject*, bool& hadMessage);
 
     void jsRef(JSGlobalObject*);
-    void jsUnref(JSGlobalObject*);
-    // Report the actual loop-ref state (matches Node's uv_has_ref), not the intent flag.
+    // .unref(), and the step that drops every loop ref this port holds when it is
+    // closed, transferred away, or its context stops. Idempotent.
+    void jsUnref();
+    // Report the actual loop-ref state, not the intent flag: node's HandleWrap::HasRef()
+    // is uv_has_ref() until the handle is closed, so a closing port keeps its refs until
+    // its 'close' event fires (https://github.com/nodejs/node/blob/v26.3.0/src/handle_wrap.h#L64-L72).
     bool jsHasRef() { return m_hasRef || m_listenerLoopRefActive; }
 
 private:
@@ -126,7 +130,7 @@ class MessagePort final : public ActiveDOMObject, public EventTarget, public Thr
 
     // ActiveDOMObject.
     void contextDestroyed() final;
-    void stop() final { close(); }
+    void stop() final;
     bool virtualHasPendingActivity() const final;
 
     // Deliver messages already queued when close() is called, before teardown.