Make GlobalErrorService's ownership model slightly less insane.

GlobalErrorService ownership was crazy; sometimes the error objects contained were owned, and sometimes they weren't. This separates the two cases into clear statements of ownership, and deprecates the case where the error objects are unowned.

BUG=555865,673578

Review-Url: https://codereview.chromium.org/2409443002
Cr-Commit-Position: refs/heads/master@{#438192}

chrome/browser/extensions/extension_disabled_ui.cc

@@ -360,13 +360,14 @@ void ExtensionDisabledGlobalError::OnShutdown(
 }
 
 void ExtensionDisabledGlobalError::RemoveGlobalError() {
-  GlobalErrorServiceFactory::GetForProfile(service_->profile())
-      ->RemoveGlobalError(this);
+  std::unique_ptr<GlobalError> ptr =
+      GlobalErrorServiceFactory::GetForProfile(service_->profile())
+          ->RemoveGlobalError(this);
   registrar_.RemoveAll();
   registry_observer_.RemoveAll();
   // Delete this object after any running tasks, so that the extension dialog
   // still has it as a delegate to finish the current tasks.
-  base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, this);
+  base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, ptr.release());
 }
 
 // Globals --------------------------------------------------------------------
@@ -382,7 +383,7 @@ void AddExtensionDisabledErrorWithIcon(base::WeakPtr<ExtensionService> service,
   const Extension* extension = service->GetInstalledExtension(extension_id);
   if (extension) {
     GlobalErrorServiceFactory::GetForProfile(service->profile())
-        ->AddGlobalError(new ExtensionDisabledGlobalError(
+        ->AddGlobalError(base::MakeUnique<ExtensionDisabledGlobalError>(
             service.get(), extension, is_remote_install, icon));
   }
 }

chrome/browser/extensions/external_install_error.cc

@@ -313,7 +313,7 @@ ExternalInstallError::ExternalInstallError(
 
 ExternalInstallError::~ExternalInstallError() {
   if (global_error_.get())
-    error_service_->RemoveGlobalError(global_error_.get());
+    error_service_->RemoveUnownedGlobalError(global_error_.get());
 }
 
 void ExternalInstallError::OnInstallPromptDone(
@@ -439,7 +439,7 @@ void ExternalInstallError::OnDialogReady(
 
   if (alert_type_ == BUBBLE_ALERT) {
     global_error_.reset(new ExternalInstallBubbleAlert(this, prompt_.get()));
-    error_service_->AddGlobalError(global_error_.get());
+    error_service_->AddUnownedGlobalError(global_error_.get());
 
     if (!manager_->has_currently_visible_install_alert()) {
       // |browser| is nullptr during unit tests, so call
@@ -454,7 +454,7 @@ void ExternalInstallError::OnDialogReady(
   } else {
     DCHECK(alert_type_ == MENU_ALERT);
     global_error_.reset(new ExternalInstallMenuAlert(this));
-    error_service_->AddGlobalError(global_error_.get());
+    error_service_->AddUnownedGlobalError(global_error_.get());
   }
 }
 

chrome/browser/extensions/warning_badge_service.cc

@@ -7,6 +7,7 @@
 #include <stddef.h>
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/extensions/warning_badge_service_factory.h"
@@ -154,12 +155,10 @@ void WarningBadgeService::ShowBadge(bool show) {
       ErrorBadge::GetMenuItemCommandID());
 
   // Activate or hide the warning badge in case the current state is incorrect.
-  if (error && !show) {
+  if (error && !show)
     service->RemoveGlobalError(error);
-    delete error;
-  } else if (!error && show) {
-    service->AddGlobalError(new ErrorBadge(this));
-  }
+  else if (!error && show)
+    service->AddGlobalError(base::MakeUnique<ErrorBadge>(this));
 }
 
 }  // namespace extensions

chrome/browser/recovery/recovery_install_global_error.cc

@@ -23,7 +23,8 @@ RecoveryInstallGlobalError::RecoveryInstallGlobalError(Profile* profile)
         : elevation_needed_(false),
           profile_(profile),
           has_shown_bubble_view_(false) {
-  GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(this);
+  GlobalErrorServiceFactory::GetForProfile(profile_)->AddUnownedGlobalError(
+      this);
 
   PrefService* pref = g_browser_process->local_state();
   if (pref->FindPreference(prefs::kRecoveryComponentNeedsElevation)) {
@@ -46,7 +47,8 @@ RecoveryInstallGlobalError::~RecoveryInstallGlobalError() {
 }
 
 void RecoveryInstallGlobalError::Shutdown() {
-  GlobalErrorServiceFactory::GetForProfile(profile_)->RemoveGlobalError(this);
+  GlobalErrorServiceFactory::GetForProfile(profile_)->RemoveUnownedGlobalError(
+      this);
 }
 
 GlobalError::Severity RecoveryInstallGlobalError::GetSeverity() {

chrome/browser/safe_browsing/srt_fetcher_win.cc

@@ -18,6 +18,7 @@
 #include "base/debug/leak_annotations.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
@@ -544,7 +545,7 @@ void DisplaySRTPrompt(const base::FilePath& download_path) {
 
   // Ownership of |global_error| is passed to the service. The error removes
   // itself from the service and self-destructs when done.
-  global_error_service->AddGlobalError(global_error);
+  global_error_service->AddGlobalError(base::WrapUnique(global_error));
 
   bool show_bubble = true;
   PrefService* local_state = g_browser_process->local_state();

chrome/browser/safe_browsing/srt_global_error_win.cc

@@ -221,8 +221,8 @@ void SRTGlobalError::MaybeExecuteSRT() {
     return;
   }
   // At this point, this object owns itself, since ownership has been taken back
-  // from the global_error_service_ in the call to RemoveGlobalError. This means
-  // that it is safe to use base::Unretained here.
+  // from the global_error_service_ in the call to OnUserInteractionStarted.
+  // This means that it is safe to use base::Unretained here.
   BrowserThread::PostBlockingPoolTask(
       FROM_HERE,
       base::Bind(
@@ -262,7 +262,7 @@ void SRTGlobalError::OnUserinteractionStarted(
   RecordSRTPromptHistogram(histogram_value);
   interacted_ = true;
   if (global_error_service_) {
-    global_error_service_->RemoveGlobalError(this);
+    global_error_service_->RemoveGlobalError(this).release();
     global_error_service_ = nullptr;
   }
 }

chrome/browser/sync/sync_global_error.cc

@@ -30,7 +30,7 @@ SyncGlobalError::SyncGlobalError(
   DCHECK(sync_service_);
   error_controller_->AddObserver(this);
   if (!switches::IsMaterialDesignUserMenu())
-    global_error_service_->AddGlobalError(this);
+    global_error_service_->AddUnownedGlobalError(this);
 }
 
 SyncGlobalError::~SyncGlobalError() {
@@ -40,7 +40,7 @@ SyncGlobalError::~SyncGlobalError() {
 
 void SyncGlobalError::Shutdown() {
   if (!switches::IsMaterialDesignUserMenu())
-    global_error_service_->RemoveGlobalError(this);
+    global_error_service_->RemoveUnownedGlobalError(this);
   error_controller_->RemoveObserver(this);
   error_controller_ = nullptr;
 }

chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm

@@ -12,6 +12,7 @@
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -484,7 +485,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionButtonUiTest,
   // up the overflow container's bounds (crbug.com/511326).
   GlobalErrorService* error_service =
       GlobalErrorServiceFactory::GetForProfile(profile());
-  error_service->AddGlobalError(new MenuError());
+  error_service->AddGlobalError(base::MakeUnique<MenuError>());
 
   // It's probably excessive to test every level of the overflow here. Test
   // having all actions overflowed, some actions overflowed, and one action

chrome/browser/ui/global_error/global_error_service.cc

@@ -8,7 +8,6 @@
 
 #include <algorithm>
 
-#include "base/stl_util.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/global_error/global_error.h"
@@ -18,18 +17,32 @@
 GlobalErrorService::GlobalErrorService(Profile* profile) : profile_(profile) {
 }
 
-GlobalErrorService::~GlobalErrorService() {
-  base::STLDeleteElements(&errors_);
+GlobalErrorService::~GlobalErrorService() {}
+
+void GlobalErrorService::AddGlobalError(std::unique_ptr<GlobalError> error) {
+  DCHECK(error);
+  GlobalError* error_ptr = error.get();
+  owned_errors_[error_ptr] = std::move(error);
+  AddUnownedGlobalError(error_ptr);
 }
 
-void GlobalErrorService::AddGlobalError(GlobalError* error) {
+void GlobalErrorService::AddUnownedGlobalError(GlobalError* error) {
   DCHECK(error);
-  errors_.push_back(error);
+  all_errors_.push_back(error);
   NotifyErrorsChanged(error);
 }
 
-void GlobalErrorService::RemoveGlobalError(GlobalError* error) {
-  errors_.erase(std::find(errors_.begin(), errors_.end(), error));
+std::unique_ptr<GlobalError> GlobalErrorService::RemoveGlobalError(
+    GlobalError* error_ptr) {
+  std::unique_ptr<GlobalError> ptr = std::move(owned_errors_[error_ptr]);
+  owned_errors_.erase(error_ptr);
+  RemoveUnownedGlobalError(error_ptr);
+  return ptr;
+}
+
+void GlobalErrorService::RemoveUnownedGlobalError(GlobalError* error) {
+  DCHECK(owned_errors_.find(error) == owned_errors_.end());
+  all_errors_.erase(std::find(all_errors_.begin(), all_errors_.end(), error));
   GlobalErrorBubbleViewBase* bubble = error->GetBubbleView();
   if (bubble)
     bubble->CloseBubbleView();
@@ -38,23 +51,19 @@ void GlobalErrorService::RemoveGlobalError(GlobalError* error) {
 
 GlobalError* GlobalErrorService::GetGlobalErrorByMenuItemCommandID(
     int command_id) const {
-  for (GlobalErrorList::const_iterator
-       it = errors_.begin(); it != errors_.end(); ++it) {
-    GlobalError* error = *it;
+  for (const auto& error : all_errors_)
     if (error->HasMenuItem() && command_id == error->MenuItemCommandID())
       return error;
-  }
-  return NULL;
+
+  return nullptr;
 }
 
 GlobalError*
 GlobalErrorService::GetHighestSeverityGlobalErrorWithAppMenuItem() const {
   GlobalError::Severity highest_severity = GlobalError::SEVERITY_LOW;
-  GlobalError* highest_severity_error = NULL;
+  GlobalError* highest_severity_error = nullptr;
 
-  for (GlobalErrorList::const_iterator
-       it = errors_.begin(); it != errors_.end(); ++it) {
-    GlobalError* error = *it;
+  for (const auto& error : all_errors_) {
     if (error->HasMenuItem()) {
       if (!highest_severity_error || error->GetSeverity() > highest_severity) {
         highest_severity = error->GetSeverity();
@@ -67,13 +76,11 @@ GlobalErrorService::GetHighestSeverityGlobalErrorWithAppMenuItem() const {
 }
 
 GlobalError* GlobalErrorService::GetFirstGlobalErrorWithBubbleView() const {
-  for (GlobalErrorList::const_iterator
-       it = errors_.begin(); it != errors_.end(); ++it) {
-    GlobalError* error = *it;
+  for (const auto& error : all_errors_) {
     if (error->HasBubbleView() && !error->HasShownBubbleView())
       return error;
   }
-  return NULL;
+  return nullptr;
 }
 
 void GlobalErrorService::NotifyErrorsChanged(GlobalError* error) {
@@ -81,7 +88,7 @@ void GlobalErrorService::NotifyErrorsChanged(GlobalError* error) {
   // notifications to both it and its off-the-record profile to update
   // incognito windows as well.
   std::vector<Profile*> profiles_to_notify;
-  if (profile_ != NULL) {
+  if (profile_) {
     profiles_to_notify.push_back(profile_);
     if (profile_->IsOffTheRecord())
       profiles_to_notify.push_back(profile_->GetOriginalProfile());

chrome/browser/ui/global_error/global_error_service.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_GLOBAL_ERROR_GLOBAL_ERROR_SERVICE_H_
 #define CHROME_BROWSER_UI_GLOBAL_ERROR_GLOBAL_ERROR_SERVICE_H_
 
+#include <map>
+#include <memory>
 #include <vector>
 
 #include "base/macros.h"
@@ -21,7 +23,7 @@ class Profile;
 class GlobalErrorService : public KeyedService {
  public:
   // Type used to represent the list of currently active errors.
-  typedef std::vector<GlobalError*> GlobalErrorList;
+  using GlobalErrorList = std::vector<GlobalError*>;
 
   // Constructs a GlobalErrorService object for the given profile. The profile
   // maybe NULL for tests.
@@ -29,14 +31,24 @@ class GlobalErrorService : public KeyedService {
   ~GlobalErrorService() override;
 
   // Adds the given error to the list of global errors and displays it on
-  // browswer windows. If no browser windows are open  then the error is
-  // displayed once a browser window is opened. This class takes ownership of
-  // the error.
-  void AddGlobalError(GlobalError* error);
-
-  // Hides the given error and removes it from the list of global errors. Caller
-  // then takes ownership of the error and is responsible for deleting it.
-  void RemoveGlobalError(GlobalError* error);
+  // browser windows. If no browser windows are open then the error is
+  // displayed once a browser window is opened.
+  void AddGlobalError(std::unique_ptr<GlobalError> error);
+
+  // Hides the given error and removes it from the list of global errors.
+  // Ownership is returned to the caller.
+  std::unique_ptr<GlobalError> RemoveGlobalError(GlobalError* error);
+
+  // DEPRECATED; DO NOT USE!
+  // http://crbug.com/673578
+  //
+  // These functions allow adding and removing of a GlobalError that is not
+  // owned by the GlobalErrorService. In this case, the error's lifetime must
+  // exceed that of the GlobalErrorService.
+  //
+  // Do not add more uses of these functions, and remove existing uses.
+  void AddUnownedGlobalError(GlobalError* error);
+  void RemoveUnownedGlobalError(GlobalError* error);
 
   // Gets the error with the given command ID or NULL if no such error exists.
   // This class maintains ownership of the returned error.
@@ -51,7 +63,7 @@ class GlobalErrorService : public KeyedService {
   GlobalError* GetFirstGlobalErrorWithBubbleView() const;
 
   // Gets all errors.
-  const GlobalErrorList& errors() { return errors_; }
+  const GlobalErrorList& errors() { return all_errors_; }
 
   // Post a notification that a global error has changed and that the error UI
   // should update it self. Pass NULL for the given error to mean all error UIs
@@ -59,7 +71,8 @@ class GlobalErrorService : public KeyedService {
   void NotifyErrorsChanged(GlobalError* error);
 
  private:
-  GlobalErrorList errors_;
+  GlobalErrorList all_errors_;
+  std::map<GlobalError*, std::unique_ptr<GlobalError>> owned_errors_;
   Profile* profile_;
 
   DISALLOW_COPY_AND_ASSIGN(GlobalErrorService);

chrome/browser/ui/global_error/global_error_service_browsertest.cc

@@ -7,6 +7,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/global_error/global_error.h"
@@ -71,7 +72,7 @@ IN_PROC_BROWSER_TEST_F(GlobalErrorServiceBrowserTest, ShowBubbleView) {
 
   GlobalErrorService* service =
       GlobalErrorServiceFactory::GetForProfile(browser()->profile());
-  service->AddGlobalError(error);
+  service->AddGlobalError(base::WrapUnique(error));
 
   EXPECT_EQ(error, service->GetFirstGlobalErrorWithBubbleView());
   EXPECT_FALSE(error->HasShownBubbleView());
@@ -92,7 +93,7 @@ IN_PROC_BROWSER_TEST_F(GlobalErrorServiceBrowserTest, CloseBubbleView) {
 
   GlobalErrorService* service =
       GlobalErrorServiceFactory::GetForProfile(browser()->profile());
-  service->AddGlobalError(error);
+  service->AddGlobalError(base::WrapUnique(error));
 
   EXPECT_EQ(error, service->GetFirstGlobalErrorWithBubbleView());
   EXPECT_FALSE(error->HasShownBubbleView());
@@ -113,6 +114,10 @@ IN_PROC_BROWSER_TEST_F(GlobalErrorServiceBrowserTest, CloseBubbleView) {
 
 // Test that bubble is silently dismissed if it is showing when the GlobalError
 // instance is removed from the profile.
+//
+// This uses the deprecated "unowned" API to the GlobalErrorService to maintain
+// coverage. When those calls are eventually removed (http://crbug.com/673578)
+// these uses should be switched to the non-deprecated API.
 #if defined(OS_WIN) || defined(OS_LINUX)
 // http://crbug.com/396473
 #define MAYBE_BubbleViewDismissedOnRemove DISABLED_BubbleViewDismissedOnRemove
@@ -125,7 +130,7 @@ IN_PROC_BROWSER_TEST_F(GlobalErrorServiceBrowserTest,
 
   GlobalErrorService* service =
       GlobalErrorServiceFactory::GetForProfile(browser()->profile());
-  service->AddGlobalError(error.get());
+  service->AddUnownedGlobalError(error.get());
 
   EXPECT_EQ(error.get(), service->GetFirstGlobalErrorWithBubbleView());
   error->ShowBubbleView(browser());
@@ -135,8 +140,7 @@ IN_PROC_BROWSER_TEST_F(GlobalErrorServiceBrowserTest,
 
   // Removing |error| from profile should dismiss the bubble view without
   // calling |error->BubbleViewDidClose|.
-  service->RemoveGlobalError(error.get());
+  service->RemoveUnownedGlobalError(error.get());
   content::RunAllPendingInMessageLoop();
   EXPECT_EQ(1, error->bubble_view_close_count());
-  // |error| is no longer owned by service and will be deleted.
 }

chrome/browser/ui/global_error/global_error_service_unittest.cc

@@ -8,6 +8,7 @@
 
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "chrome/browser/ui/global_error/global_error.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -76,12 +77,12 @@ TEST(GlobalErrorServiceTest, AddError) {
   EXPECT_EQ(0u, service->errors().size());
 
   BaseError* error1 = new BaseError;
-  service->AddGlobalError(error1);
+  service->AddGlobalError(base::WrapUnique(error1));
   EXPECT_EQ(1u, service->errors().size());
   EXPECT_EQ(error1, service->errors()[0]);
 
   BaseError* error2 = new BaseError;
-  service->AddGlobalError(error2);
+  service->AddGlobalError(base::WrapUnique(error2));
   EXPECT_EQ(2u, service->errors().size());
   EXPECT_EQ(error1, service->errors()[0]);
   EXPECT_EQ(error2, service->errors()[1]);
@@ -96,18 +97,22 @@ TEST(GlobalErrorServiceTest, AddError) {
 TEST(GlobalErrorServiceTest, RemoveError) {
   std::unique_ptr<GlobalErrorService> service(new GlobalErrorService(NULL));
   BaseError error1;
-  service->AddGlobalError(&error1);
+  service->AddUnownedGlobalError(&error1);
   BaseError error2;
-  service->AddGlobalError(&error2);
+  service->AddUnownedGlobalError(&error2);
 
   EXPECT_EQ(2u, service->errors().size());
-  service->RemoveGlobalError(&error1);
+  service->RemoveUnownedGlobalError(&error1);
   EXPECT_EQ(1u, service->errors().size());
   EXPECT_EQ(&error2, service->errors()[0]);
-  service->RemoveGlobalError(&error2);
+  service->RemoveUnownedGlobalError(&error2);
   EXPECT_EQ(0u, service->errors().size());
 
   // Ensure that deleting the service does not delete the error objects.
+  //
+  // NB: If the service _does_ delete the error objects, then it called the
+  // delete operator on a stack-allocated object, which is undefined behavior,
+  // which we can't really use to prove anything. :(
   EXPECT_EQ(2, BaseError::count());
   service.reset();
   EXPECT_EQ(2, BaseError::count());
@@ -120,16 +125,16 @@ TEST(GlobalErrorServiceTest, GetMenuItem) {
   MenuError* error3 = new MenuError(3, GlobalError::SEVERITY_HIGH);
 
   GlobalErrorService service(NULL);
-  service.AddGlobalError(error1);
-  service.AddGlobalError(error2);
-  service.AddGlobalError(error3);
+  service.AddGlobalError(base::WrapUnique(error1));
+  service.AddGlobalError(base::WrapUnique(error2));
+  service.AddGlobalError(base::WrapUnique(error3));
 
   EXPECT_EQ(error2, service.GetGlobalErrorByMenuItemCommandID(2));
   EXPECT_EQ(error3, service.GetGlobalErrorByMenuItemCommandID(3));
   EXPECT_EQ(NULL, service.GetGlobalErrorByMenuItemCommandID(4));
 }
 
-// Test getting the error with the higest severity.
+// Test getting the error with the highest severity.
 TEST(GlobalErrorServiceTest, HighestSeverity) {
   MenuError* error1 = new MenuError(1, GlobalError::SEVERITY_LOW);
   MenuError* error2 = new MenuError(2, GlobalError::SEVERITY_MEDIUM);
@@ -138,18 +143,17 @@ TEST(GlobalErrorServiceTest, HighestSeverity) {
   GlobalErrorService service(NULL);
   EXPECT_EQ(NULL, service.GetHighestSeverityGlobalErrorWithAppMenuItem());
 
-  service.AddGlobalError(error1);
+  service.AddGlobalError(base::WrapUnique(error1));
   EXPECT_EQ(error1, service.GetHighestSeverityGlobalErrorWithAppMenuItem());
 
-  service.AddGlobalError(error2);
+  service.AddGlobalError(base::WrapUnique(error2));
   EXPECT_EQ(error2, service.GetHighestSeverityGlobalErrorWithAppMenuItem());
 
-  service.AddGlobalError(error3);
+  service.AddGlobalError(base::WrapUnique(error3));
   EXPECT_EQ(error3, service.GetHighestSeverityGlobalErrorWithAppMenuItem());
 
   // Remove the highest-severity error.
   service.RemoveGlobalError(error3);
-  delete error3;
 
   // Now error2 should be the next highest severity error.
   EXPECT_EQ(error2, service.GetHighestSeverityGlobalErrorWithAppMenuItem());

chrome/browser/ui/toolbar/app_menu_model_unittest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/toolbar/app_menu_model.h"
 
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/defaults.h"
 #include "chrome/browser/prefs/browser_prefs.h"
@@ -179,13 +180,11 @@ TEST_F(AppMenuModelTest, GlobalError) {
       GlobalErrorServiceFactory::GetForProfile(browser()->profile());
   ProfileOAuth2TokenServiceFactory::GetForProfile(browser()->profile());
   const int command1 = 1234567;
-  // AddGlobalError takes ownership of error1.
   MenuError* error1 = new MenuError(command1);
-  service->AddGlobalError(error1);
+  service->AddGlobalError(base::WrapUnique(error1));
   const int command2 = 1234568;
-  // AddGlobalError takes ownership of error2.
   MenuError* error2 = new MenuError(command2);
-  service->AddGlobalError(error2);
+  service->AddGlobalError(base::WrapUnique(error2));
 
   AppMenuModel model(this, browser());
   int index1 = model.GetIndexOfCommandId(command1);