Changeset 260535 in webkit


Ignore:
Timestamp:
Apr 22, 2020 3:45:00 PM (4 years ago)
Author:
Chris Dumez
Message:

[ Mac wk2 ] imported/w3c/web-platform-tests/notifications/event-onclose.html is flaky failing.
https://bugs.webkit.org/show_bug.cgi?id=209483
<rdar://problem/60830377>

Reviewed by Geoffrey Garen.

Source/WebCore:

Align garbage collection of Notification JS wrapper with the specification:

In particular, the following changes were made:

  1. Instead of using the legacy setPendingActivity() / unsetPendingActivity(), override ActiveDOMObject::virtualHasPendingActivity() to implement the behavior documented in the specification.
  2. Keep the wrapper alive as long as the notification is showing and as long as there are relevant event listeners, as per [1]. Previously, we failed to check for event listeners, which was suboptimal.
  3. Update the constructor to queue a task on the event loop in order to show the notification asynchronously, instead of relying on a SuspendableTimer for this purpose. Previously, the JS wrapper could get collected between construction and the notification getting shown, which was leading to the test flakiness.

No new tests, unskipped existing test.

  • Modules/notifications/Notification.cpp:

(WebCore::Notification::Notification):
(WebCore::Notification::show):
(WebCore::Notification::finalize):
(WebCore::Notification::dispatchShowEvent):
(WebCore::Notification::dispatchClickEvent):
(WebCore::Notification::dispatchCloseEvent):
(WebCore::Notification::dispatchErrorEvent):
(WebCore::Notification::eventListenersDidChange):
(WebCore::Notification::virtualHasPendingActivity const):

  • Modules/notifications/Notification.h:

LayoutTests:

Unskip test now that it is no longer flaky.

  • platform/mac-wk2/TestExpectations:
Location:
trunk
Files:
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r260533 r260535  
     12020-04-22  Chris Dumez  <cdumez@apple.com>
     2
     3        [ Mac wk2 ] imported/w3c/web-platform-tests/notifications/event-onclose.html is flaky failing.
     4        https://bugs.webkit.org/show_bug.cgi?id=209483
     5        <rdar://problem/60830377>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        Unskip test now that it is no longer flaky.
     10
     11        * platform/mac-wk2/TestExpectations:
     12
    1132020-04-22  Myles C. Maxfield  <mmaxfield@apple.com>
    214
  • trunk/LayoutTests/platform/mac-wk2/TestExpectations

    r260471 r260535  
    10291029webkit.org/b/209295 [ Debug ] imported/w3c/web-platform-tests/service-workers/service-worker/respond-with-body-accessed-response.https.html  [ Pass Failure ]
    10301030
    1031 webkit.org/b/209483 imported/w3c/web-platform-tests/notifications/event-onclose.html [ Pass Failure ]
    1032 
    10331031webkit.org/b/209503 [ Debug ] http/tests/referrer-policy-anchor/origin/cross-origin-http.https.html [ Pass Crash ]
    10341032
  • trunk/Source/WebCore/ChangeLog

    r260534 r260535  
     12020-04-22  Chris Dumez  <cdumez@apple.com>
     2
     3        [ Mac wk2 ] imported/w3c/web-platform-tests/notifications/event-onclose.html is flaky failing.
     4        https://bugs.webkit.org/show_bug.cgi?id=209483
     5        <rdar://problem/60830377>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        Align garbage collection of Notification JS wrapper with the specification:
     10        - https://notifications.spec.whatwg.org/#garbage-collection [1]
     11
     12        In particular, the following changes were made:
     13        1. Instead of using the legacy setPendingActivity() / unsetPendingActivity(), override
     14           ActiveDOMObject::virtualHasPendingActivity() to implement the behavior documented
     15           in the specification.
     16        2. Keep the wrapper alive as long as the notification is showing and as long as there
     17           are relevant event listeners, as per [1]. Previously, we failed to check for event
     18           listeners, which was suboptimal.
     19        3. Update the constructor to queue a task on the event loop in order to show the
     20           notification asynchronously, instead of relying on a SuspendableTimer for this
     21           purpose. Previously, the JS wrapper could get collected between construction and
     22           the notification getting shown, which was leading to the test flakiness.
     23
     24        No new tests, unskipped existing test.
     25
     26        * Modules/notifications/Notification.cpp:
     27        (WebCore::Notification::Notification):
     28        (WebCore::Notification::show):
     29        (WebCore::Notification::finalize):
     30        (WebCore::Notification::dispatchShowEvent):
     31        (WebCore::Notification::dispatchClickEvent):
     32        (WebCore::Notification::dispatchCloseEvent):
     33        (WebCore::Notification::dispatchErrorEvent):
     34        (WebCore::Notification::eventListenersDidChange):
     35        (WebCore::Notification::virtualHasPendingActivity const):
     36        * Modules/notifications/Notification.h:
     37
    1382020-04-22  Don Olmstead  <don.olmstead@sony.com>
    239
  • trunk/Source/WebCore/Modules/notifications/Notification.cpp

    r255680 r260535  
    6565    , m_tag(options.tag)
    6666    , m_state(Idle)
    67     , m_showNotificationTimer(&document, *this, &Notification::show)
    6867{
    6968    if (!options.icon.isEmpty()) {
     
    7372    }
    7473
    75     m_showNotificationTimer.startOneShot(0_s);
    76     m_showNotificationTimer.suspendIfNeeded();
     74    queueTaskKeepingObjectAlive(*this, TaskSource::UserInteraction, [this] {
     75        show();
     76    });
    7777}
    7878
     
    9595        return;
    9696    }
    97     if (client.show(this)) {
     97    if (client.show(this))
    9898        m_state = Showing;
    99         setPendingActivity(*this);
    100     }
    10199}
    102100
     
    144142        return;
    145143    m_state = Closed;
    146     unsetPendingActivity(*this);
    147 }
    148 
    149 void Notification::queueTask(Function<void()>&& task)
    150 {
    151     auto* document = this->document();
    152     if (!document)
    153         return;
    154 
    155     document->eventLoop().queueTask(TaskSource::UserInteraction, WTFMove(task));
    156144}
    157145
    158146void Notification::dispatchShowEvent()
    159147{
    160     queueTask([this, pendingActivity = makePendingActivity(*this)] {
    161         dispatchEvent(Event::create(eventNames().showEvent, Event::CanBubble::No, Event::IsCancelable::No));
    162     });
     148    queueTaskToDispatchEvent(*this, TaskSource::UserInteraction, Event::create(eventNames().showEvent, Event::CanBubble::No, Event::IsCancelable::No));
    163149}
    164150
    165151void Notification::dispatchClickEvent()
    166152{
    167     queueTask([this, pendingActivity = makePendingActivity(*this)] {
     153    queueTaskKeepingObjectAlive(*this, TaskSource::UserInteraction, [this] {
    168154        WindowFocusAllowedIndicator windowFocusAllowed;
    169155        dispatchEvent(Event::create(eventNames().clickEvent, Event::CanBubble::No, Event::IsCancelable::No));
     
    173159void Notification::dispatchCloseEvent()
    174160{
    175     queueTask([this, pendingActivity = makePendingActivity(*this)] {
    176         dispatchEvent(Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No));
    177     });
     161    queueTaskToDispatchEvent(*this, TaskSource::UserInteraction, Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No));
    178162    finalize();
    179163}
     
    181165void Notification::dispatchErrorEvent()
    182166{
    183     queueTask([this, pendingActivity = makePendingActivity(*this)] {
    184         dispatchEvent(Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));
    185     });
     167    queueTaskToDispatchEvent(*this, TaskSource::UserInteraction, Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));
    186168}
    187169
     
    216198}
    217199
     200void Notification::eventListenersDidChange()
     201{
     202    m_hasRelevantEventListener = hasEventListeners(eventNames().clickEvent)
     203        || hasEventListeners(eventNames().closeEvent)
     204        || hasEventListeners(eventNames().errorEvent)
     205        || hasEventListeners(eventNames().showEvent);
     206}
     207
     208// A Notification object must not be garbage collected while the list of notifications contains its corresponding
     209// notification and it has an event listener whose type is click, show, close, or error.
     210// See https://notifications.spec.whatwg.org/#garbage-collection
     211bool Notification::virtualHasPendingActivity() const
     212{
     213    return m_state == Showing && m_hasRelevantEventListener;
     214}
     215
    218216} // namespace WebCore
    219217
  • trunk/Source/WebCore/Modules/notifications/Notification.h

    r251528 r260535  
    3838#include "NotificationDirection.h"
    3939#include "NotificationPermission.h"
    40 #include "SuspendableTimer.h"
    4140#include <wtf/URL.h>
    4241#include "WritingMode.h"
     
    9796    EventTargetInterface eventTargetInterface() const final { return NotificationEventTargetInterfaceType; }
    9897
    99     void queueTask(Function<void()>&&);
    100 
    10198    // ActiveDOMObject
    10299    const char* activeDOMObjectName() const final;
    103100    void suspend(ReasonForSuspension);
    104101    void stop() final;
     102    bool virtualHasPendingActivity() const final;
    105103
     104    // EventTarget
    106105    void refEventTarget() final { ref(); }
    107106    void derefEventTarget() final { deref(); }
     107    void eventListenersDidChange() final;
    108108
    109109    String m_title;
     
    116116    enum State { Idle, Showing, Closed };
    117117    State m_state { Idle };
    118 
    119     SuspendableTimer m_showNotificationTimer;
     118    bool m_hasRelevantEventListener { false };
    120119};
    121120
Note: See TracChangeset for help on using the changeset viewer.