Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,10 @@
"ios": "cpp",
"oxlint.json": "jsonc",
"bun.lock": "jsonc",
"*.inc": "cpp",
"__locale": "cpp",
"locale": "cpp",
"fstream": "cpp",
},
"C_Cpp.files.exclude": {
"**/.vscode": true,
Expand Down
33 changes: 28 additions & 5 deletions src/bun.js/bindings/webcore/MessagePort.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include <wtf/TZoneMallocInlines.h>
#include <wtf/Lock.h>
#include <wtf/Scope.h>
#include "Event.h"

extern "C" void Bun__eventLoop__incrementRefConcurrently(void* bunVM, int delta);

Expand Down Expand Up @@ -104,6 +105,7 @@ void MessagePort::notifyMessageAvailable(const MessagePortIdentifier& identifier
Ref<MessagePort> MessagePort::create(ScriptExecutionContext& scriptExecutionContext, const MessagePortIdentifier& local, const MessagePortIdentifier& remote)
{
auto messagePort = adoptRef(*new MessagePort(scriptExecutionContext, local, remote));
messagePort->m_strongRef = messagePort.ptr();
// messagePort->suspendIfNeeded();
return messagePort;
}
Expand Down Expand Up @@ -229,18 +231,37 @@ void MessagePort::start()
return;

m_started = true;
scriptExecutionContext()->processMessageWithMessagePortsSoon([pendingActivity = Ref { *this }] {});
auto* context = scriptExecutionContext();
context->processMessageWithMessagePortsSoon([pendingActivity = Ref { *this }] {});
context->refEventLoop();
}

void MessagePort::close()
{
if (m_isDetached)
shouldClose = true;

tryClose();
}

void MessagePort::tryClose()
{
if (!shouldClose|| m_isDetached || m_messagesInFlight > 0)
return;
m_isDetached = true;

dispatchCloseEvent();
MessagePortChannelProvider::singleton().messagePortClosed(m_identifier);

removeAllEventListeners();

// message port is closed, we can safely mark it for GC
this->m_strongRef = nullptr;

scriptExecutionContext()->unrefEventLoop();
}

void MessagePort::dispatchCloseEvent()
{
dispatchEvent(Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No));
}

void MessagePort::dispatchMessages()
Expand All @@ -266,6 +287,7 @@ void MessagePort::dispatchMessages()
auto* globalObject = defaultGlobalObject(context->globalObject());
Ref vm = globalObject->vm();
auto scope = DECLARE_CATCH_SCOPE(vm);
protectedThis->m_messagesInFlight += messages.size();

for (auto& message : messages) {
// close() in Worker onmessage handler should prevent next message from dispatching.
Expand All @@ -287,6 +309,8 @@ void MessagePort::dispatchMessages()
ScriptExecutionContext::postTaskTo(context->identifier(), [protectedThis = Ref { *this }, ports = WTFMove(ports), message = WTFMove(message)](ScriptExecutionContext& context) mutable {
auto event = MessageEvent::create(*context.jsGlobalObject(), message.message.releaseNonNull(), {}, {}, {}, WTFMove(ports));
protectedThis->dispatchEvent(event.event);
protectedThis->m_messagesInFlight -= 1;
protectedThis->tryClose();
});
}
};
Expand Down Expand Up @@ -427,8 +451,7 @@ Ref<MessagePort> MessagePort::entangle(ScriptExecutionContext& context, Transfer
bool MessagePort::addEventListener(const AtomString& eventType, Ref<EventListener>&& listener, const AddEventListenerOptions& options)
{
if (eventType == eventNames().messageEvent) {
if (listener->isAttribute())
start();
Comment on lines -430 to -431

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment on actual issue. #19863 (comment)

I will emphasis that we should make WebCore implementation to align withe the JS specs and have that NodeJS compatibility layer abstracted away.

And until then just having documentation for such points would be suffice.

start();
m_hasMessageEventListener = true;
}
return EventTarget::addEventListener(eventType, WTFMove(listener), options);
Expand Down
6 changes: 6 additions & 0 deletions src/bun.js/bindings/webcore/MessagePort.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ class MessagePort final : /* public ActiveDOMObject, */ public ContextDestructio
bool isDetached() const { return m_isDetached; }

void dispatchMessages();
void dispatchCloseEvent();

// Returns null if there is no entangled port, or if the entangled port is run by a different thread.
// This is used solely to enable a GC optimization. Some platforms may not be able to determine ownership
Expand Down Expand Up @@ -149,6 +150,11 @@ class MessagePort final : /* public ActiveDOMObject, */ public ContextDestructio

uint32_t m_messageEventCount { 0 };
static void onDidChangeListenerImpl(EventTarget& self, const AtomString& eventType, OnDidChangeListenerKind kind);
// keeps MessagePort alive until we close()
RefPtr<MessagePort> m_strongRef = nullptr;
unsigned m_messagesInFlight = 0;
bool shouldClose = false;
void tryClose();
};

WebCoreOpaqueRoot root(MessagePort*);
Expand Down
1 change: 1 addition & 0 deletions src/bun.js/bindings/webcore/SerializedScriptValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5779,6 +5779,7 @@ ExceptionOr<Ref<SerializedScriptValue>> SerializedScriptValue::create(JSGlobalOb
if (auto port = JSMessagePort::toWrapped(vm, transferable.get())) {
if (port->isDetached())
return Exception { DataCloneError, "MessagePort in transfer list is already detached"_s };
port->dispatchCloseEvent();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dispatchCloseEvent() call during transfer appears to be inconsistent with the MessagePort specification. According to the standard, MessagePorts should only dispatch 'close' events when explicitly closed via the close() method, not when transferred between contexts. Transferring a port detaches it from its original context but doesn't conceptually "close" it - it continues to exist in the target context. This behavior difference could cause compatibility issues with Node.js and other standard implementations.

Suggested change
port->dispatchCloseEvent();
port->detachWithoutClosing();

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

messagePorts.append(WTFMove(port));
continue;
}
Expand Down