Fix UNNotification multi profile support

The UNNotification API expects us to provide a unique identifier for
each notification we show. The previous implementation simply used the
notification id as-is which breaks with multiple profiles as their ids
may overlap.

This CL changes the used id to be "ProfileID|NotificationId" instead to
avoid this issue. Also fixes support for the renotify flag.

Bug: 1127306
Change-Id: I9f302c3e045d2f6f3358f5d528e80f9a626fca59
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2637519
Commit-Queue: Richard Knoll <knollr@chromium.org>
Reviewed-by: Rayan Kanso <rayankans@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845621}

chrome/browser/notifications/notification_platform_bridge_mac_unnotification.h

@@ -63,6 +63,7 @@ class API_AVAILABLE(macosx(10.14)) NotificationPlatformBridgeMacUNNotification
 
   // Process notification request that got delivered successfully.
   void DeliveredSuccessfully(
+      const std::string& notification_id,
       base::scoped_nsobject<UNNotificationBuilder> builder);
 
   // Determine whether to start synchronization process of notifications or not.

chrome/browser/notifications/notification_platform_bridge_mac_unnotification.mm

@@ -136,10 +136,8 @@ void NotificationPlatformBridgeMacUNNotification::Display(
     [builder setButtons:buttonOne secondaryButton:buttonTwo];
   }
 
-  NSString* notification_id = base::SysUTF8ToNSString(notification.id());
-
   [builder setOrigin:base::SysUTF8ToNSString(notification.origin_url().spec())];
-  [builder setNotificationId:notification_id];
+  [builder setNotificationId:base::SysUTF8ToNSString(notification.id())];
   [builder setProfileId:base::SysUTF8ToNSString(GetProfileId(profile))];
   [builder setIncognito:profile->IsOffTheRecord()];
   [builder setCreatorPid:[NSNumber numberWithInteger:static_cast<NSInteger>(
@@ -156,6 +154,10 @@ void NotificationPlatformBridgeMacUNNotification::Display(
   // Create a new category from the desired action buttons.
   UNNotificationCategory* category = [builder buildCategory];
 
+  std::string system_notification_id =
+      DeriveMacNotificationId(GetProfileId(profile), notification.id());
+  NSString* notification_id = base::SysUTF8ToNSString(system_notification_id);
+
   // Check if this notification had an already existing category from a previous
   // call, if that is the case then remove it.
   if (UNNotificationCategory* existing =
@@ -185,13 +187,16 @@ void NotificationPlatformBridgeMacUNNotification::Display(
         FROM_HERE,
         base::BindOnce(
             &NotificationPlatformBridgeMacUNNotification::DeliveredSuccessfully,
-            weak_ptr, std::move(builder)));
+            weak_ptr, system_notification_id, std::move(builder)));
   };
 
-  if ([notification_center_
-          respondsToSelector:@selector
-          (replaceContentForRequestWithIdentifier:
-                               replacementContent:completionHandler:)]) {
+  // If the renotify is not set try to replace the notification silently.
+  bool should_replace = !notification.renotify();
+  bool can_replace = [notification_center_
+      respondsToSelector:@selector
+      (replaceContentForRequestWithIdentifier:
+                           replacementContent:completionHandler:)];
+  if (should_replace && can_replace) {
     // If the notification has been delivered before, it will get updated in the
     // notification center. If it hasn't been delivered before it will deliver
     // it and show it on the screen.
@@ -213,27 +218,21 @@ void NotificationPlatformBridgeMacUNNotification::Display(
 void NotificationPlatformBridgeMacUNNotification::Close(
     Profile* profile,
     const std::string& notification_id) {
-  NSString* candidateId = base::SysUTF8ToNSString(notification_id);
-  NSString* currentProfileId = base::SysUTF8ToNSString(GetProfileId(profile));
+  NSString* notificationId = base::SysUTF8ToNSString(
+      DeriveMacNotificationId(GetProfileId(profile), notification_id));
   base::WeakPtr<NotificationPlatformBridgeMacUNNotification> weak_ptr =
       weak_factory_.GetWeakPtr();
 
   [notification_center_ getDeliveredNotificationsWithCompletionHandler:^(
                             NSArray<UNNotification*>* _Nonnull notifications) {
     for (UNNotification* notification in notifications) {
-      NSString* toastId = [[[[notification request] content] userInfo]
-          objectForKey:notification_constants::kNotificationId];
-      NSString* persistentProfileId =
-          [[[[notification request] content] userInfo]
-              objectForKey:notification_constants::kNotificationProfileId];
-
-      if ([toastId isEqualToString:candidateId] &&
-          [persistentProfileId isEqualToString:currentProfileId]) {
+      NSString* toastNotificationId = [[notification request] identifier];
+      if ([notificationId isEqualToString:toastNotificationId]) {
         content::GetUIThreadTaskRunner({})->PostTask(
             FROM_HERE,
             base::BindOnce(
                 &NotificationPlatformBridgeMacUNNotification::DoClose, weak_ptr,
-                base::SysNSStringToUTF8(toastId)));
+                base::SysNSStringToUTF8(notificationId)));
         break;
       }
     }
@@ -317,14 +316,14 @@ void NotificationPlatformBridgeMacUNNotification::DoClose(
 }
 
 void NotificationPlatformBridgeMacUNNotification::DeliveredSuccessfully(
+    const std::string& notification_id,
     base::scoped_nsobject<UNNotificationBuilder> builder) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   NSDictionary* dict = [builder buildDictionary];
 
-  [delivered_notifications_
-      setObject:dict
-         forKey:[dict objectForKey:notification_constants::kNotificationId]];
+  [delivered_notifications_ setObject:dict
+                               forKey:base::SysUTF8ToNSString(notification_id)];
 
   NotificationPlatformBridgeMacUNNotification::MaybeStartSynchronization();
 }
@@ -356,11 +355,12 @@ void NotificationPlatformBridgeMacUNNotification::SynchronizeNotifications() {
 
     for (UNNotification* notification in notifications) {
       std::string notification_id =
-          base::SysNSStringToUTF8([[[[notification request] content] userInfo]
-              objectForKey:notification_constants::kNotificationId]);
+          base::SysNSStringToUTF8([[notification request] identifier]);
       notification_ids.insert(std::move(notification_id));
     }
 
+    // TODO(crbug/1134570): Check for displayed alerts as well.
+
     content::GetUIThreadTaskRunner({})->PostTask(
         FROM_HERE, base::BindOnce(&NotificationPlatformBridgeMacUNNotification::
                                       DoSynchronizeNotifications,

chrome/browser/notifications/notification_platform_bridge_mac_unnotification_unittest.mm

@@ -79,7 +79,7 @@ API_AVAILABLE(macosx(10.14))
 @end
 
 @implementation FakeUNUserNotificationCenter {
-  base::scoped_nsobject<NSMutableArray> _banners;
+  base::scoped_nsobject<NSMutableDictionary> _banners;
   base::scoped_nsobject<NSSet> _categories;
 }
 
@@ -88,7 +88,7 @@ API_AVAILABLE(macosx(10.14))
 - (instancetype)init {
   if ((self = [super init])) {
     settings.reset([[FakeUNNotificationSettings alloc] init]);
-    _banners.reset([[NSMutableArray alloc] init]);
+    _banners.reset([[NSMutableDictionary alloc] init]);
     _categories.reset([[NSSet alloc] init]);
   }
   return self;
@@ -118,7 +118,7 @@ API_AVAILABLE(macosx(10.14))
   base::scoped_nsobject<FakeNotification> notification(
       [[FakeNotification alloc] init]);
   [notification setRequest:request];
-  [_banners addObject:notification];
+  [_banners setObject:notification forKey:[request identifier]];
   notificationDelivered(/*error=*/nil);
 }
 
@@ -127,13 +127,13 @@ API_AVAILABLE(macosx(10.14))
   base::scoped_nsobject<FakeNotification> notification(
       [[FakeNotification alloc] init]);
   [notification setRequest:request];
-  [_banners addObject:notification];
+  [_banners setObject:notification forKey:[request identifier]];
   completionHandler(/*error=*/nil);
 }
 
 - (void)getDeliveredNotificationsWithCompletionHandler:
     (void (^)(NSArray<UNNotification*>* notifications))completionHandler {
-  completionHandler([[_banners copy] autorelease]);
+  completionHandler([_banners allValues]);
 }
 
 - (void)getNotificationCategoriesWithCompletionHandler:
@@ -149,13 +149,7 @@ API_AVAILABLE(macosx(10.14))
 
 - (void)removeDeliveredNotificationsWithIdentifiers:
     (NSArray<NSString*>*)identifiers {
-  [_banners filterUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(
-                                                  UNNotification* notification,
-                                                  NSDictionary* bindings) {
-              NSString* toastId = [[[[notification request] content] userInfo]
-                  objectForKey:notification_constants::kNotificationId];
-              return ![identifiers containsObject:toastId];
-            }]];
+  [_banners removeObjectsForKeys:identifiers];
 }
 
 - (void)getNotificationSettingsWithCompletionHandler:
@@ -205,8 +199,6 @@ class UNNotificationPlatformBridgeMacTest : public testing::Test {
   std::unique_ptr<NotificationPlatformBridgeMacUNNotification> bridge_;
   TestingProfile* profile_ = nullptr;
   base::HistogramTester histogram_tester_;
-
- private:
   content::BrowserTaskEnvironment task_environment_;
   TestingProfileManager manager_;
 };
@@ -244,6 +236,24 @@ TEST_F(UNNotificationPlatformBridgeMacTest, TestDisplay) {
   }
 }
 
+TEST_F(UNNotificationPlatformBridgeMacTest, TestDisplayMultipleProfiles) {
+  if (@available(macOS 10.14, *)) {
+    TestingProfile* profile_1 = manager_.CreateTestingProfile("P1");
+    TestingProfile* profile_2 = manager_.CreateTestingProfile("P2");
+
+    // Show two notifications with the same id from different profiles.
+    bridge_->Display(NotificationHandler::Type::WEB_PERSISTENT, profile_1,
+                     CreateNotification("id"), /*metadata=*/nullptr);
+    bridge_->Display(NotificationHandler::Type::WEB_PERSISTENT, profile_2,
+                     CreateNotification("id"), /*metadata=*/nullptr);
+
+    [center_ getDeliveredNotificationsWithCompletionHandler:^(
+                 NSArray<UNNotification*>* _Nonnull notifications) {
+      ASSERT_EQ(2u, [notifications count]);
+    }];
+  }
+}
+
 TEST_F(UNNotificationPlatformBridgeMacTest, TestNotificationHasIcon) {
   if (@available(macOS 10.14, *)) {
     Notification notification = CreateNotification();

chrome/browser/notifications/notification_platform_bridge_mac_utils.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_PLATFORM_BRIDGE_MAC_UTILS_H_
 #define CHROME_BROWSER_NOTIFICATIONS_NOTIFICATION_PLATFORM_BRIDGE_MAC_UTILS_H_
 
+#include <string>
+
 #include "base/strings/string16.h"
 #include "chrome/browser/notifications/notification_common.h"
 #include "ui/message_center/public/cpp/notification.h"
@@ -24,6 +26,11 @@ base::string16 CreateMacNotificationContext(
     const message_center::Notification& notification,
     bool requires_attribution);
 
+// Derives a unique notification identifier to be used by the macOS system
+// notification center to uniquely identify a notification.
+std::string DeriveMacNotificationId(const std::string& profile_id,
+                                    const std::string& notification_id);
+
 // Validates contents of the |response| dictionary as received from the system
 // when a notification gets activated.
 bool VerifyMacNotificationData(NSDictionary* response) WARN_UNUSED_RESULT;

chrome/browser/notifications/notification_platform_bridge_mac_utils.mm

@@ -7,6 +7,7 @@
 #include "base/feature_list.h"
 #include "base/i18n/number_formatting.h"
 #include "base/optional.h"
+#include "base/strings/strcat.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/system/sys_info.h"
@@ -124,6 +125,11 @@ base::string16 CreateMacNotificationContext(
   return etldplusone;
 }
 
+std::string DeriveMacNotificationId(const std::string& profile_id,
+                                    const std::string& notification_id) {
+  return base::StrCat({profile_id, "|", notification_id});
+}
+
 bool VerifyMacNotificationData(NSDictionary* response) {
   if (![response
           objectForKey:notification_constants::kNotificationButtonIndex] ||