Changeset 258613 in webkit

Timestamp:

Mar 17, 2020, 5:12:47 PM (6 years ago)

Author:

Chris Dumez

Message:

REGRESSION: [ macOS wk1 ] ASSERTION FAILED: _notifications.contains(notificationID) imported/w3c/web-platform-tests/notifications/constructor-basic.html is flaky crashing
​https://bugs.webkit.org/show_bug.cgi?id=207307
<rdar://problem/59206964>

Reviewed by Alex Christensen.

Source/WebKitLegacy/mac:

• WebView/WebNotification.h:
• WebView/WebNotification.mm:

(-[WebNotification finalize]):

Tools:

When [MockWebNotificationProvider reset] was called at the end of the test, it would remove all
notifications from the map but not tell WebCore that the notification were discarded. As a result,
WebCore would later tell the MockWebNotificationProvider to cancel the notification but this
notification would no longer be in the map, causing us to hit an assertion in debug.

To address the issue, we now call Notification::finalize() in [MockWebNotificationProvider reset]
to let WebCore know the notification was discarded. This is similar to what is already done for
WebKit2 in WebNotificationManager::clearNotifications().

• DumpRenderTree/mac/MockWebNotificationProvider.mm:

(-[MockWebNotificationProvider reset]):

LayoutTests:

Add test coverage.

• http/wpt/notifications/constructor-basic-bfcache-expected.txt: Added.
• http/wpt/notifications/constructor-basic-bfcache.html: Added.
• platform/mac/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/wpt/notifications (added)
• LayoutTests/http/wpt/notifications/constructor-basic-bfcache-expected.txt (added)
• LayoutTests/http/wpt/notifications/constructor-basic-bfcache.html (added)
• LayoutTests/platform/mac/TestExpectations (modified) (1 diff)
• LayoutTests/platform/win/TestExpectations (modified) (1 diff)
• Source/WebKitLegacy/mac/ChangeLog (modified) (1 diff)
• Source/WebKitLegacy/mac/WebView/WebNotification.h (modified) (1 diff)
• Source/WebKitLegacy/mac/WebView/WebNotification.mm (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/DumpRenderTree/mac/MockWebNotificationProvider.mm (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-03-17  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION: [ macOS wk1 ] ASSERTION FAILED: _notifications.contains(notificationID) imported/w3c/web-platform-tests/notifications/constructor-basic.html is flaky crashing
+        https://bugs.webkit.org/show_bug.cgi?id=207307
+        <rdar://problem/59206964>
+
+        Reviewed by Alex Christensen.
+
+        Add test coverage.
+
+        * http/wpt/notifications/constructor-basic-bfcache-expected.txt: Added.
+        * http/wpt/notifications/constructor-basic-bfcache.html: Added.
+        * platform/mac/TestExpectations:
+
 2020-03-17  Chris Fleizach  <cfleizach@apple.com>
 

trunk/LayoutTests/platform/mac/TestExpectations

@@ -1964 +1964 @@
 webkit.org/b/207333 imported/w3c/web-platform-tests/2dcontext/imagebitmap/createImageBitmap-transfer.html [ Pass Failure ]
 
-webkit.org/b/207307 imported/w3c/web-platform-tests/notifications/constructor-basic.html [ Pass Crash ]
-
 webkit.org/b/112939 fast/performance/performance-now-timestamps.html [ Pass Failure ]
 

trunk/LayoutTests/platform/win/TestExpectations

@@ -198 +198 @@
 # TODO This port doesn't support Notifications.
 http/tests/notifications/ [ Skip ]
+http/wpt/notifications/ [ Skip ]
 fast/history/page-cache-notification-showing.html [ Skip ]
 fast/history/page-cache-notification-suspendable.html [ Skip ]

trunk/Source/WebKitLegacy/mac/ChangeLog

@@ +1 @@
+2020-03-17  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION: [ macOS wk1 ] ASSERTION FAILED: _notifications.contains(notificationID) imported/w3c/web-platform-tests/notifications/constructor-basic.html is flaky crashing
+        https://bugs.webkit.org/show_bug.cgi?id=207307
+        <rdar://problem/59206964>
+
+        Reviewed by Alex Christensen.
+
+        * WebView/WebNotification.h:
+        * WebView/WebNotification.mm:
+        (-[WebNotification finalize]):
+
 2020-03-17  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/WebKitLegacy/mac/WebView/WebNotification.h

@@ -51 +51 @@
 - (void)dispatchClickEvent;
 - (void)dispatchErrorEvent;
+- (void)finalize;
 
 @end

trunk/Source/WebKitLegacy/mac/WebView/WebNotification.mm

@@ -194 +194 @@
 }
 
-@end
-
+- (void)finalize
+{
+#if ENABLE(NOTIFICATIONS)
+    core(self)->finalize();
+#endif
+}
+
+@end
+

trunk/Tools/ChangeLog

@@ +1 @@
+2020-03-17  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION: [ macOS wk1 ] ASSERTION FAILED: _notifications.contains(notificationID) imported/w3c/web-platform-tests/notifications/constructor-basic.html is flaky crashing
+        https://bugs.webkit.org/show_bug.cgi?id=207307
+        <rdar://problem/59206964>
+
+        Reviewed by Alex Christensen.
+
+        When [MockWebNotificationProvider reset] was called at the end of the test, it would remove all
+        notifications from the map but not tell WebCore that the notification were discarded. As a result,
+        WebCore would later tell the MockWebNotificationProvider to cancel the notification but this
+        notification would no longer be in the map, causing us to hit an assertion in debug.
+
+        To address the issue, we now call Notification::finalize() in [MockWebNotificationProvider reset]
+        to let WebCore know the notification was discarded. This is similar to what is already done for
+        WebKit2 in WebNotificationManager::clearNotifications().
+
+        * DumpRenderTree/mac/MockWebNotificationProvider.mm:
+        (-[MockWebNotificationProvider reset]):
+
 2020-03-17  Don Olmstead  <don.olmstead@sony.com>
 

trunk/Tools/DumpRenderTree/mac/MockWebNotificationProvider.mm

@@ -144 +144 @@
 - (void)reset
 {
-    _notifications.clear();
+    auto notifications = WTFMove(_notifications);
+    for (auto notification : notifications.values())
+        [notification finalize];
+
     _notificationViewMap.clear();
     [self removeAllWebNotificationPermissions];