Assertion failure in MessagePort::contextDestroyed in http/tests/security/MessagePort...
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Dec 2017 22:47:20 +0000 (22:47 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Dec 2017 22:47:20 +0000 (22:47 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226202 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/events/message-port-constructor-for-deleted-document-expected.txt
LayoutTests/fast/events/message-port-constructor-for-deleted-document.html
LayoutTests/fast/events/resources/copy-of-message-port-context-destroyed.html [new file with mode: 0644]
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/dom/MessagePort.cpp
Source/WebCore/dom/MessagePort.h
Source/WebCore/dom/ScriptExecutionContext.cpp

index bc2b0a6..6d65e96 100644 (file)
@@ -1,3 +1,15 @@
+2017-12-20  Brady Eidson  <beidson@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  <youenn@apple.com>
 
         LayoutTests/http/tests/workers/service/service-worker-cache-api.https.html is failing on most platforms
index 301266a..b66875d 100644 (file)
@@ -44,10 +44,7 @@ function test2()
     } catch (ex) {
     }
 
-    log("Didn't crash: SUCCESS");
-
-    if (window.testRunner)
-        testRunner.notifyDone();
+    location.href='resources/copy-of-message-port-context-destroyed.html';
 }
 
 </script>
diff --git a/LayoutTests/fast/events/resources/copy-of-message-port-context-destroyed.html b/LayoutTests/fast/events/resources/copy-of-message-port-context-destroyed.html
new file mode 100644 (file)
index 0000000..28a037a
--- /dev/null
@@ -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 = "data:text/html,<body>Hello!" ;
+}
+</script>
+<body onload="test()">
+<pre id=log></pre>
+<iframe style="display:none" id=iframe></iframe>
\ No newline at end of file
index e9e951d..23cd335 100644 (file)
@@ -541,9 +541,6 @@ fast/repaint/overflow-scroll-touch-repaint.html [ Skip ]
 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.
index fecffb8..1d11304 100644 (file)
@@ -1,3 +1,30 @@
+2017-12-20  Brady Eidson  <beidson@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  <youenn@apple.com>
 
         Do not search for service worker registration in case of non HTTP navigation loads
index 1e6316e..585bdb1 100644 (file)
 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 @@ std::unique_ptr<MessagePortChannel> MessagePort::disentangle()
     // 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 @@ void MessagePort::dispatchMessages()
     }
 }
 
-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 @@ bool MessagePort::hasPendingActivity()
     return false;
 }
 
-MessagePort* MessagePort::locallyEntangledPort()
+MessagePort* MessagePort::locallyEntangledPort() const
 {
     return m_entangledChannel ? m_entangledChannel->locallyEntangledPort(m_scriptExecutionContext) : nullptr;
 }
@@ -218,4 +222,14 @@ bool MessagePort::addEventListener(const AtomicString& eventType, Ref<EventListe
     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
index 660ca51..ffb28c5 100644 (file)
@@ -26,6 +26,7 @@
 
 #pragma once
 
+#include "ActiveDOMObject.h"
 #include "EventTarget.h"
 #include "ExceptionOr.h"
 #include "MessagePortChannel.h"
@@ -40,7 +41,7 @@ namespace WebCore {
 
 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 @@ public:
     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 @@ private:
     std::unique_ptr<MessagePortChannel> m_entangledChannel;
     bool m_started { false };
     bool m_closed { false };
-    ScriptExecutionContext* m_scriptExecutionContext;
 };
 
 } // namespace WebCore
index a8c90be..726f502 100644 (file)
@@ -127,9 +127,6 @@ ScriptExecutionContext::~ScriptExecutionContext()
     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 @@ void ScriptExecutionContext::stopActiveDOMObjects()
     }
 
     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 @@ bool ScriptExecutionContext::hasPendingActivity() const
             return true;
     }
 
-    for (auto* messagePort : m_messagePorts) {
-        if (messagePort->hasPendingActivity())
-            return true;
-    }
-
     return false;
 }