Reland "Fix a few memory leaks in NotificationPlatformBridgeWin"

This is a reland of 3d28a29b.

In the original change, NotificationPlatformBridgeWinTest.Suppress and
NotificationPlatformBridgeWinUITest.GetDisplayed failed in win-asan bot.

This CL fixed it.

The fix also required MockIToastNotification to derive from RuntimeClass,
which significantly simplified its implementation.

Original change's description:
> Fix a few memory leaks in NotificationPlatformBridgeWin
>
> We should 1) wrap the bare pointers in ComPtr; and 2) attach
> QueryInterfaces to ComPtr directly.
>
> Bug: 884224
> Change-Id: I91cb5c7312d8e24f59962ffb33faadaba36bd7ef
> Reviewed-on: https://chromium-review.googlesource.com/1227493
> Commit-Queue: Xi Cheng <chengx@chromium.org>
> Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
> Reviewed-by: Robert Liao <robliao@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#591737}

Bug: 884224, 886957
Change-Id: If2674299437f04f637f6b2b8278b40a4dbf7b9d0
Reviewed-on: https://chromium-review.googlesource.com/1228991
Commit-Queue: Xi Cheng <chengx@chromium.org>
Reviewed-by: Finnur Thorarinsson <finnur@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592487}

chrome/browser/notifications/notification_platform_bridge_win.cc

@@ -5,8 +5,9 @@
 #include "chrome/browser/notifications/notification_platform_bridge_win.h"
 
 #include <memory>
+#include <utility>
 
-#include <wrl/client.h>
+#include <objbase.h>
 #include <wrl/event.h>
 
 #include "base/bind.h"
@@ -212,8 +213,8 @@ class NotificationPlatformBridgeWinImpl
       return hr;
     }
 
-    winui::Notifications::IToastNotification2* toast2ptr = nullptr;
-    hr = (*toast_notification)->QueryInterface(&toast2ptr);
+    mswr::ComPtr<winui::Notifications::IToastNotification2> toast2;
+    hr = (*toast_notification)->QueryInterface(IID_PPV_ARGS(&toast2));
     if (FAILED(hr)) {
       LogDisplayHistogram(DisplayStatus::CREATE_TOAST_NOTIFICATION2_FAILED);
       DLOG(ERROR) << "Failed to get IToastNotification2 object " << std::hex
@@ -221,9 +222,6 @@ class NotificationPlatformBridgeWinImpl
       return hr;
     }
 
-    mswr::ComPtr<winui::Notifications::IToastNotification2> toast2;
-    toast2.Attach(toast2ptr);
-
     // Set the Group and Tag values for the notification, in order to support
     // closing/replacing notification by tag. Both of these values have a limit
     // of 64 characters, which is problematic because they are out of our
@@ -255,12 +253,12 @@ class NotificationPlatformBridgeWinImpl
     // 1) Renotify flag is not set.
     // 2) They are not new (no other notification with same tag is found).
     if (!notification.renotify()) {
-      std::vector<winui::Notifications::IToastNotification*> notifications;
-      GetNotifications(profile_id, incognito, &notifications);
+      std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>
+          notifications = GetNotifications(profile_id, incognito);
 
-      for (auto* notification : notifications) {
-        winui::Notifications::IToastNotification2* t2 = nullptr;
-        HRESULT hr = notification->QueryInterface(&t2);
+      for (const auto& notification : notifications) {
+        mswr::ComPtr<winui::Notifications::IToastNotification2> t2;
+        HRESULT hr = notification->QueryInterface(IID_PPV_ARGS(&t2));
         if (FAILED(hr))
           continue;
 
@@ -463,16 +461,17 @@ class NotificationPlatformBridgeWinImpl
     return true;
   }
 
-  void GetDisplayedFromActionCenter(
-      const std::string& profile_id,
-      bool incognito,
-      std::vector<winui::Notifications::IToastNotification*>* notifications)
-      const {
+  std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>
+  GetDisplayedFromActionCenter(const std::string& profile_id,
+                               bool incognito) const {
+    std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>
+        notifications;
+
     mswr::ComPtr<winui::Notifications::IToastNotificationHistory> history;
     if (!GetIToastNotificationHistory(&history)) {
       LogGetDisplayedStatus(GetDisplayedStatus::GET_TOAST_HISTORY_FAILED);
       DLOG(ERROR) << "Failed to get IToastNotificationHistory";
-      return;
+      return notifications;
     }
 
     mswr::ComPtr<winui::Notifications::IToastNotificationHistory2> history2;
@@ -483,7 +482,7 @@ class NotificationPlatformBridgeWinImpl
           GetDisplayedStatus::QUERY_TOAST_NOTIFICATION_HISTORY2_FAILED);
       DLOG(ERROR) << "Failed to get IToastNotificationHistory2 " << std::hex
                   << hr;
-      return;
+      return notifications;
     }
 
     ScopedHString application_id = ScopedHString::Create(GetAppId());
@@ -495,7 +494,7 @@ class NotificationPlatformBridgeWinImpl
     if (FAILED(hr)) {
       LogGetDisplayedStatus(GetDisplayedStatus::GET_HISTORY_WITH_ID_FAILED);
       DLOG(ERROR) << "GetHistoryWithId failed " << std::hex << hr;
-      return;
+      return notifications;
     }
 
     uint32_t size;
@@ -503,11 +502,11 @@ class NotificationPlatformBridgeWinImpl
     if (FAILED(hr)) {
       LogGetDisplayedStatus(GetDisplayedStatus::GET_SIZE_FAILED);
       DLOG(ERROR) << "History get_Size call failed " << std::hex << hr;
-      return;
+      return notifications;
     }
 
     GetDisplayedStatus status = GetDisplayedStatus::SUCCESS;
-    winui::Notifications::IToastNotification* tn;
+    mswr::ComPtr<winui::Notifications::IToastNotification> tn;
     for (uint32_t index = 0; index < size; ++index) {
       hr = list->GetAt(0U, &tn);
       if (FAILED(hr)) {
@@ -516,21 +515,19 @@ class NotificationPlatformBridgeWinImpl
                     << " " << std::hex << hr;
         continue;
       }
-      notifications->push_back(tn);
+      notifications.push_back(std::move(tn));
     }
 
     LogGetDisplayedStatus(status);
+    return notifications;
   }
 
-  void GetNotifications(const std::string& profile_id,
-                        bool incognito,
-                        std::vector<winui::Notifications::IToastNotification*>*
-                            notifications) const {
+  std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>
+  GetNotifications(const std::string& profile_id, bool incognito) const {
     if (!NotificationPlatformBridgeWinImpl::notifications_for_testing_) {
-      GetDisplayedFromActionCenter(profile_id, incognito, notifications);
+      return GetDisplayedFromActionCenter(profile_id, incognito);
     } else {
-      *notifications =
-          *NotificationPlatformBridgeWinImpl::notifications_for_testing_;
+      return *NotificationPlatformBridgeWinImpl::notifications_for_testing_;
     }
   }
 
@@ -541,12 +538,13 @@ class NotificationPlatformBridgeWinImpl
     // to NotificationPlatformBridgeWin::GetDisplayed.
     DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
-    std::vector<winui::Notifications::IToastNotification*> notifications;
-    GetNotifications(profile_id, incognito, &notifications);
+    std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>
+        notifications = GetNotifications(profile_id, incognito);
 
     auto displayed_notifications = std::make_unique<std::set<std::string>>();
-    for (auto* notification : notifications) {
-      NotificationLaunchId launch_id(GetNotificationLaunchId(notification));
+    for (const auto& notification : notifications) {
+      NotificationLaunchId launch_id(
+          GetNotificationLaunchId(notification.Get()));
       if (!launch_id.is_valid()) {
         LogGetDisplayedLaunchIdStatus(
             GetDisplayedLaunchIdStatus::DECODE_LAUNCH_ID_FAILED);
@@ -740,10 +738,10 @@ class NotificationPlatformBridgeWinImpl
     return hr;
   }
 
-  static std::vector<ABI::Windows::UI::Notifications::IToastNotification*>*
+  static std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>*
       notifications_for_testing_;
 
-  static ABI::Windows::UI::Notifications::IToastNotifier* notifier_for_testing_;
+  static winui::Notifications::IToastNotifier* notifier_for_testing_;
 
   // Whether the required functions from combase.dll have been loaded.
   bool com_functions_initialized_;
@@ -760,10 +758,10 @@ class NotificationPlatformBridgeWinImpl
   DISALLOW_COPY_AND_ASSIGN(NotificationPlatformBridgeWinImpl);
 };
 
-std::vector<ABI::Windows::UI::Notifications::IToastNotification*>*
+std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>*
     NotificationPlatformBridgeWinImpl::notifications_for_testing_ = nullptr;
 
-ABI::Windows::UI::Notifications::IToastNotifier*
+winui::Notifications::IToastNotifier*
     NotificationPlatformBridgeWinImpl::notifier_for_testing_ = nullptr;
 
 NotificationPlatformBridgeWin::NotificationPlatformBridgeWin() {
@@ -897,13 +895,13 @@ void NotificationPlatformBridgeWin::ForwardHandleEventForTesting(
 }
 
 void NotificationPlatformBridgeWin::SetDisplayedNotificationsForTesting(
-    std::vector<ABI::Windows::UI::Notifications::IToastNotification*>*
+    std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>*
         notifications) {
   NotificationPlatformBridgeWinImpl::notifications_for_testing_ = notifications;
 }
 
 void NotificationPlatformBridgeWin::SetNotifierForTesting(
-    ABI::Windows::UI::Notifications::IToastNotifier* notifier) {
+    winui::Notifications::IToastNotifier* notifier) {
   NotificationPlatformBridgeWinImpl::notifier_for_testing_ = notifier;
 }
 

chrome/browser/notifications/notification_platform_bridge_win.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include <windows.ui.notifications.h>
+#include <wrl/client.h>
 
 #include "base/macros.h"
 #include "base/optional.h"
@@ -72,8 +73,8 @@ class NotificationPlatformBridgeWin : public NotificationPlatformBridge {
 
   // Initializes the displayed notification vector. Only for use in testing.
   void SetDisplayedNotificationsForTesting(
-      std::vector<ABI::Windows::UI::Notifications::IToastNotification*>*
-          notifications);
+      std::vector<Microsoft::WRL::ComPtr<
+          ABI::Windows::UI::Notifications::IToastNotification>>* notifications);
 
   // Sets a Toast Notifier to use to display notifications, when run in a test.
   void SetNotifierForTesting(

chrome/browser/notifications/notification_platform_bridge_win_interactive_uitest.cc

@@ -6,6 +6,7 @@
 
 #include <windows.data.xml.dom.h>
 #include <wrl/client.h>
+#include <wrl/implements.h>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
@@ -341,7 +342,8 @@ IN_PROC_BROWSER_TEST_F(NotificationPlatformBridgeWinUITest, GetDisplayed) {
   NotificationPlatformBridgeWin* bridge = GetBridge();
   ASSERT_TRUE(bridge);
 
-  std::vector<winui::Notifications::IToastNotification*> notifications;
+  std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>
+      notifications;
   bridge->SetDisplayedNotificationsForTesting(&notifications);
 
   // Validate that empty list of notifications show 0 results.
@@ -361,20 +363,16 @@ IN_PROC_BROWSER_TEST_F(NotificationPlatformBridgeWinUITest, GetDisplayed) {
   bool incognito = true;
 
   Profile* profile1 = CreateTestingProfile("P1");
-  MockIToastNotification item1(GetToastString(L"P1i", L"P1", incognito),
-                               L"tag");
-  notifications.push_back(&item1);
-  MockIToastNotification item2(GetToastString(L"P1reg", L"P1", !incognito),
-                               L"tag");
-  notifications.push_back(&item2);
+  notifications.push_back(Microsoft::WRL::Make<MockIToastNotification>(
+      GetToastString(L"P1i", L"P1", incognito), L"tag"));
+  notifications.push_back(Microsoft::WRL::Make<MockIToastNotification>(
+      GetToastString(L"P1reg", L"P1", !incognito), L"tag"));
 
   Profile* profile2 = CreateTestingProfile("P2");
-  MockIToastNotification item3(GetToastString(L"P2i", L"P2", incognito),
-                               L"tag");
-  notifications.push_back(&item3);
-  MockIToastNotification item4(GetToastString(L"P2reg", L"P2", !incognito),
-                               L"tag");
-  notifications.push_back(&item4);
+  notifications.push_back(Microsoft::WRL::Make<MockIToastNotification>(
+      GetToastString(L"P2i", L"P2", incognito), L"tag"));
+  notifications.push_back(Microsoft::WRL::Make<MockIToastNotification>(
+      GetToastString(L"P2reg", L"P2", !incognito), L"tag"));
 
   // Query for profile P1 in incognito (should return 1 item).
   {

chrome/browser/notifications/notification_platform_bridge_win_unittest.cc

@@ -5,9 +5,11 @@
 #include "chrome/browser/notifications/notification_platform_bridge_win.h"
 
 #include <memory>
+#include <utility>
 
 #include <windows.ui.notifications.h>
 #include <wrl/client.h>
+#include <wrl/implements.h>
 
 #include "base/hash.h"
 #include "base/strings/string16.h"
@@ -200,7 +202,8 @@ TEST_F(NotificationPlatformBridgeWinTest, Suppress) {
 
   base::win::ScopedCOMInitializer com_initializer;
 
-  std::vector<winui::Notifications::IToastNotification*> notifications;
+  std::vector<mswr::ComPtr<winui::Notifications::IToastNotification>>
+      notifications;
   notification_platform_bridge_win_->SetDisplayedNotificationsForTesting(
       &notifications);
 
@@ -220,9 +223,10 @@ TEST_F(NotificationPlatformBridgeWinTest, Suppress) {
   // Register a single notification with a specific tag.
   std::string tag_data = std::string(kNotificationId) + "|" + kProfileId + "|0";
   base::string16 tag = base::UintToString16(base::Hash(tag_data));
-  MockIToastNotification item1(
-      L"<toast launch=\"0|0|Default|0|https://foo.com/|id\"></toast>", tag);
-  notifications.push_back(&item1);
+  // Microsoft::WRL::Make() requires MockIToastNotification to derive from
+  // RuntimeClass.
+  notifications.push_back(Microsoft::WRL::Make<MockIToastNotification>(
+      L"<toast launch=\"0|0|Default|0|https://foo.com/|id\"></toast>", tag));
 
   // Request this notification with renotify true (should not be suppressed).
   ASSERT_HRESULT_SUCCEEDED(GetToast(launch_id, /*renotify=*/true, kProfileId,

chrome/browser/notifications/win/mock_itoastnotification.cc

@@ -18,40 +18,6 @@ MockIToastNotification::MockIToastNotification(const base::string16& xml,
                                                const base::string16& tag)
     : xml_(xml), group_(L"Notifications"), tag_(tag) {}
 
-HRESULT MockIToastNotification::QueryInterface(REFIID riid, void** ppvObject) {
-  if (riid == ABI::Windows::UI::Notifications::IID_IToastNotification) {
-    AddRef();
-    *ppvObject = static_cast<IToastNotification*>(this);
-    return S_OK;
-  } else if (riid == ABI::Windows::UI::Notifications::IID_IToastNotification2) {
-    AddRef();
-    *ppvObject = static_cast<IToastNotification2*>(this);
-    return S_OK;
-  }
-
-  return E_NOINTERFACE;
-}
-
-ULONG MockIToastNotification::AddRef() {
-  return ++refcount_;
-}
-
-ULONG MockIToastNotification::Release() {
-  return --refcount_;
-}
-
-HRESULT MockIToastNotification::GetIids(ULONG* iidCount, IID** iids) {
-  return E_NOTIMPL;
-}
-
-HRESULT MockIToastNotification::GetRuntimeClassName(HSTRING* className) {
-  return E_NOTIMPL;
-}
-
-HRESULT MockIToastNotification::GetTrustLevel(TrustLevel* trustLevel) {
-  return E_NOTIMPL;
-}
-
 HRESULT MockIToastNotification::get_Content(winxml::Dom::IXmlDocument** value) {
   mswr::ComPtr<ABI::Windows::Data::Xml::Dom::IXmlDocumentIO> xml_document_io;
   base::win::ScopedHString id = base::win::ScopedHString::Create(

chrome/browser/notifications/win/mock_itoastnotification.h

@@ -7,25 +7,21 @@
 
 #include <windows.ui.notifications.h>
 
+#include <wrl/implements.h>
+
 #include "base/macros.h"
 #include "base/strings/string16.h"
 
 class MockIToastNotification
-    : public ABI::Windows::UI::Notifications::IToastNotification,
-      public ABI::Windows::UI::Notifications::IToastNotification2 {
+    : public Microsoft::WRL::RuntimeClass<
+          Microsoft::WRL::RuntimeClassFlags<
+              Microsoft::WRL::WinRt | Microsoft::WRL::InhibitRoOriginateError>,
+          ABI::Windows::UI::Notifications::IToastNotification,
+          ABI::Windows::UI::Notifications::IToastNotification2> {
  public:
   explicit MockIToastNotification(const base::string16& xml,
                                   const base::string16& tag);
-  ~MockIToastNotification() = default;
-
-  // IInspectable implementation:
-  HRESULT STDMETHODCALLTYPE QueryInterface(REFIID riid,
-                                           void** ppvObject) override;
-  ULONG STDMETHODCALLTYPE AddRef() override;
-  ULONG STDMETHODCALLTYPE Release() override;
-  HRESULT STDMETHODCALLTYPE GetIids(ULONG* iidCount, IID** iids) override;
-  HRESULT STDMETHODCALLTYPE GetRuntimeClassName(HSTRING* className) override;
-  HRESULT STDMETHODCALLTYPE GetTrustLevel(TrustLevel* trustLevel) override;
+  ~MockIToastNotification() override = default;
 
   // ABI::Windows::UI::Notifications::IToastNotification implementation:
   HRESULT STDMETHODCALLTYPE
@@ -67,8 +63,6 @@ class MockIToastNotification
   base::string16 group_;
   base::string16 tag_;
 
-  int refcount_ = 0;
-
   DISALLOW_COPY_AND_ASSIGN(MockIToastNotification);
 };