Changeset 226202 in webkit


Ignore:
Timestamp:
Dec 20, 2017 2:47:20 PM (6 years ago)
Author:
beidson@apple.com
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.
Location:
trunk
Files:
1 added
8 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r226199 r226202  
     12017-12-20  Brady Eidson  <beidson@apple.com>
     2
     3        Assertion failure in MessagePort::contextDestroyed in http/tests/security/MessagePort/event-listener-context.html, usually attributed to later tests.
     4        https://bugs.webkit.org/show_bug.cgi?id=94458
     5
     6        Reviewed by Chris Dumez.
     7
     8        * fast/events/message-port-constructor-for-deleted-document-expected.txt:
     9        * fast/events/message-port-constructor-for-deleted-document.html:
     10        * fast/events/resources/copy-of-message-port-context-destroyed.html: Added.
     11        * platform/mac/TestExpectations: Reenable the now-reliable and now-passing test.
     12
    1132017-12-20  Youenn Fablet  <youenn@apple.com>
    214
  • trunk/LayoutTests/fast/events/message-port-constructor-for-deleted-document-expected.txt

    r129542 r226202  
    1 Test that destroying a document doesn't cause a crash when executing MessageChannel constructor saved from its Window object.
    2 
    31Didn't crash: SUCCESS
    4 
  • trunk/LayoutTests/fast/events/message-port-constructor-for-deleted-document.html

    r120792 r226202  
    4545    }
    4646
    47     log("Didn't crash: SUCCESS");
    48 
    49     if (window.testRunner)
    50         testRunner.notifyDone();
     47    location.href='resources/copy-of-message-port-context-destroyed.html';
    5148}
    5249
  • trunk/LayoutTests/platform/mac/TestExpectations

    r226189 r226202  
    541541webkit.org/b/27684 compositing/text-on-scaled-layer.html [ ImageOnlyFailure ]
    542542webkit.org/b/27684 compositing/text-on-scaled-surface.html [ ImageOnlyFailure ]
    543 
    544 # Asserts in MessagePort::contextDestroyed, but the assert usually gets attributed to later tests.
    545 webkit.org/b/94458 fast/events/message-port-constructor-for-deleted-document.html
    546543
    547544webkit.org/b/95501 http/tests/security/inactive-document-with-empty-security-origin.html [ Skip ]
  • trunk/Source/WebCore/ChangeLog

    r226200 r226202  
     12017-12-20  Brady Eidson  <beidson@apple.com>
     2
     3        Assertion failure in MessagePort::contextDestroyed in http/tests/security/MessagePort/event-listener-context.html, usually attributed to later tests.
     4        https://bugs.webkit.org/show_bug.cgi?id=94458
     5
     6        Reviewed by Chris Dumez.
     7
     8        No new tests (Changed existing test to reliably crash before this change, and work after it)
     9
     10        There was already a glaring FIXME that said "MessagePorts should be ActiveDOMObjects"
     11       
     12        It was right, and it fixes up this subtle lifetime issue.
     13       
     14        * dom/MessagePort.cpp:
     15        (WebCore::MessagePort::MessagePort):
     16        (WebCore::MessagePort::hasPendingActivity const):
     17        (WebCore::MessagePort::locallyEntangledPort const):
     18        (WebCore::MessagePort::activeDOMObjectName const):
     19        (WebCore::MessagePort::hasPendingActivity): Deleted.
     20        (WebCore::MessagePort::locallyEntangledPort): Deleted.
     21        * dom/MessagePort.h:
     22
     23        * dom/ScriptExecutionContext.cpp:
     24        (WebCore::ScriptExecutionContext::~ScriptExecutionContext):
     25        (WebCore::ScriptExecutionContext::stopActiveDOMObjects):
     26        (WebCore::ScriptExecutionContext::hasPendingActivity const):
     27
    1282017-12-20  Youenn Fablet  <youenn@apple.com>
    229
  • trunk/Source/WebCore/dom/MessagePort.cpp

    r219856 r226202  
    3636
    3737MessagePort::MessagePort(ScriptExecutionContext& scriptExecutionContext)
    38     : m_scriptExecutionContext(&scriptExecutionContext)
     38    : ActiveDOMObject(&scriptExecutionContext)
    3939{
    4040    m_scriptExecutionContext->createdMessagePort(*this);
     41    suspendIfNeeded();
    4142
    4243    // Don't need to call processMessagePortMessagesSoon() here, because the port will not be opened until start() is invoked.
     
    8687    ASSERT(m_scriptExecutionContext);
    8788    m_scriptExecutionContext->destroyedMessagePort(*this);
     89    m_scriptExecutionContext->willDestroyActiveDOMObject(*this);
     90    m_scriptExecutionContext->willDestroyDestructionObserver(*this);
     91
    8892    m_scriptExecutionContext = nullptr;
    8993
     
    162166}
    163167
    164 bool MessagePort::hasPendingActivity()
     168bool MessagePort::hasPendingActivity() const
    165169{
    166170    // The spec says that entangled message ports should always be treated as if they have a strong reference.
     
    173177}
    174178
    175 MessagePort* MessagePort::locallyEntangledPort()
     179MessagePort* MessagePort::locallyEntangledPort() const
    176180{
    177181    return m_entangledChannel ? m_entangledChannel->locallyEntangledPort(m_scriptExecutionContext) : nullptr;
     
    219223}
    220224
     225const char* MessagePort::activeDOMObjectName() const
     226{
     227    return "MessagePort";
     228}
     229
     230bool MessagePort::canSuspendForDocumentSuspension() const
     231{
     232    return !hasPendingActivity() || (!m_started || m_closed);
     233}
     234
    221235} // namespace WebCore
  • trunk/Source/WebCore/dom/MessagePort.h

    r224740 r226202  
    2727#pragma once
    2828
     29#include "ActiveDOMObject.h"
    2930#include "EventTarget.h"
    3031#include "ExceptionOr.h"
     
    4142class Frame;
    4243
    43 class MessagePort final : public RefCounted<MessagePort>, public EventTargetWithInlineData {
     44class MessagePort final : public RefCounted<MessagePort>, public ActiveDOMObject, public EventTargetWithInlineData {
    4445public:
    4546    static Ref<MessagePort> create(ScriptExecutionContext& scriptExecutionContext) { return adoptRef(*new MessagePort(scriptExecutionContext)); }
     
    6162    bool started() const { return m_started; }
    6263
    63     void contextDestroyed();
    64 
    65     ScriptExecutionContext* scriptExecutionContext() const final { return m_scriptExecutionContext; }
    66 
    6764    void dispatchMessages();
    68 
    69     bool hasPendingActivity();
    7065
    7166    // Returns null if there is no entangled port, or if the entangled port is run by a different thread.
    7267    // This is used solely to enable a GC optimization. Some platforms may not be able to determine ownership
    7368    // of the remote port (since it may live cross-process) - those platforms may always return null.
    74     MessagePort* locallyEntangledPort();
     69    MessagePort* locallyEntangledPort() const;
    7570
    7671    using RefCounted::ref;
    7772    using RefCounted::deref;
    7873
    79 private:
    80     explicit MessagePort(ScriptExecutionContext&);
     74    // ActiveDOMObject
     75    const char* activeDOMObjectName() const final;
     76    bool canSuspendForDocumentSuspension() const final;
     77    void contextDestroyed() final;
     78    void stop() final { close(); }
     79    bool hasPendingActivity() const final;
    8180
     81    // EventTargetWithInlineData.
     82    EventTargetInterface eventTargetInterface() const final { return MessagePortEventTargetInterfaceType; }
     83    ScriptExecutionContext* scriptExecutionContext() const final { return ActiveDOMObject::scriptExecutionContext(); }
    8284    void refEventTarget() final { ref(); }
    8385    void derefEventTarget() final { deref(); }
    8486
    85     EventTargetInterface eventTargetInterface() const final { return MessagePortEventTargetInterfaceType; }
     87private:
     88    explicit MessagePort(ScriptExecutionContext&);
    8689
    8790    bool addEventListener(const AtomicString& eventType, Ref<EventListener>&&, const AddEventListenerOptions&) final;
     
    98101    bool m_started { false };
    99102    bool m_closed { false };
    100     ScriptExecutionContext* m_scriptExecutionContext;
    101103};
    102104
  • trunk/Source/WebCore/dom/ScriptExecutionContext.cpp

    r226088 r226202  
    128128        destructionObserver->contextDestroyed();
    129129
    130     for (auto* messagePort : m_messagePorts)
    131         messagePort->contextDestroyed();
    132 
    133130#if !ASSERT_DISABLED
    134131    m_inScriptExecutionContextDestructor = false;
     
    310307
    311308    m_activeDOMObjectAdditionForbidden = false;
    312 
    313     // FIXME: Make message ports be active DOM objects and let them implement stop instead
    314     // of having this separate mechanism just for them.
    315     for (auto* messagePort : m_messagePorts)
    316         messagePort->close();
    317309}
    318310
     
    516508    for (auto* activeDOMObject : m_activeDOMObjects) {
    517509        if (activeDOMObject->hasPendingActivity())
    518             return true;
    519     }
    520 
    521     for (auto* messagePort : m_messagePorts) {
    522         if (messagePort->hasPendingActivity())
    523510            return true;
    524511    }
Note: See TracChangeset for help on using the changeset viewer.