Diff
Modified: trunk/LayoutTests/ChangeLog (229627 => 229628)
--- trunk/LayoutTests/ChangeLog 2018-03-15 17:35:29 UTC (rev 229627)
+++ trunk/LayoutTests/ChangeLog 2018-03-15 18:21:10 UTC (rev 229628)
@@ -1,3 +1,15 @@
+2018-03-15 Youenn Fablet <[email protected]>
+
+ 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 <[email protected]>
[GTK][WPE] Enable service workers
Modified: trunk/LayoutTests/TestExpectations (229627 => 229628)
--- trunk/LayoutTests/TestExpectations 2018-03-15 17:35:29 UTC (rev 229627)
+++ trunk/LayoutTests/TestExpectations 2018-03-15 18:21:10 UTC (rev 229628)
@@ -211,6 +211,8 @@
# 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 ]
Added: trunk/LayoutTests/http/tests/workers/worker-messageport-2-expected.txt (0 => 229628)
--- trunk/LayoutTests/http/tests/workers/worker-messageport-2-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/workers/worker-messageport-2-expected.txt 2018-03-15 18:21:10 UTC (rev 229628)
@@ -0,0 +1 @@
+PASS
Added: trunk/LayoutTests/http/tests/workers/worker-messageport-2.html (0 => 229628)
--- trunk/LayoutTests/http/tests/workers/worker-messageport-2.html (rev 0)
+++ trunk/LayoutTests/http/tests/workers/worker-messageport-2.html 2018-03-15 18:21:10 UTC (rev 229628)
@@ -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>
Modified: trunk/Source/WebCore/ChangeLog (229627 => 229628)
--- trunk/Source/WebCore/ChangeLog 2018-03-15 17:35:29 UTC (rev 229627)
+++ trunk/Source/WebCore/ChangeLog 2018-03-15 18:21:10 UTC (rev 229628)
@@ -1,3 +1,25 @@
+2018-03-15 Youenn Fablet <[email protected]>
+
+ 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 <[email protected]>
Adopt new AVURLAssetUseClientURLLoadingExclusively AVURLAsset creation option.
Modified: trunk/Source/WebCore/dom/MessagePort.cpp (229627 => 229628)
--- trunk/Source/WebCore/dom/MessagePort.cpp 2018-03-15 17:35:29 UTC (rev 229627)
+++ trunk/Source/WebCore/dom/MessagePort.cpp 2018-03-15 18:21:10 UTC (rev 229628)
@@ -34,6 +34,7 @@
#include "MessagePortChannelProvider.h"
#include "MessageWithMessagePorts.h"
#include "WorkerGlobalScope.h"
+#include "WorkerThread.h"
namespace WebCore {
@@ -108,6 +109,8 @@
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 @@
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 @@
}
};
- 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 @@
// 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;
+ RefPtr<WorkerThread> workerThread;
+ if (is<WorkerGlobalScope>(*m_scriptExecutionContext))
+ workerThread = &downcast<WorkerGlobalScope>(*m_scriptExecutionContext).thread();
- if (hasActivity == MessagePortChannelProvider::HasActivity::No && !hasHadLocalActivity)
- m_isRemoteEligibleForGC = true;
+ MessagePortChannelProvider::singleton().checkRemotePortForActivity(m_remoteIdentifier, [weakThis = makeWeakPtr(const_cast<MessagePort*>(this)), workerThread = WTFMove(workerThread)](MessagePortChannelProvider::HasActivity hasActivity) mutable {
- if (hasActivity == MessagePortChannelProvider::HasActivity::Yes)
- m_isRemoteEligibleForGC = false;
-
- m_isAskingRemoteAboutGC = false;
- };
-
-
- if (!m_scriptExecutionContext)
+ ASSERT(isMainThread());
+ if (!workerThread) {
+ if (weakThis)
+ weakThis->updateActivity(hasActivity);
return;
-
- if (m_scriptExecutionContext->isContextThread()) {
- innerHandler(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;
}
Modified: trunk/Source/WebCore/dom/MessagePort.h (229627 => 229628)
--- trunk/Source/WebCore/dom/MessagePort.h 2018-03-15 17:35:29 UTC (rev 229627)
+++ trunk/Source/WebCore/dom/MessagePort.h 2018-03-15 18:21:10 UTC (rev 229628)
@@ -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 @@
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 @@
// 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 @@
MessagePortIdentifier m_remoteIdentifier;
mutable std::atomic<unsigned> m_refCount { 1 };
+
+#if !ASSERT_DISABLED
+ Ref<Thread> m_creationThread { Thread::current() };
+#endif
};
} // namespace WebCore
