MessagePort: ref the port when its first 'message' listener is added

src/jsc/bindings/webcore/JSMessagePort.cpp

@@ -203,10 +203,14 @@ static inline bool setJSMessagePort_onmessageSetter(JSGlobalObject& lexicalGloba
     vm.writeBarrier(&thisObject, value);
     ensureStillAliveHere(value);
 
-    // node: a callable handler starts the port and keeps the loop alive; assigning anything else
-    // clears the handler and lets the loop exit again.
+    // An installed handler is a 'message' listener: registering it started the port and, if it
+    // is the first listener, ref'd it (node's setupPortReferencing). The setter takes no ref of
+    // its own, so a handler added next to other listeners leaves an earlier unref() in force,
+    // as in node. Anything that cannot be called (a cleared handler, or an object
+    // setAttributeEventListener stores but JSEventListener never invokes) lets the loop exit
+    // again, as node's setter also only counts functions.
     if (value.isCallable())
-        thisObject.wrapped().jsRef(&lexicalGlobalObject);
+        thisObject.wrapped().didSetMessageHandler();
     else
         thisObject.wrapped().jsUnref(&lexicalGlobalObject);
 

src/jsc/bindings/webcore/MessagePort.cpp

@@ -60,14 +60,21 @@ MessagePort::MessagePort(ScriptExecutionContext& context, Ref<MessagePortPipe>&&
 {
     // The WeakPtrFactory must be initialized on the owning thread.
     EventTarget::initializeWeakPtrFactory();
-    // Any port with a 'message' listener refs the event loop (matching node: a
-    // listening port keeps its thread alive until closed or unref'd); otherwise a
-    // buffered message could be lost if its listener is added late.
+    // A port's first 'message' listener refs it (see onDidChangeListenerImpl), matching node:
+    // a listening port keeps its thread alive until it is closed or unref'd, so a buffered
+    // message is not lost to the loop exiting.
     onDidChangeListener = &MessagePort::onDidChangeListenerImpl;
 }
 
 MessagePort::~MessagePort()
 {
+    // A listening port becomes collectable as soon as its peer closes, possibly before the
+    // posted peerClosed() (which only holds a weak pointer to us) runs and releases the
+    // listener loop-ref. Release it here too, or it outlives the port and the loop never
+    // idles. m_hasRef cannot be set here: it holds a reference to this object.
+    ASSERT(!m_hasRef);
+    m_isRefd = false;
+    updateListenerEventLoopRef();
     if (!m_isDetached)
         m_pipe->close(m_side, MessagePortPipe::CloseKind::Collected);
 }
@@ -224,9 +231,8 @@ 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.
+    // Release the self-reference taken by jsRef() (.ref() from JS). The JS .close()
+    // binding calls jsUnref() first; stop() and contextDestroyed() do not.
     if (m_hasRef) {
         m_hasRef = false;
         if (auto* context = scriptExecutionContext())
@@ -285,8 +291,8 @@ void MessagePort::peerClosed()
     // Fire 'close' (guarded against a double dispatch) and release this side's loop refs
     // so the loop can idle, matching node.
     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.
+    // jsUnref() clears both the listener loop-ref (m_isRefd) and the .ref() keepalive
+    // (m_hasRef), so a listening transferred port stops pinning the loop.
     auto* globalObject = defaultGlobalObject(context->globalObject());
     jsUnref(globalObject);
 }
@@ -499,7 +505,11 @@ void MessagePort::onDidChangeListenerImpl(EventTarget& self, const AtomString& e
     auto& port = static_cast<MessagePort&>(self);
     switch (kind) {
     case Add:
-        port.m_messageEventCount++;
+        // Node's setupPortReferencing calls port.ref() for the first 'message' listener, so
+        // listening re-refs a port that was unref()'d before anyone listened. on(),
+        // addEventListener(), once() and an installed onmessage handler all register here.
+        if (++port.m_messageEventCount == 1)
+            port.setRefd();
         break;
     case Remove:
         if (port.m_messageEventCount > 0)
@@ -512,6 +522,12 @@ void MessagePort::onDidChangeListenerImpl(EventTarget& self, const AtomString& e
     port.updateListenerEventLoopRef();
 }
 
+void MessagePort::didSetMessageHandler()
+{
+    if (m_messageEventCount == 1)
+        setRefd();
+}
+
 bool MessagePort::addEventListener(const AtomString& eventType, Ref<EventListener>&& listener, const AddEventListenerOptions& options)
 {
     if (eventType == eventNames().messageEvent) {
@@ -550,24 +566,28 @@ WebCoreOpaqueRoot root(MessagePort* port)
     return WebCoreOpaqueRoot { port };
 }
 
-void MessagePort::jsRef(JSGlobalObject* lexicalGlobalObject)
+bool MessagePort::setRefd()
 {
     // 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.
+    // a ref taken now 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 listener 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))
-        return;
+        return false;
 
-    // Re-acquire the message-listener loop-ref (if a listener is present) that .unref() released.
-    if (!m_isRefd) {
-        m_isRefd = true;
-        updateListenerEventLoopRef();
-    }
+    m_isRefd = true;
+    updateListenerEventLoopRef();
+    return true;
+}
+
+void MessagePort::jsRef(JSGlobalObject* lexicalGlobalObject)
+{
+    if (!setRefd())
+        return;
 
     if (!m_hasRef) {
         m_hasRef = true;

src/jsc/bindings/webcore/MessagePort.h

@@ -115,6 +115,11 @@ class MessagePort final : public ActiveDOMObject, public EventTarget, public Thr
 
     void jsRef(JSGlobalObject*);
     void jsUnref(JSGlobalObject*);
+    // The .onmessage setter installed a callable handler. Installing one is a listener add,
+    // which refs the port like any first 'message' listener; but node's setter also
+    // re-registers a replaced handler (removeListener, then newListener), so replacing the
+    // port's only 'message' listener refs it again even after an unref(). This covers that.
+    void didSetMessageHandler();
     // Report the actual loop-ref state (matches Node's uv_has_ref), not the intent flag.
     bool jsHasRef() { return m_hasRef || m_listenerLoopRefActive; }
 
@@ -159,17 +164,24 @@ class MessagePort final : public ActiveDOMObject, public EventTarget, public Thr
     // Read from the GC thread: a port whose only listener is 'close' must survive
     // until that event is delivered, or the peer's close is lost to a collection.
     std::atomic<bool> m_hasCloseEventListener { false };
+    // The explicit .ref() hold: an event-loop ref plus a self-ref, so a ref()'d port with no
+    // listener pins the loop and outlives its wrapper (node never collects an open port).
     bool m_hasRef { false };
 
-    // Whether .ref()/.unref() want this port to keep the loop alive (default refd);
-    // independent of m_hasRef (the .onmessage=/.ref() keepalive).
-    bool m_isRefd { true };
+    // Node's ref flag as far as listeners are concerned: set by .ref() and by the first
+    // 'message' listener (node's setupPortReferencing calls port.ref() for it), cleared by
+    // .unref() and by close()/transfer/peer close. A fresh port is unref'd, as in node. It
+    // keeps the loop alive only while a 'message' listener is present.
+    bool m_isRefd { false };
     // Whether the message-listener mechanism currently holds an event-loop ref
     // (held iff m_isRefd && m_messageEventCount > 0).
     bool m_listenerLoopRefActive { false };
 
     uint32_t m_messageEventCount { 0 };
     static void onDidChangeListenerImpl(EventTarget& self, const AtomString& eventType, OnDidChangeListenerKind kind);
+    // Sets m_isRefd for .ref() and for the first 'message' listener; declines (returning false)
+    // once the port can no longer receive, see the definition.
+    bool setRefd();
     // Reconciles the listener event-loop ref with (m_isRefd && m_messageEventCount > 0).
     void updateListenerEventLoopRef();
 };