Skip to content
Permalink
Browse files
MessagePort is not always destroyed on the right thread
https://bugs.webkit.org/show_bug.cgi?id=183619
<rdar://problem/38204711>

Reviewed by Chris Dumez.

Source/WebCore:

Add assertion to ensure MessagePort is destroyed in the right thread.
Modify methods taking a ref in a lambda to rely on weak pointers and refing the WorkerThread if in a worker context.
It is safe to ref the WorkerThread since it is thread safe ref counted and we are passing the ref to the main thread
where the WorkerThread is expected to be destroyed.

Test: http/tests/workers/worker-messageport-2.html

* dom/MessagePort.cpp:
(WebCore::MessagePort::~MessagePort):
(WebCore::MessagePort::dispatchMessages):
(WebCore::MessagePort::updateActivity):
(WebCore::MessagePort::hasPendingActivity const):
* dom/MessagePort.h:

LayoutTests:

* TestExpectations:
* http/tests/workers/worker-messageport-2-expected.txt: Added.
* http/tests/workers/worker-messageport-2.html: Added.


Canonical link: https://commits.webkit.org/199308@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@229628 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
youennf committed Mar 15, 2018
1 parent 799eb19 commit a2b95601bebfd3d370821588f6d2bfd3b40092af
@@ -1,3 +1,15 @@
2018-03-15 Youenn Fablet <youenn@apple.com>

MessagePort is not always destroyed on the right thread
https://bugs.webkit.org/show_bug.cgi?id=183619
<rdar://problem/38204711>

Reviewed by Chris Dumez.

* TestExpectations:
* http/tests/workers/worker-messageport-2-expected.txt: Added.
* http/tests/workers/worker-messageport-2.html: Added.

2018-03-15 Ms2ger <Ms2ger@igalia.com>

[GTK][WPE] Enable service workers
@@ -211,6 +211,8 @@ imported/w3c/web-platform-tests/service-workers/service-worker/fetch-request-red
# Reenable it when having a custom gc exposed in service workers.
[ Debug ] http/wpt/service-workers/fetchEvent.https.html [ Skip ]

[ Debug ] http/tests/workers/worker-messageport-2.html [ Slow ]

# Skip workers tests that are timing out or are SharedWorker related only
imported/w3c/web-platform-tests/workers/constructors/Worker/same-origin.html [ Skip ]
imported/w3c/web-platform-tests/workers/data-url-shared.html [ Skip ]
@@ -0,0 +1 @@
PASS
@@ -0,0 +1,45 @@
<html>
<body>
<p>Test postMessage and garbage collection.</p>
<div id=result></div>
<script>
if (window.testRunner) {
testRunner.dumpAsText();
testRunner.waitUntilDone();
}

var worker = new Worker('resources/messageport-echo-worker.js');

worker.onmessage = (event) => {
if (event.data !== "ready") {
document.body.innerHTML = "FAIL";
if (window.testRunner)
testRunner.notifyDone();
return;
}
worker.terminate();

function gcRec(n) {
if (n < 1)
return {};
var temp = {i: "ab" + i + (i / 100000)};
temp += "foo";
gcRec(n-1);
}
for (var i = 0; i < 100000; i++)
gcRec(10);

setTimeout(() => {
document.body.innerHTML = "PASS";
if (window.testRunner)
testRunner.notifyDone();
}, 0);
}

var channel = new MessageChannel();
channel.port1.onmessage = (event) => { };
worker.postMessage("Here is your port", [channel.port2]);

</script>
</body>
</html>
@@ -1,3 +1,25 @@
2018-03-15 Youenn Fablet <youenn@apple.com>

MessagePort is not always destroyed on the right thread
https://bugs.webkit.org/show_bug.cgi?id=183619
<rdar://problem/38204711>

Reviewed by Chris Dumez.

Add assertion to ensure MessagePort is destroyed in the right thread.
Modify methods taking a ref in a lambda to rely on weak pointers and refing the WorkerThread if in a worker context.
It is safe to ref the WorkerThread since it is thread safe ref counted and we are passing the ref to the main thread
where the WorkerThread is expected to be destroyed.

Test: http/tests/workers/worker-messageport-2.html

* dom/MessagePort.cpp:
(WebCore::MessagePort::~MessagePort):
(WebCore::MessagePort::dispatchMessages):
(WebCore::MessagePort::updateActivity):
(WebCore::MessagePort::hasPendingActivity const):
* dom/MessagePort.h:

2018-03-15 Jer Noble <jer.noble@apple.com>

Adopt new AVURLAssetUseClientURLLoadingExclusively AVURLAsset creation option.
@@ -34,6 +34,7 @@
#include "MessagePortChannelProvider.h"
#include "MessageWithMessagePorts.h"
#include "WorkerGlobalScope.h"
#include "WorkerThread.h"

namespace WebCore {

@@ -108,6 +109,8 @@ MessagePort::MessagePort(ScriptExecutionContext& scriptExecutionContext, const M

MessagePort::~MessagePort()
{
ASSERT(m_creationThread.ptr() == &Thread::current());

LOG(MessagePorts, "Destroyed MessagePort %s (%p) in process %" PRIu64, m_identifier.logString().utf8().data(), this, Process::identifier().toUInt64());

ASSERT(allMessagePortsLock().isLocked());
@@ -243,8 +246,16 @@ void MessagePort::dispatchMessages()
if (!isEntangled())
return;

auto messagesTakenHandler = [this, protectedThis = makeRef(*this)](Vector<MessageWithMessagePorts>&& messages, Function<void()>&& completionCallback) mutable {
auto innerHandler = [this, otherProtectedThis = WTFMove(protectedThis)](Vector<MessageWithMessagePorts>&& messages) {
RefPtr<WorkerThread> workerThread;
if (is<WorkerGlobalScope>(*m_scriptExecutionContext))
workerThread = &downcast<WorkerGlobalScope>(*m_scriptExecutionContext).thread();

auto messagesTakenHandler = [this, weakThis = makeWeakPtr(this), workerThread = WTFMove(workerThread)](Vector<MessageWithMessagePorts>&& messages, Function<void()>&& completionCallback) mutable {
ASSERT(isMainThread());
auto innerHandler = [this, weakThis = WTFMove(weakThis)](auto&& messages) {
if (!weakThis)
return;

LOG(MessagePorts, "MessagePort %s (%p) dispatching %zu messages", m_identifier.logString().utf8().data(), this, messages.size());

if (!m_scriptExecutionContext)
@@ -265,26 +276,36 @@ void MessagePort::dispatchMessages()
}
};

if (!m_scriptExecutionContext)
return;

if (m_scriptExecutionContext->isContextThread()) {
if (!workerThread) {
innerHandler(WTFMove(messages));
completionCallback();
return;
}

m_scriptExecutionContext->postTask([innerHandler = WTFMove(innerHandler), messages = WTFMove(messages), completionCallback = WTFMove(completionCallback)](ScriptExecutionContext&) mutable {
workerThread->runLoop().postTaskForMode([innerHandler = WTFMove(innerHandler), messages = WTFMove(messages), completionCallback = WTFMove(completionCallback)](auto&) mutable {
innerHandler(WTFMove(messages));
RunLoop::main().dispatch([completionCallback = WTFMove(completionCallback)] {
completionCallback();
});
});
}, WorkerRunLoop::defaultMode());
};

MessagePortChannelProvider::singleton().takeAllMessagesForPort(m_identifier, WTFMove(messagesTakenHandler));
}

void MessagePort::updateActivity(MessagePortChannelProvider::HasActivity hasActivity)
{
bool hasHadLocalActivity = m_hasHadLocalActivitySinceLastCheck;
m_hasHadLocalActivitySinceLastCheck = false;

if (hasActivity == MessagePortChannelProvider::HasActivity::No && !hasHadLocalActivity)
m_isRemoteEligibleForGC = true;

if (hasActivity == MessagePortChannelProvider::HasActivity::Yes)
m_isRemoteEligibleForGC = false;

m_isAskingRemoteAboutGC = false;
}

bool MessagePort::hasPendingActivity() const
{
m_mightBeEligibleForGC = true;
@@ -303,32 +324,23 @@ bool MessagePort::hasPendingActivity() const

// If we're not in the middle of asking the remote port about collectability, do so now.
if (!m_isAskingRemoteAboutGC) {
MessagePortChannelProvider::singleton().checkRemotePortForActivity(m_remoteIdentifier, [this, protectedThis = makeRef(*this)](MessagePortChannelProvider::HasActivity hasActivity) mutable {
auto innerHandler = [this, otherProtectedThis = WTFMove(protectedThis)](MessagePortChannelProvider::HasActivity hasActivity) {
bool hasHadLocalActivity = m_hasHadLocalActivitySinceLastCheck;
m_hasHadLocalActivitySinceLastCheck = false;

if (hasActivity == MessagePortChannelProvider::HasActivity::No && !hasHadLocalActivity)
m_isRemoteEligibleForGC = true;

if (hasActivity == MessagePortChannelProvider::HasActivity::Yes)
m_isRemoteEligibleForGC = false;
RefPtr<WorkerThread> workerThread;
if (is<WorkerGlobalScope>(*m_scriptExecutionContext))
workerThread = &downcast<WorkerGlobalScope>(*m_scriptExecutionContext).thread();

m_isAskingRemoteAboutGC = false;
};
MessagePortChannelProvider::singleton().checkRemotePortForActivity(m_remoteIdentifier, [weakThis = makeWeakPtr(const_cast<MessagePort*>(this)), workerThread = WTFMove(workerThread)](MessagePortChannelProvider::HasActivity hasActivity) mutable {


if (!m_scriptExecutionContext)
return;

if (m_scriptExecutionContext->isContextThread()) {
innerHandler(hasActivity);
ASSERT(isMainThread());
if (!workerThread) {
if (weakThis)
weakThis->updateActivity(hasActivity);
return;
}

m_scriptExecutionContext->postTask([innerHandler = WTFMove(innerHandler), hasActivity](ScriptExecutionContext&) mutable {
innerHandler(hasActivity);
});
workerThread->runLoop().postTaskForMode([weakThis = WTFMove(weakThis), hasActivity](auto&) mutable {
if (weakThis)
weakThis->updateActivity(hasActivity);
}, WorkerRunLoop::defaultMode());
});
m_isAskingRemoteAboutGC = true;
}
@@ -32,6 +32,8 @@
#include "MessagePortChannel.h"
#include "MessagePortIdentifier.h"
#include "MessageWithMessagePorts.h"
#include <wtf/Threading.h>
#include <wtf/WeakPtr.h>

namespace JSC {
class ExecState;
@@ -48,6 +50,8 @@ class MessagePort final : public ActiveDOMObject, public EventTargetWithInlineDa
static Ref<MessagePort> create(ScriptExecutionContext&, const MessagePortIdentifier& local, const MessagePortIdentifier& remote);
virtual ~MessagePort();

auto& weakPtrFactory() const { return m_weakFactory; }

ExceptionOr<void> postMessage(JSC::ExecState&, JSC::JSValue message, Vector<JSC::Strong<JSC::JSObject>>&&);

void start();
@@ -106,10 +110,14 @@ class MessagePort final : public ActiveDOMObject, public EventTargetWithInlineDa
// A port starts out its life entangled, and remains entangled until it is closed or is cloned.
bool isEntangled() const { return !m_closed && m_entangled; }

void updateActivity(MessagePortChannelProvider::HasActivity);

bool m_started { false };
bool m_closed { false };
bool m_entangled { true };

WeakPtrFactory<MessagePort> m_weakFactory;

// Flags to manage querying the remote port for GC purposes
mutable bool m_mightBeEligibleForGC { false };
mutable bool m_hasHadLocalActivitySinceLastCheck { false };
@@ -121,6 +129,10 @@ class MessagePort final : public ActiveDOMObject, public EventTargetWithInlineDa
MessagePortIdentifier m_remoteIdentifier;

mutable std::atomic<unsigned> m_refCount { 1 };

#if !ASSERT_DISABLED
Ref<Thread> m_creationThread { Thread::current() };
#endif
};

} // namespace WebCore

0 comments on commit a2b9560

Please sign in to comment.