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,17 @@ 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, after
+    // sync code and microtasks) and release the loop refs there, matching node's
+    // handle which stays ref'd until its close callback runs. stop() releases
+    // them instead if teardown discards the task unrun.
     queueTaskKeepingObjectAlive(*this, TaskSource::PostedMessageQueue, [](MessagePort& port) {
+        port.jsUnref();
         port.dispatchCloseEvent();
         port.removeAllEventListeners();
     });
@@ -282,13 +268,10 @@ 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's 'close' handler sees hasRef() false (its handle closes when the
+    // peer's close arrives): release this side's loop refs, then fire 'close'.
+    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 +284,10 @@ 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
-    // 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();
-    }
+    // A transferred port is inert (the receiving side gets a fresh MessagePort
+    // for the same pipe endpoint), so nothing later would release jsRef()'s
+    // refs. The caller (disentanglePorts) holds a RefPtr across the deref().
+    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 +365,20 @@ void MessagePort::dispatchEvent(Event& event)
     EventTarget::dispatchEvent(event);
 }
 
+void MessagePort::stop()
+{
+    close();
+    // Teardown discards close()'s queued 'close' task unrun, so release the refs here.
+    jsUnref();
+}
+
 void MessagePort::contextDestroyed()
 {
     ASSERT(scriptExecutionContext());
 
-    close();
+    // jsRef()'s self-ref may be the last reference to this port.
+    Ref protectedThis { *this };
+    stop();
     ActiveDOMObject::contextDestroyed();
 }
 
@@ -552,15 +530,12 @@ 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.
-    // 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))
+    // Once the port is closed, transferred away, explicitly closed by its peer,
+    // or its 'close' event has fired (the only signal when a collected peer
+    // closed the channel), nothing would ever release a ref taken here, so a
+    // late `.ref()` or `onmessage =` would pin the loop forever. Node no-ops
+    // ref() on a closed port too.
+    if (!isEntangled() || m_pipe->isOtherSideClosedByRequest(m_side) || m_closeEventDispatched)
         return;
 
     // Re-acquire the message-listener loop-ref (if a listener is present) that .unref() released.
@@ -576,18 +551,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();
     }
+    // unrefEventLoop() balances jsRef()'s refKeepAlive; 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: drops the peer's
+    // loop refs, then fires its 'close' event.
     void peerClosed();
     void dispatchCloseEvent();
 
@@ -114,8 +114,10 @@ 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(); also drops every loop ref on close, transfer, or context stop. Idempotent.
+    void jsUnref();
+    // The actual loop-ref state, not the intent flag: node's hasRef() stays true
+    // until the handle closes, right before its 'close' event fires.
     bool jsHasRef() { return m_hasRef || m_listenerLoopRefActive; }
 
 private:
@@ -126,7 +128,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.