Changeset 190593 in webkit

Timestamp:

Oct 5, 2015 4:45:56 PM (9 years ago)

Author:

mmaxfield@apple.com

Message:

REGRESSION(189668?): http/tests/notifications/events.html flakily asserts or times out
​https://bugs.webkit.org/show_bug.cgi?id=149218

Reviewed by Alexey Proskuryakov.

Tools:

Because of r189668, WebKitTestRunner now tears down and recreates its WKNotificationManagerRef
when the TestOptions change. Previously, WebNotificationProvider only could handle a single
WKNotificationManagerRef. Because the ower of the WKNotificationManagerRef is reference counted,
and AppKit internally retains some objects which end up retaining the WKNotificationManagerRef,
the old WKNotificationManager may not be destroyed before the new one is created. Therefore,
WebNotificationProvider must be updated to appropriately handle multiple
WKNotificationManagerRefs in flight at the same time.

• WebKitTestRunner/WebNotificationProvider.cpp:

(WTR::WebNotificationProvider::~WebNotificationProvider):
(WTR::WebNotificationProvider::showWebNotification):
(WTR::WebNotificationProvider::closeWebNotification):
(WTR::WebNotificationProvider::addNotificationManager):
(WTR::WebNotificationProvider::removeNotificationManager):
(WTR::WebNotificationProvider::simulateWebNotificationClick):
(WTR::WebNotificationProvider::reset):

• WebKitTestRunner/WebNotificationProvider.h:

LayoutTests:

• TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/WebKitTestRunner/WebNotificationProvider.cpp (modified) (3 diffs)
• Tools/WebKitTestRunner/WebNotificationProvider.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2015-10-05  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        REGRESSION(189668?): http/tests/notifications/events.html flakily asserts or times out
+        https://bugs.webkit.org/show_bug.cgi?id=149218
+
+        Reviewed by Alexey Proskuryakov.
+
+        * TestExpectations:
+
 2015-10-05  Dean Jackson  <dino@apple.com>
 

trunk/LayoutTests/TestExpectations

@@ -677 +677 @@
 
 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-overrides.html [ Failure Pass ]
-
-webkit.org/b/149218 http/tests/notifications/events.html [ Pass Crash Timeout ]

trunk/Tools/ChangeLog

@@ +1 @@
+2015-10-05  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        REGRESSION(189668?): http/tests/notifications/events.html flakily asserts or times out
+        https://bugs.webkit.org/show_bug.cgi?id=149218
+
+        Reviewed by Alexey Proskuryakov.
+
+        Because of r189668, WebKitTestRunner now tears down and recreates its WKNotificationManagerRef
+        when the TestOptions change. Previously, WebNotificationProvider only could handle a single
+        WKNotificationManagerRef. Because the ower of the WKNotificationManagerRef is reference counted,
+        and AppKit internally retains some objects which end up retaining the WKNotificationManagerRef,
+        the old WKNotificationManager may not be destroyed before the new one is created. Therefore,
+        WebNotificationProvider must be updated to appropriately handle multiple
+        WKNotificationManagerRefs in flight at the same time.
+
+        * WebKitTestRunner/WebNotificationProvider.cpp:
+        (WTR::WebNotificationProvider::~WebNotificationProvider):
+        (WTR::WebNotificationProvider::showWebNotification):
+        (WTR::WebNotificationProvider::closeWebNotification):
+        (WTR::WebNotificationProvider::addNotificationManager):
+        (WTR::WebNotificationProvider::removeNotificationManager):
+        (WTR::WebNotificationProvider::simulateWebNotificationClick):
+        (WTR::WebNotificationProvider::reset):
+        * WebKitTestRunner/WebNotificationProvider.h:
+
 2015-10-05  Daniel Bates  <dabates@apple.com>
 

trunk/Tools/WebKitTestRunner/WebNotificationProvider.cpp

@@ -66 +66 @@
 WebNotificationProvider::~WebNotificationProvider()
 {
-    if (m_currentNotificationManager)
-        WKNotificationManagerSetProvider(m_currentNotificationManager.get(), nullptr);
+    for (auto& manager : m_ownedNotifications)
+        WKNotificationManagerSetProvider(manager.key.get(), nullptr);
 }
 
@@ -85 +85 @@
 }
 
-void WebNotificationProvider::showWebNotification(WKPageRef, WKNotificationRef notification)
+void WebNotificationProvider::showWebNotification(WKPageRef page, WKNotificationRef notification)
 {
-    if (!m_currentNotificationManager)
-        return;
+    auto context = WKPageGetContext(page);
+    auto notificationManager = WKContextGetNotificationManager(context);
+    uint64_t id = WKNotificationGetID(notification);
 
-    uint64_t id = WKNotificationGetID(notification);
-    ASSERT(!m_shownNotifications.contains(id));
-    m_shownNotifications.add(id);
+    ASSERT(m_ownedNotifications.contains(notificationManager));
+    auto addResult = m_ownedNotifications.find(notificationManager)->value.add(id);
+    ASSERT_UNUSED(addResult, addResult.isNewEntry);
+    auto addResult2 = m_owningManager.set(id, notificationManager);
+    ASSERT_UNUSED(addResult2, addResult2.isNewEntry);
 
-    WKNotificationManagerProviderDidShowNotification(m_currentNotificationManager.get(), WKNotificationGetID(notification));
+    WKNotificationManagerProviderDidShowNotification(notificationManager, id);
 }
 
 void WebNotificationProvider::closeWebNotification(WKNotificationRef notification)
 {
-    if (!m_currentNotificationManager)
-        return;
+    uint64_t id = WKNotificationGetID(notification);
+    ASSERT(m_owningManager.contains(id));
+    auto notificationManager = m_owningManager.get(id);
 
-    uint64_t id = WKNotificationGetID(notification);
+    ASSERT(m_ownedNotifications.contains(notificationManager));
+    bool success = m_ownedNotifications.find(notificationManager)->value.remove(id);
+    ASSERT_UNUSED(success, success);
+    m_owningManager.remove(id);
+
     WKRetainPtr<WKUInt64Ref> wkID = WKUInt64Create(id);
     WKRetainPtr<WKMutableArrayRef> array(AdoptWK, WKMutableArrayCreate());
     WKArrayAppendItem(array.get(), wkID.get());
-    m_shownNotifications.remove(id);
-    WKNotificationManagerProviderDidCloseNotifications(m_currentNotificationManager.get(), array.get());
+    WKNotificationManagerProviderDidCloseNotifications(notificationManager, array.get());
 }
 
 void WebNotificationProvider::addNotificationManager(WKNotificationManagerRef manager)
 {
-    m_currentNotificationManager = manager;
+    m_ownedNotifications.add(manager, HashSet<uint64_t>());
 }
 
 void WebNotificationProvider::removeNotificationManager(WKNotificationManagerRef manager)
 {
+    auto iterator = m_ownedNotifications.find(manager);
+    ASSERT(iterator != m_ownedNotifications.end());
+    auto toRemove = iterator->value;
+    WKRetainPtr<WKNotificationManagerRef> guard(manager);
+    m_ownedNotifications.remove(iterator);
+    WKRetainPtr<WKMutableArrayRef> array = adoptWK(WKMutableArrayCreate());
+    for (uint64_t notificationID : toRemove) {
+        bool success = m_owningManager.remove(notificationID);
+        ASSERT_UNUSED(success, success);
+        WKArrayAppendItem(array.get(), adoptWK(WKUInt64Create(notificationID)).get());
+    }
+    WKNotificationManagerProviderDidCloseNotifications(manager, array.get());
 }
 
@@ -127 +146 @@
 void WebNotificationProvider::simulateWebNotificationClick(uint64_t notificationID)
 {
-    if (!m_currentNotificationManager)
-        return;
-
-    ASSERT(m_shownNotifications.contains(notificationID));
-    WKNotificationManagerProviderDidClickNotification(m_currentNotificationManager.get(), notificationID);
+    ASSERT(m_owningManager.contains(notificationID));
+    WKNotificationManagerProviderDidClickNotification(m_owningManager.get(notificationID), notificationID);
 }
 
 void WebNotificationProvider::reset()
 {
-    if (!m_currentNotificationManager) {
-        m_shownNotifications.clear();
-        return;
+    for (auto& notificationPair : m_ownedNotifications) {
+        if (notificationPair.value.isEmpty())
+            continue;
+        WKRetainPtr<WKMutableArrayRef> array = adoptWK(WKMutableArrayCreate());
+        for (uint64_t notificationID : notificationPair.value)
+            WKArrayAppendItem(array.get(), adoptWK(WKUInt64Create(notificationID)).get());
+
+        notificationPair.value.clear();
+        WKNotificationManagerProviderDidCloseNotifications(notificationPair.key.get(), array.get());
     }
-
-    WKRetainPtr<WKMutableArrayRef> array(AdoptWK, WKMutableArrayCreate());
-    HashSet<uint64_t>::const_iterator itEnd = m_shownNotifications.end();
-    for (HashSet<uint64_t>::const_iterator it = m_shownNotifications.begin(); it != itEnd; ++it) {
-        WKRetainPtr<WKUInt64Ref> wkID = WKUInt64Create(*it);
-        WKArrayAppendItem(array.get(), wkID.get());
-    }
-
-    m_shownNotifications.clear();
-    WKNotificationManagerProviderDidCloseNotifications(m_currentNotificationManager.get(), array.get());
+    m_owningManager.clear();
 }
 

trunk/Tools/WebKitTestRunner/WebNotificationProvider.h

@@ -30 +30 @@
 #include <WebKit/WKNotificationProvider.h>
 #include <WebKit/WKRetainPtr.h>
+#include <wtf/HashMap.h>
 #include <wtf/HashSet.h>
 
@@ -50 +51 @@
 
 private:
-    WKRetainPtr<WKNotificationManagerRef> m_currentNotificationManager;
-    HashSet<uint64_t> m_shownNotifications;
+    // Inverses of each other.
+    HashMap<WKRetainPtr<WKNotificationManagerRef>, HashSet<uint64_t>> m_ownedNotifications;
+    HashMap<uint64_t, WKNotificationManagerRef> m_owningManager;
 };