- Revision
- 226202
- Author
- beid...@apple.com
- Date
- 2017-12-20 14:47:20 -0800 (Wed, 20 Dec 2017)
Log Message
Assertion failure in MessagePort::contextDestroyed in http/tests/security/MessagePort/event-listener-context.html, usually attributed to later tests.
https://bugs.webkit.org/show_bug.cgi?id=94458
Reviewed by Chris Dumez.
Source/WebCore:
No new tests (Changed existing test to reliably crash before this change, and work after it)
There was already a glaring FIXME that said "MessagePorts should be ActiveDOMObjects"
It was right, and it fixes up this subtle lifetime issue.
* dom/MessagePort.cpp:
(WebCore::MessagePort::MessagePort):
(WebCore::MessagePort::hasPendingActivity const):
(WebCore::MessagePort::locallyEntangledPort const):
(WebCore::MessagePort::activeDOMObjectName const):
(WebCore::MessagePort::hasPendingActivity): Deleted.
(WebCore::MessagePort::locallyEntangledPort): Deleted.
* dom/MessagePort.h:
* dom/ScriptExecutionContext.cpp:
(WebCore::ScriptExecutionContext::~ScriptExecutionContext):
(WebCore::ScriptExecutionContext::stopActiveDOMObjects):
(WebCore::ScriptExecutionContext::hasPendingActivity const):
LayoutTests:
* fast/events/message-port-constructor-for-deleted-document-expected.txt:
* fast/events/message-port-constructor-for-deleted-document.html:
* fast/events/resources/copy-of-message-port-context-destroyed.html: Added.
* platform/mac/TestExpectations: Reenable the now-reliable and now-passing test.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (226201 => 226202)
--- trunk/LayoutTests/ChangeLog 2017-12-20 22:09:01 UTC (rev 226201)
+++ trunk/LayoutTests/ChangeLog 2017-12-20 22:47:20 UTC (rev 226202)
@@ -1,3 +1,15 @@
+2017-12-20 Brady Eidson <beid...@apple.com>
+
+ Assertion failure in MessagePort::contextDestroyed in http/tests/security/MessagePort/event-listener-context.html, usually attributed to later tests.
+ https://bugs.webkit.org/show_bug.cgi?id=94458
+
+ Reviewed by Chris Dumez.
+
+ * fast/events/message-port-constructor-for-deleted-document-expected.txt:
+ * fast/events/message-port-constructor-for-deleted-document.html:
+ * fast/events/resources/copy-of-message-port-context-destroyed.html: Added.
+ * platform/mac/TestExpectations: Reenable the now-reliable and now-passing test.
+
2017-12-20 Youenn Fablet <you...@apple.com>
LayoutTests/http/tests/workers/service/service-worker-cache-api.https.html is failing on most platforms
Modified: trunk/LayoutTests/fast/events/message-port-constructor-for-deleted-document-expected.txt (226201 => 226202)
--- trunk/LayoutTests/fast/events/message-port-constructor-for-deleted-document-expected.txt 2017-12-20 22:09:01 UTC (rev 226201)
+++ trunk/LayoutTests/fast/events/message-port-constructor-for-deleted-document-expected.txt 2017-12-20 22:47:20 UTC (rev 226202)
@@ -1,4 +1 @@
-Test that destroying a document doesn't cause a crash when executing MessageChannel constructor saved from its Window object.
-
Didn't crash: SUCCESS
-
Modified: trunk/LayoutTests/fast/events/message-port-constructor-for-deleted-document.html (226201 => 226202)
--- trunk/LayoutTests/fast/events/message-port-constructor-for-deleted-document.html 2017-12-20 22:09:01 UTC (rev 226201)
+++ trunk/LayoutTests/fast/events/message-port-constructor-for-deleted-document.html 2017-12-20 22:47:20 UTC (rev 226202)
@@ -44,10 +44,7 @@
} catch (ex) {
}
- log("Didn't crash: SUCCESS");
-
- if (window.testRunner)
- testRunner.notifyDone();
+ location.href='';
}
</script>
Added: trunk/LayoutTests/fast/events/resources/copy-of-message-port-context-destroyed.html (0 => 226202)
--- trunk/LayoutTests/fast/events/resources/copy-of-message-port-context-destroyed.html (rev 0)
+++ trunk/LayoutTests/fast/events/resources/copy-of-message-port-context-destroyed.html 2017-12-20 22:47:20 UTC (rev 226202)
@@ -0,0 +1,41 @@
+<script>
+if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+}
+
+var port;
+var gc_stuff = new Array();
+
+gc_and_crash = function() {
+ if (this.GCController)
+ GCController.collect();
+ else {
+ // V8 needs that many objects to run GC.
+ for(i = 0; i < 100000; i++) {
+ p = new Object();
+ gc_stuff.push(p);
+ gc_stuff.push(p + p);
+ }
+ }
+
+ // If the bug 43140 is regressed, this will crash, at least in v8-based ports.
+ port.start();
+
+ document.getElementById("log").innerText = "Didn't crash: SUCCESS";
+ if (window.testRunner)
+ testRunner.notifyDone();
+}
+
+function test() {
+ var iframe = document.getElementById("iframe");
+ var channel = new iframe.contentWindow.MessageChannel();
+ port = channel.port1;
+
+ iframe._onload_ = function() { gc_and_crash(); }
+ iframe.src = "" ;
+}
+</script>
+<body _onload_="test()">
+<pre id=log></pre>
+<iframe style="display:none" id=iframe></iframe>
\ No newline at end of file
Modified: trunk/LayoutTests/platform/mac/TestExpectations (226201 => 226202)
--- trunk/LayoutTests/platform/mac/TestExpectations 2017-12-20 22:09:01 UTC (rev 226201)
+++ trunk/LayoutTests/platform/mac/TestExpectations 2017-12-20 22:47:20 UTC (rev 226202)
@@ -541,9 +541,6 @@
webkit.org/b/27684 compositing/text-on-scaled-layer.html [ ImageOnlyFailure ]
webkit.org/b/27684 compositing/text-on-scaled-surface.html [ ImageOnlyFailure ]
-# Asserts in MessagePort::contextDestroyed, but the assert usually gets attributed to later tests.
-webkit.org/b/94458 fast/events/message-port-constructor-for-deleted-document.html
-
webkit.org/b/95501 http/tests/security/inactive-document-with-empty-security-origin.html [ Skip ]
# Rendering/Layout issue of CJK vertical text with some fonts.
Modified: trunk/Source/WebCore/ChangeLog (226201 => 226202)
--- trunk/Source/WebCore/ChangeLog 2017-12-20 22:09:01 UTC (rev 226201)
+++ trunk/Source/WebCore/ChangeLog 2017-12-20 22:47:20 UTC (rev 226202)
@@ -1,3 +1,30 @@
+2017-12-20 Brady Eidson <beid...@apple.com>
+
+ Assertion failure in MessagePort::contextDestroyed in http/tests/security/MessagePort/event-listener-context.html, usually attributed to later tests.
+ https://bugs.webkit.org/show_bug.cgi?id=94458
+
+ Reviewed by Chris Dumez.
+
+ No new tests (Changed existing test to reliably crash before this change, and work after it)
+
+ There was already a glaring FIXME that said "MessagePorts should be ActiveDOMObjects"
+
+ It was right, and it fixes up this subtle lifetime issue.
+
+ * dom/MessagePort.cpp:
+ (WebCore::MessagePort::MessagePort):
+ (WebCore::MessagePort::hasPendingActivity const):
+ (WebCore::MessagePort::locallyEntangledPort const):
+ (WebCore::MessagePort::activeDOMObjectName const):
+ (WebCore::MessagePort::hasPendingActivity): Deleted.
+ (WebCore::MessagePort::locallyEntangledPort): Deleted.
+ * dom/MessagePort.h:
+
+ * dom/ScriptExecutionContext.cpp:
+ (WebCore::ScriptExecutionContext::~ScriptExecutionContext):
+ (WebCore::ScriptExecutionContext::stopActiveDOMObjects):
+ (WebCore::ScriptExecutionContext::hasPendingActivity const):
+
2017-12-20 Youenn Fablet <you...@apple.com>
Do not search for service worker registration in case of non HTTP navigation loads
Modified: trunk/Source/WebCore/dom/MessagePort.cpp (226201 => 226202)
--- trunk/Source/WebCore/dom/MessagePort.cpp 2017-12-20 22:09:01 UTC (rev 226201)
+++ trunk/Source/WebCore/dom/MessagePort.cpp 2017-12-20 22:47:20 UTC (rev 226202)
@@ -35,9 +35,10 @@
namespace WebCore {
MessagePort::MessagePort(ScriptExecutionContext& scriptExecutionContext)
- : m_scriptExecutionContext(&scriptExecutionContext)
+ : ActiveDOMObject(&scriptExecutionContext)
{
m_scriptExecutionContext->createdMessagePort(*this);
+ suspendIfNeeded();
// Don't need to call processMessagePortMessagesSoon() here, because the port will not be opened until start() is invoked.
}
@@ -85,6 +86,9 @@
// We can't receive any messages or generate any events after this, so remove ourselves from the list of active ports.
ASSERT(m_scriptExecutionContext);
m_scriptExecutionContext->destroyedMessagePort(*this);
+ m_scriptExecutionContext->willDestroyActiveDOMObject(*this);
+ m_scriptExecutionContext->willDestroyDestructionObserver(*this);
+
m_scriptExecutionContext = nullptr;
return WTFMove(m_entangledChannel);
@@ -161,7 +165,7 @@
}
}
-bool MessagePort::hasPendingActivity()
+bool MessagePort::hasPendingActivity() const
{
// The spec says that entangled message ports should always be treated as if they have a strong reference.
// We'll also stipulate that the queue needs to be open (if the app drops its reference to the port before start()-ing it, then it's not really entangled as it's unreachable).
@@ -172,7 +176,7 @@
return false;
}
-MessagePort* MessagePort::locallyEntangledPort()
+MessagePort* MessagePort::locallyEntangledPort() const
{
return m_entangledChannel ? m_entangledChannel->locallyEntangledPort(m_scriptExecutionContext) : nullptr;
}
@@ -218,4 +222,14 @@
return EventTargetWithInlineData::addEventListener(eventType, WTFMove(listener), options);
}
+const char* MessagePort::activeDOMObjectName() const
+{
+ return "MessagePort";
+}
+
+bool MessagePort::canSuspendForDocumentSuspension() const
+{
+ return !hasPendingActivity() || (!m_started || m_closed);
+}
+
} // namespace WebCore
Modified: trunk/Source/WebCore/dom/MessagePort.h (226201 => 226202)
--- trunk/Source/WebCore/dom/MessagePort.h 2017-12-20 22:09:01 UTC (rev 226201)
+++ trunk/Source/WebCore/dom/MessagePort.h 2017-12-20 22:47:20 UTC (rev 226202)
@@ -26,6 +26,7 @@
#pragma once
+#include "ActiveDOMObject.h"
#include "EventTarget.h"
#include "ExceptionOr.h"
#include "MessagePortChannel.h"
@@ -40,7 +41,7 @@
class Frame;
-class MessagePort final : public RefCounted<MessagePort>, public EventTargetWithInlineData {
+class MessagePort final : public RefCounted<MessagePort>, public ActiveDOMObject, public EventTargetWithInlineData {
public:
static Ref<MessagePort> create(ScriptExecutionContext& scriptExecutionContext) { return adoptRef(*new MessagePort(scriptExecutionContext)); }
virtual ~MessagePort();
@@ -60,29 +61,31 @@
void messageAvailable();
bool started() const { return m_started; }
- void contextDestroyed();
-
- ScriptExecutionContext* scriptExecutionContext() const final { return m_scriptExecutionContext; }
-
void dispatchMessages();
- bool hasPendingActivity();
-
// 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
// of the remote port (since it may live cross-process) - those platforms may always return null.
- MessagePort* locallyEntangledPort();
+ MessagePort* locallyEntangledPort() const;
using RefCounted::ref;
using RefCounted::deref;
-private:
- explicit MessagePort(ScriptExecutionContext&);
+ // ActiveDOMObject
+ const char* activeDOMObjectName() const final;
+ bool canSuspendForDocumentSuspension() const final;
+ void contextDestroyed() final;
+ void stop() final { close(); }
+ bool hasPendingActivity() const final;
+ // EventTargetWithInlineData.
+ EventTargetInterface eventTargetInterface() const final { return MessagePortEventTargetInterfaceType; }
+ ScriptExecutionContext* scriptExecutionContext() const final { return ActiveDOMObject::scriptExecutionContext(); }
void refEventTarget() final { ref(); }
void derefEventTarget() final { deref(); }
- EventTargetInterface eventTargetInterface() const final { return MessagePortEventTargetInterfaceType; }
+private:
+ explicit MessagePort(ScriptExecutionContext&);
bool addEventListener(const AtomicString& eventType, Ref<EventListener>&&, const AddEventListenerOptions&) final;
@@ -97,7 +100,6 @@
std::unique_ptr<MessagePortChannel> m_entangledChannel;
bool m_started { false };
bool m_closed { false };
- ScriptExecutionContext* m_scriptExecutionContext;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/dom/ScriptExecutionContext.cpp (226201 => 226202)
--- trunk/Source/WebCore/dom/ScriptExecutionContext.cpp 2017-12-20 22:09:01 UTC (rev 226201)
+++ trunk/Source/WebCore/dom/ScriptExecutionContext.cpp 2017-12-20 22:47:20 UTC (rev 226202)
@@ -127,9 +127,6 @@
while (auto* destructionObserver = m_destructionObservers.takeAny())
destructionObserver->contextDestroyed();
- for (auto* messagePort : m_messagePorts)
- messagePort->contextDestroyed();
-
#if !ASSERT_DISABLED
m_inScriptExecutionContextDestructor = false;
#endif
@@ -309,11 +306,6 @@
}
m_activeDOMObjectAdditionForbidden = false;
-
- // FIXME: Make message ports be active DOM objects and let them implement stop instead
- // of having this separate mechanism just for them.
- for (auto* messagePort : m_messagePorts)
- messagePort->close();
}
void ScriptExecutionContext::suspendActiveDOMObjectIfNeeded(ActiveDOMObject& activeDOMObject)
@@ -518,11 +510,6 @@
return true;
}
- for (auto* messagePort : m_messagePorts) {
- if (messagePort->hasPendingActivity())
- return true;
- }
-
return false;
}
