REGRESSION(r189668): Notification tests are flakey
authormmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Jul 2016 02:53:11 +0000 (02:53 +0000)
committermmaxfield@apple.com <mmaxfield@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 2 Jul 2016 02:53:11 +0000 (02:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=159375
<rdar://problem/22760990>

Reviewed by Alexey Proskuryakov.

Source/WebKit2:

Implement WKNotificationManagerGetLocalID(). For more information, see the entry in Tools/ChangeLog.

* UIProcess/API/C/WKNotificationManager.cpp:
(WKNotificationManagerGetLocalID):
* UIProcess/Notifications/WebNotificationManagerProxy.cpp:
(WebKit::WebNotificationManagerProxy::notificationLocalIDForTesting):
* UIProcess/Notifications/WebNotificationManagerProxy.h:
* WebKit2.xcodeproj/project.pbxproj:

Tools:

Notifications are objects which must exist in both the UI Process and the Web Process. Each process
identifies a notification object by a unique ID. When the Web Process sends a message regarding a
notification to the UI Process, the UI Process's WebNotificationManagerProxy holds a map from
(Page ID, Web Process notification ID) -> UI Process notification ID. This works as intended.

Our tests, however, include an additional method, simulateWebNotificationClick(), which is implemented
by WebKitTestRunner in the Web Process via the Injected Bundle. This method involves sending a message
to the UI process, to handle the simulated click. However, that RPC didn't perform the same local ->
global notification ID mapping, causing the wrong notification to be investigated.

The solution is for WebNotificationProvider, implemented in WebKitTestRunner in the UI Process, to
manually perform this same mapping. Luckily, this object already receives callbacks every time a
notification is created or destroyed. However, because this object is implemented outside WebKit,
it isn't privy to the internal Web Process ID used inside WebNotificationmanagerProxy. Therefore,
this patch adds a private testing function which returns this internal ID. Once given this intenal ID,
WebNotificationProvider can properly map between the different IDs.

* WebKitTestRunner/TestController.cpp:
(WTR::TestController::runTestingServerLoop):
(WTR::TestController::simulateWebNotificationClick):
* WebKitTestRunner/WebNotificationProvider.cpp:
(WTR::WebNotificationProvider::showWebNotification):
(WTR::removeGlobalIDFromIDMap):
(WTR::WebNotificationProvider::closeWebNotification):
(WTR::WebNotificationProvider::removeNotificationManager):
(WTR::WebNotificationProvider::simulateWebNotificationClick):
(WTR::WebNotificationProvider::reset):
* WebKitTestRunner/WebNotificationProvider.h:

LayoutTests:

* platform/mac/TestExpectations:
* platform/mac-wk2/TestExpectations:

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

12 files changed:
LayoutTests/ChangeLog
LayoutTests/platform/mac-wk2/TestExpectations
LayoutTests/platform/mac/TestExpectations
Source/WebKit2/ChangeLog
Source/WebKit2/UIProcess/API/C/WKNotificationManager.cpp
Source/WebKit2/UIProcess/API/C/WKNotificationManager.h
Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp
Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.h
Tools/ChangeLog
Tools/WebKitTestRunner/TestController.cpp
Tools/WebKitTestRunner/WebNotificationProvider.cpp
Tools/WebKitTestRunner/WebNotificationProvider.h

index e144e4e..fba68f7 100644 (file)
@@ -1,3 +1,14 @@
+2016-07-01  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        REGRESSION(r189668): Notification tests are flakey
+        https://bugs.webkit.org/show_bug.cgi?id=159375
+        <rdar://problem/22760990>
+
+        Reviewed by Alexey Proskuryakov.
+
+        * platform/mac/TestExpectations:
+        * platform/mac-wk2/TestExpectations:
+
 2016-07-01  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r202766.
index edab0d6..706d5fd 100644 (file)
@@ -276,8 +276,6 @@ webkit.org/b/150095 http/tests/xmlhttprequest/xmlhttprequest-overridemimetype-in
 
 webkit.org/b/150241 webarchive/loading/object.html [ Pass Crash ] 
 
-webkit.org/b/150736 http/tests/notifications/legacy/events.html [ Pass Crash ]
-
 webkit.org/b/150942 animations/multiple-backgrounds.html [ Pass ImageOnlyFailure ]
 
 webkit.org/b/151053 http/tests/security/cross-frame-access-put.html [ Pass Failure ]
index f1044da..2fb3509 100644 (file)
@@ -1263,9 +1263,6 @@ webkit.org/b/150225 fast/custom-elements [ Pass ]
 # Marking test as flaky in El Capitan
 webkit.org/b/149819 [ Debug ElCapitan+ ] compositing/video/video-poster.html [ Pass Crash ]
 
-# Marking as flaky again, patch did not work
-webkit.org/b/149218 http/tests/notifications/events.html [ Pass Crash ]
-
 # Flaky on Mac
 webkit.org/b/148435 storage/domstorage/events/basic-body-attribute.html [ Pass Failure ]
 
index 05f7297..96d6d39 100644 (file)
@@ -1,3 +1,20 @@
+2016-07-01  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        REGRESSION(r189668): Notification tests are flakey
+        https://bugs.webkit.org/show_bug.cgi?id=159375
+        <rdar://problem/22760990>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Implement WKNotificationManagerGetLocalID(). For more information, see the entry in Tools/ChangeLog.
+
+        * UIProcess/API/C/WKNotificationManager.cpp:
+        (WKNotificationManagerGetLocalID):
+        * UIProcess/Notifications/WebNotificationManagerProxy.cpp:
+        (WebKit::WebNotificationManagerProxy::notificationLocalIDForTesting):
+        * UIProcess/Notifications/WebNotificationManagerProxy.h:
+        * WebKit2.xcodeproj/project.pbxproj:
+
 2016-07-01  Jer Noble  <jer.noble@apple.com>
 
         REGRESSION(r201405): Fullscreen video no longer enters low-power mode
index 1e050a6..70f15f0 100644 (file)
@@ -27,6 +27,7 @@
 #include "WKNotificationManager.h"
 
 #include "WKAPICast.h"
+#include "WebNotification.h"
 #include "WebNotificationManagerProxy.h"
 
 using namespace WebKit;
@@ -65,3 +66,8 @@ void WKNotificationManagerProviderDidRemoveNotificationPolicies(WKNotificationMa
 {
     toImpl(managerRef)->providerDidRemoveNotificationPolicies(toImpl(origins));
 }
+
+uint64_t WKNotificationManagerGetLocalIDForTesting(WKNotificationManagerRef manager, WKNotificationRef notification)
+{
+    return toImpl(manager)->notificationLocalIDForTesting(toImpl(notification));
+}
index cb88036..b78909c 100644 (file)
@@ -41,6 +41,7 @@ WK_EXPORT void WKNotificationManagerProviderDidClickNotification(WKNotificationM
 WK_EXPORT void WKNotificationManagerProviderDidCloseNotifications(WKNotificationManagerRef managerRef, WKArrayRef notificationIDs);
 WK_EXPORT void WKNotificationManagerProviderDidUpdateNotificationPolicy(WKNotificationManagerRef managerRef, WKSecurityOriginRef origin, bool allowed);
 WK_EXPORT void WKNotificationManagerProviderDidRemoveNotificationPolicies(WKNotificationManagerRef managerRef, WKArrayRef origins);
+WK_EXPORT uint64_t WKNotificationManagerGetLocalIDForTesting(WKNotificationManagerRef managerRef, WKNotificationRef notification);
 
 #ifdef __cplusplus
 }
index 6263516..a034007 100644 (file)
@@ -256,4 +256,16 @@ void WebNotificationManagerProxy::providerDidRemoveNotificationPolicies(API::Arr
     processPool()->sendToAllProcesses(Messages::WebNotificationManager::DidRemoveNotificationDecisions(originStrings));
 }
 
+uint64_t WebNotificationManagerProxy::notificationLocalIDForTesting(WebNotification* notification)
+{
+    if (!notification)
+        return 0;
+
+    auto it = m_globalNotificationMap.find(notification->notificationID());
+    if (it == m_globalNotificationMap.end())
+        return 0;
+
+    return it->value.second;
+}
+
 } // namespace WebKit
index 44f46a9..fce07f5 100644 (file)
@@ -67,6 +67,8 @@ public:
     void providerDidUpdateNotificationPolicy(const API::SecurityOrigin*, bool allowed);
     void providerDidRemoveNotificationPolicies(API::Array* origins);
 
+    uint64_t notificationLocalIDForTesting(WebNotification*);
+
     using API::Object::ref;
     using API::Object::deref;
 
index a7d9676..1448f44 100644 (file)
@@ -1,3 +1,40 @@
+2016-07-01  Myles C. Maxfield  <mmaxfield@apple.com>
+
+        REGRESSION(r189668): Notification tests are flakey
+        https://bugs.webkit.org/show_bug.cgi?id=159375
+        <rdar://problem/22760990>
+
+        Reviewed by Alexey Proskuryakov.
+
+        Notifications are objects which must exist in both the UI Process and the Web Process. Each process
+        identifies a notification object by a unique ID. When the Web Process sends a message regarding a
+        notification to the UI Process, the UI Process's WebNotificationManagerProxy holds a map from
+        (Page ID, Web Process notification ID) -> UI Process notification ID. This works as intended.
+
+        Our tests, however, include an additional method, simulateWebNotificationClick(), which is implemented
+        by WebKitTestRunner in the Web Process via the Injected Bundle. This method involves sending a message
+        to the UI process, to handle the simulated click. However, that RPC didn't perform the same local ->
+        global notification ID mapping, causing the wrong notification to be investigated.
+
+        The solution is for WebNotificationProvider, implemented in WebKitTestRunner in the UI Process, to
+        manually perform this same mapping. Luckily, this object already receives callbacks every time a
+        notification is created or destroyed. However, because this object is implemented outside WebKit,
+        it isn't privy to the internal Web Process ID used inside WebNotificationmanagerProxy. Therefore,
+        this patch adds a private testing function which returns this internal ID. Once given this intenal ID,
+        WebNotificationProvider can properly map between the different IDs.
+
+        * WebKitTestRunner/TestController.cpp:
+        (WTR::TestController::runTestingServerLoop):
+        (WTR::TestController::simulateWebNotificationClick):
+        * WebKitTestRunner/WebNotificationProvider.cpp:
+        (WTR::WebNotificationProvider::showWebNotification):
+        (WTR::removeGlobalIDFromIDMap):
+        (WTR::WebNotificationProvider::closeWebNotification):
+        (WTR::WebNotificationProvider::removeNotificationManager):
+        (WTR::WebNotificationProvider::simulateWebNotificationClick):
+        (WTR::WebNotificationProvider::reset):
+        * WebKitTestRunner/WebNotificationProvider.h:
+
 2016-07-01  Alexey Proskuryakov  <ap@apple.com>
 
         Simplify LayoutTestRelay
index 3202554..b4dbe0e 100644 (file)
@@ -1746,7 +1746,7 @@ void TestController::didRemoveNavigationGestureSnapshot(WKPageRef)
 
 void TestController::simulateWebNotificationClick(uint64_t notificationID)
 {
-    m_webNotificationProvider.simulateWebNotificationClick(notificationID);
+    m_webNotificationProvider.simulateWebNotificationClick(mainWebView()->page(), notificationID);
 }
 
 void TestController::setGeolocationPermission(bool enabled)
index de6adec..b009b36 100644 (file)
@@ -28,6 +28,7 @@
 
 #include <WebKit/WKMutableArray.h>
 #include <WebKit/WKNotification.h>
+#include <WebKit/WKNotificationManager.h>
 #include <WebKit/WKNumber.h>
 #include <WebKit/WKSecurityOriginRef.h>
 #include <wtf/Assertions.h>
@@ -95,10 +96,23 @@ void WebNotificationProvider::showWebNotification(WKPageRef page, WKNotification
     ASSERT_UNUSED(addResult, addResult.isNewEntry);
     auto addResult2 = m_owningManager.set(id, notificationManager);
     ASSERT_UNUSED(addResult2, addResult2.isNewEntry);
+    auto addResult3 = m_localToGlobalNotificationIDMap.add(std::make_pair(page, WKNotificationManagerGetLocalIDForTesting(notificationManager, notification)), id);
+    ASSERT_UNUSED(addResult3, addResult3.isNewEntry);
 
     WKNotificationManagerProviderDidShowNotification(notificationManager, id);
 }
 
+static void removeGlobalIDFromIDMap(HashMap<std::pair<WKPageRef, uint64_t>, uint64_t>& map, uint64_t id)
+{
+    for (auto iter = map.begin(); iter != map.end(); ++iter) {
+        if (iter->value == id) {
+            map.remove(iter);
+            return;
+        }
+    }
+    ASSERT_NOT_REACHED();
+}
+
 void WebNotificationProvider::closeWebNotification(WKNotificationRef notification)
 {
     uint64_t id = WKNotificationGetID(notification);
@@ -110,6 +124,8 @@ void WebNotificationProvider::closeWebNotification(WKNotificationRef notificatio
     ASSERT_UNUSED(success, success);
     m_owningManager.remove(id);
 
+    removeGlobalIDFromIDMap(m_localToGlobalNotificationIDMap, id);
+
     WKRetainPtr<WKUInt64Ref> wkID = WKUInt64Create(id);
     WKRetainPtr<WKMutableArrayRef> array(AdoptWK, WKMutableArrayCreate());
     WKArrayAppendItem(array.get(), wkID.get());
@@ -132,6 +148,7 @@ void WebNotificationProvider::removeNotificationManager(WKNotificationManagerRef
     for (uint64_t notificationID : toRemove) {
         bool success = m_owningManager.remove(notificationID);
         ASSERT_UNUSED(success, success);
+        removeGlobalIDFromIDMap(m_localToGlobalNotificationIDMap, notificationID);
         WKArrayAppendItem(array.get(), adoptWK(WKUInt64Create(notificationID)).get());
     }
     WKNotificationManagerProviderDidCloseNotifications(manager, array.get());
@@ -143,10 +160,12 @@ WKDictionaryRef WebNotificationProvider::notificationPermissions()
     return WKMutableDictionaryCreate();
 }
 
-void WebNotificationProvider::simulateWebNotificationClick(uint64_t notificationID)
+void WebNotificationProvider::simulateWebNotificationClick(WKPageRef page, uint64_t notificationID)
 {
-    ASSERT(m_owningManager.contains(notificationID));
-    WKNotificationManagerProviderDidClickNotification(m_owningManager.get(notificationID), notificationID);
+    ASSERT(m_localToGlobalNotificationIDMap.contains(std::make_pair(page, notificationID)));
+    auto globalID = m_localToGlobalNotificationIDMap.get(std::make_pair(page, notificationID));
+    ASSERT(m_owningManager.contains(globalID));
+    WKNotificationManagerProviderDidClickNotification(m_owningManager.get(globalID), globalID);
 }
 
 void WebNotificationProvider::reset()
@@ -162,6 +181,7 @@ void WebNotificationProvider::reset()
         WKNotificationManagerProviderDidCloseNotifications(notificationPair.key.get(), array.get());
     }
     m_owningManager.clear();
+    m_localToGlobalNotificationIDMap.clear();
 }
 
 } // namespace WTR
index d8724a9..ccf8b1b 100644 (file)
@@ -46,13 +46,15 @@ public:
     void removeNotificationManager(WKNotificationManagerRef);
     WKDictionaryRef notificationPermissions();
 
-    void simulateWebNotificationClick(uint64_t notificationID);
+    void simulateWebNotificationClick(WKPageRef, uint64_t notificationID);
     void reset();
 
 private:
     // Inverses of each other.
     HashMap<WKRetainPtr<WKNotificationManagerRef>, HashSet<uint64_t>> m_ownedNotifications;
     HashMap<uint64_t, WKNotificationManagerRef> m_owningManager;
+
+    HashMap<std::pair<WKPageRef, uint64_t>, uint64_t> m_localToGlobalNotificationIDMap;
 };
 
 }