<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[260576] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/260576">260576</a></dd>
<dt>Author</dt> <dd>cdumez@apple.com</dd>
<dt>Date</dt> <dd>2020-04-23 10:15:45 -0700 (Thu, 23 Apr 2020)</dd>
</dl>

<h3>Log Message</h3>
<pre>[ 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 Geoff Garen.

Source/WebCore:

Align garbage collection of Notification JS wrapper with the specification:
- https://notifications.spec.whatwg.org/#garbage-collection [1]

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:</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkLayoutTestsplatformmacwk2TestExpectations">trunk/LayoutTests/platform/mac-wk2/TestExpectations</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreModulesnotificationsNotificationcpp">trunk/Source/WebCore/Modules/notifications/Notification.cpp</a></li>
<li><a href="#trunkSourceWebCoreModulesnotificationsNotificationh">trunk/Source/WebCore/Modules/notifications/Notification.h</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (260575 => 260576)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog      2020-04-23 17:07:56 UTC (rev 260575)
+++ trunk/LayoutTests/ChangeLog 2020-04-23 17:15:45 UTC (rev 260576)
</span><span class="lines">@@ -1,3 +1,15 @@
</span><ins>+2020-04-23  Chris Dumez  <cdumez@apple.com>
+
+        [ 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 Geoff Garen.
+
+        Unskip test now that it is no longer flaky.
+
+        * platform/mac-wk2/TestExpectations:
+
</ins><span class="cx"> 2020-04-23  Diego Pino Garcia  <dpino@igalia.com>
</span><span class="cx"> 
</span><span class="cx">         [WPE] Gardening, update expectations after r259705
</span></span></pre></div>
<a id="trunkLayoutTestsplatformmacwk2TestExpectations"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/platform/mac-wk2/TestExpectations (260575 => 260576)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/platform/mac-wk2/TestExpectations      2020-04-23 17:07:56 UTC (rev 260575)
+++ trunk/LayoutTests/platform/mac-wk2/TestExpectations 2020-04-23 17:15:45 UTC (rev 260576)
</span><span class="lines">@@ -1028,8 +1028,6 @@
</span><span class="cx"> 
</span><span class="cx"> webkit.org/b/209295 [ Debug ] imported/w3c/web-platform-tests/service-workers/service-worker/respond-with-body-accessed-response.https.html  [ Pass Failure ]
</span><span class="cx"> 
</span><del>-webkit.org/b/209483 imported/w3c/web-platform-tests/notifications/event-onclose.html [ Pass Failure ]
-
</del><span class="cx"> webkit.org/b/209503 [ Debug ] http/tests/referrer-policy-anchor/origin/cross-origin-http.https.html [ Pass Crash ]
</span><span class="cx"> 
</span><span class="cx"> webkit.org/b/209564 svg/as-image/svg-image-with-data-uri-background.html [ Pass ImageOnlyFailure ]
</span></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (260575 => 260576)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog   2020-04-23 17:07:56 UTC (rev 260575)
+++ trunk/Source/WebCore/ChangeLog      2020-04-23 17:15:45 UTC (rev 260576)
</span><span class="lines">@@ -1,3 +1,40 @@
</span><ins>+2020-04-23  Chris Dumez  <cdumez@apple.com>
+
+        [ 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 Geoff Garen.
+
+        Align garbage collection of Notification JS wrapper with the specification:
+        - https://notifications.spec.whatwg.org/#garbage-collection [1]
+
+        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:
+
</ins><span class="cx"> 2020-04-23  Andres Gonzalez  <andresg_22@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Correction for patch 397001.
</span></span></pre></div>
<a id="trunkSourceWebCoreModulesnotificationsNotificationcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/Modules/notifications/Notification.cpp (260575 => 260576)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/Modules/notifications/Notification.cpp      2020-04-23 17:07:56 UTC (rev 260575)
+++ trunk/Source/WebCore/Modules/notifications/Notification.cpp 2020-04-23 17:15:45 UTC (rev 260576)
</span><span class="lines">@@ -53,6 +53,7 @@
</span><span class="cx"> {
</span><span class="cx">     auto notification = adoptRef(*new Notification(context, title, options));
</span><span class="cx">     notification->suspendIfNeeded();
</span><ins>+    notification->showSoon();
</ins><span class="cx">     return notification;
</span><span class="cx"> }
</span><span class="cx"> 
</span><span class="lines">@@ -64,7 +65,6 @@
</span><span class="cx">     , m_body(options.body)
</span><span class="cx">     , m_tag(options.tag)
</span><span class="cx">     , m_state(Idle)
</span><del>-    , m_showNotificationTimer(&document, *this, &Notification::show)
</del><span class="cx"> {
</span><span class="cx">     if (!options.icon.isEmpty()) {
</span><span class="cx">         auto iconURL = document.completeURL(options.icon);
</span><span class="lines">@@ -71,13 +71,18 @@
</span><span class="cx">         if (iconURL.isValid())
</span><span class="cx">             m_icon = iconURL;
</span><span class="cx">     }
</span><del>-
-    m_showNotificationTimer.startOneShot(0_s);
-    m_showNotificationTimer.suspendIfNeeded();
</del><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> Notification::~Notification() = default;
</span><span class="cx"> 
</span><ins>+
+void Notification::showSoon()
+{
+    queueTaskKeepingObjectAlive(*this, TaskSource::UserInteraction, [this] {
+        show();
+    });
+}
+
</ins><span class="cx"> void Notification::show()
</span><span class="cx"> {
</span><span class="cx">     // prevent double-showing
</span><span class="lines">@@ -94,10 +99,8 @@
</span><span class="cx">         dispatchErrorEvent();
</span><span class="cx">         return;
</span><span class="cx">     }
</span><del>-    if (client.show(this)) {
</del><ins>+    if (client.show(this))
</ins><span class="cx">         m_state = Showing;
</span><del>-        setPendingActivity(*this);
-    }
</del><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void Notification::close()
</span><span class="lines">@@ -143,28 +146,16 @@
</span><span class="cx">     if (m_state == Closed)
</span><span class="cx">         return;
</span><span class="cx">     m_state = Closed;
</span><del>-    unsetPendingActivity(*this);
</del><span class="cx"> }
</span><span class="cx"> 
</span><del>-void Notification::queueTask(Function<void()>&& task)
-{
-    auto* document = this->document();
-    if (!document)
-        return;
-
-    document->eventLoop().queueTask(TaskSource::UserInteraction, WTFMove(task));
-}
-
</del><span class="cx"> void Notification::dispatchShowEvent()
</span><span class="cx"> {
</span><del>-    queueTask([this, pendingActivity = makePendingActivity(*this)] {
-        dispatchEvent(Event::create(eventNames().showEvent, Event::CanBubble::No, Event::IsCancelable::No));
-    });
</del><ins>+    queueTaskToDispatchEvent(*this, TaskSource::UserInteraction, Event::create(eventNames().showEvent, Event::CanBubble::No, Event::IsCancelable::No));
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void Notification::dispatchClickEvent()
</span><span class="cx"> {
</span><del>-    queueTask([this, pendingActivity = makePendingActivity(*this)] {
</del><ins>+    queueTaskKeepingObjectAlive(*this, TaskSource::UserInteraction, [this] {
</ins><span class="cx">         WindowFocusAllowedIndicator windowFocusAllowed;
</span><span class="cx">         dispatchEvent(Event::create(eventNames().clickEvent, Event::CanBubble::No, Event::IsCancelable::No));
</span><span class="cx">     });
</span><span class="lines">@@ -172,17 +163,13 @@
</span><span class="cx"> 
</span><span class="cx"> void Notification::dispatchCloseEvent()
</span><span class="cx"> {
</span><del>-    queueTask([this, pendingActivity = makePendingActivity(*this)] {
-        dispatchEvent(Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No));
-    });
</del><ins>+    queueTaskToDispatchEvent(*this, TaskSource::UserInteraction, Event::create(eventNames().closeEvent, Event::CanBubble::No, Event::IsCancelable::No));
</ins><span class="cx">     finalize();
</span><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> void Notification::dispatchErrorEvent()
</span><span class="cx"> {
</span><del>-    queueTask([this, pendingActivity = makePendingActivity(*this)] {
-        dispatchEvent(Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));
-    });
</del><ins>+    queueTaskToDispatchEvent(*this, TaskSource::UserInteraction, Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));
</ins><span class="cx"> }
</span><span class="cx"> 
</span><span class="cx"> auto Notification::permission(Document& document) -> Permission
</span><span class="lines">@@ -215,6 +202,22 @@
</span><span class="cx">     NotificationController::from(page)->client().requestPermission(&document, WTFMove(callback));
</span><span class="cx"> }
</span><span class="cx"> 
</span><ins>+void Notification::eventListenersDidChange()
+{
+    m_hasRelevantEventListener = hasEventListeners(eventNames().clickEvent)
+        || hasEventListeners(eventNames().closeEvent)
+        || hasEventListeners(eventNames().errorEvent)
+        || hasEventListeners(eventNames().showEvent);
+}
+
+// A Notification object must not be garbage collected while the list of notifications contains its corresponding
+// notification and it has an event listener whose type is click, show, close, or error.
+// See https://notifications.spec.whatwg.org/#garbage-collection
+bool Notification::virtualHasPendingActivity() const
+{
+    return m_state == Showing && m_hasRelevantEventListener;
+}
+
</ins><span class="cx"> } // namespace WebCore
</span><span class="cx"> 
</span><span class="cx"> #endif // ENABLE(NOTIFICATIONS)
</span></span></pre></div>
<a id="trunkSourceWebCoreModulesnotificationsNotificationh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/Modules/notifications/Notification.h (260575 => 260576)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/Modules/notifications/Notification.h        2020-04-23 17:07:56 UTC (rev 260575)
+++ trunk/Source/WebCore/Modules/notifications/Notification.h   2020-04-23 17:15:45 UTC (rev 260576)
</span><span class="lines">@@ -37,7 +37,6 @@
</span><span class="cx"> #include "EventTarget.h"
</span><span class="cx"> #include "NotificationDirection.h"
</span><span class="cx"> #include "NotificationPermission.h"
</span><del>-#include "SuspendableTimer.h"
</del><span class="cx"> #include <wtf/URL.h>
</span><span class="cx"> #include "WritingMode.h"
</span><span class="cx"> 
</span><span class="lines">@@ -96,15 +95,18 @@
</span><span class="cx">     Document* document() const;
</span><span class="cx">     EventTargetInterface eventTargetInterface() const final { return NotificationEventTargetInterfaceType; }
</span><span class="cx"> 
</span><del>-    void queueTask(Function<void()>&&);
</del><ins>+    void showSoon();
</ins><span class="cx"> 
</span><span class="cx">     // ActiveDOMObject
</span><span class="cx">     const char* activeDOMObjectName() const final;
</span><span class="cx">     void suspend(ReasonForSuspension);
</span><span class="cx">     void stop() final;
</span><ins>+    bool virtualHasPendingActivity() const final;
</ins><span class="cx"> 
</span><ins>+    // EventTarget
</ins><span class="cx">     void refEventTarget() final { ref(); }
</span><span class="cx">     void derefEventTarget() final { deref(); }
</span><ins>+    void eventListenersDidChange() final;
</ins><span class="cx"> 
</span><span class="cx">     String m_title;
</span><span class="cx">     Direction m_direction;
</span><span class="lines">@@ -115,8 +117,7 @@
</span><span class="cx"> 
</span><span class="cx">     enum State { Idle, Showing, Closed };
</span><span class="cx">     State m_state { Idle };
</span><del>-
-    SuspendableTimer m_showNotificationTimer;
</del><ins>+    bool m_hasRelevantEventListener { false };
</ins><span class="cx"> };
</span><span class="cx"> 
</span><span class="cx"> } // namespace WebCore
</span></span></pre>
</div>
</div>

</body>
</html>