[Managed Session] Housekeeping on Managed Session platform delegates

Continuation of work from crrev.com/c/1202147, fixing up how delegates
are set and persisted (using the base::NoDestructor pattern).

Bug: 865947
Change-Id: Ib8768fe71e91c5f1bad400ec047b2c941a939608
Reviewed-on: https://chromium-review.googlesource.com/1210502Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Ivan Šandrk <isandrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589940}

chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc

@@ -170,13 +170,14 @@ bool GetUserLockAttributes(const user_manager::User* user,
 // not freed and they leak but that is fine.
 void SetPublicAccountDelegates() {
   extensions::PermissionsUpdater::SetPlatformDelegate(
-      new extensions::PermissionsUpdaterDelegateChromeOS);
+      std::make_unique<extensions::PermissionsUpdaterDelegateChromeOS>());
 
   extensions::ExtensionTabUtil::SetPlatformDelegate(
-      new extensions::ExtensionTabUtilDelegateChromeOS);
+      std::make_unique<extensions::ExtensionTabUtilDelegateChromeOS>());
 
   extensions::ActiveTabPermissionGranter::SetPlatformDelegate(
-      new extensions::ActiveTabPermissionGranterDelegateChromeOS);
+      std::make_unique<
+          extensions::ActiveTabPermissionGranterDelegateChromeOS>());
 }
 
 policy::MinimumVersionPolicyHandler* GetMinimumVersionPolicyHandler() {
@@ -232,9 +233,9 @@ ChromeUserManagerImpl::CreateChromeUserManager() {
 
 // static
 void ChromeUserManagerImpl::ResetPublicAccountDelegatesForTesting() {
-  delete extensions::PermissionsUpdater::SetPlatformDelegate(nullptr);
-  delete extensions::ExtensionTabUtil::SetPlatformDelegate(nullptr);
-  delete extensions::ActiveTabPermissionGranter::SetPlatformDelegate(nullptr);
+  extensions::PermissionsUpdater::SetPlatformDelegate(nullptr);
+  extensions::ExtensionTabUtil::SetPlatformDelegate(nullptr);
+  extensions::ActiveTabPermissionGranter::SetPlatformDelegate(nullptr);
 }
 
 ChromeUserManagerImpl::ChromeUserManagerImpl()

chrome/browser/extensions/active_tab_apitest.cc

@@ -75,8 +75,8 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, ActiveTab) {
   {
     // Setup state.
     chromeos::ScopedTestPublicSessionLoginState login_state;
-    auto delegate = std::make_unique<ExtensionTabUtilDelegateChromeOS>();
-    ExtensionTabUtil::SetPlatformDelegate(delegate.get());
+    ExtensionTabUtil::SetPlatformDelegate(
+        std::make_unique<ExtensionTabUtilDelegateChromeOS>());
 
     ExtensionTestMessageListener listener(false);
     ResultCatcher catcher;

chrome/browser/extensions/active_tab_permission_granter.cc

@@ -5,9 +5,11 @@
 #include "chrome/browser/extensions/active_tab_permission_granter.h"
 
 #include <set>
+#include <utility>
 #include <vector>
 
 #include "base/feature_list.h"
+#include "base/no_destructor.h"
 #include "chrome/browser/extensions/extension_action_runner.h"
 #include "chrome/browser/extensions/extension_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -71,16 +73,23 @@ void SendMessageToProcesses(
     tab_process->Send(create_message.Run(false));
 }
 
-ActiveTabPermissionGranter::Delegate* g_active_tab_permission_granter_delegate =
-    nullptr;
+std::unique_ptr<ActiveTabPermissionGranter::Delegate>& GetDelegateWrapper() {
+  static base::NoDestructor<
+      std::unique_ptr<ActiveTabPermissionGranter::Delegate>>
+      delegate_wrapper;
+  return *delegate_wrapper;
+}
+
+ActiveTabPermissionGranter::Delegate* GetDelegate() {
+  return GetDelegateWrapper().get();
+}
 
 // Returns true if activeTab is allowed to be granted to the extension. This can
 // return false for platform-specific implementations.
 bool ShouldGrantActiveTabOrPrompt(const Extension* extension,
                                   content::WebContents* web_contents) {
-  return !g_active_tab_permission_granter_delegate ||
-         g_active_tab_permission_granter_delegate->ShouldGrantActiveTabOrPrompt(
-             extension, web_contents);
+  return !GetDelegate() ||
+         GetDelegate()->ShouldGrantActiveTabOrPrompt(extension, web_contents);
 }
 
 }  // namespace
@@ -98,14 +107,9 @@ ActiveTabPermissionGranter::ActiveTabPermissionGranter(
 ActiveTabPermissionGranter::~ActiveTabPermissionGranter() {}
 
 // static
-ActiveTabPermissionGranter::Delegate*
-ActiveTabPermissionGranter::SetPlatformDelegate(Delegate* delegate) {
-  // Disallow setting it twice (but allow resetting - don't forget to free in
-  // that case).
-  CHECK(!g_active_tab_permission_granter_delegate || !delegate);
-  Delegate* previous_delegate = g_active_tab_permission_granter_delegate;
-  g_active_tab_permission_granter_delegate = delegate;
-  return previous_delegate;
+void ActiveTabPermissionGranter::SetPlatformDelegate(
+    std::unique_ptr<Delegate> delegate) {
+  GetDelegateWrapper() = std::move(delegate);
 }
 
 void ActiveTabPermissionGranter::GrantIfRequested(const Extension* extension) {

chrome/browser/extensions/active_tab_permission_granter.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_EXTENSIONS_ACTIVE_TAB_PERMISSION_GRANTER_H_
 #define CHROME_BROWSER_EXTENSIONS_ACTIVE_TAB_PERMISSION_GRANTER_H_
 
+#include <memory>
 #include <set>
 #include <string>
 
@@ -47,9 +48,8 @@ class ActiveTabPermissionGranter
                              Profile* profile);
   ~ActiveTabPermissionGranter() override;
 
-  // Platform specific delegate should be set during startup. |delegate| is a
-  // singleton instance and is leaked.
-  static Delegate* SetPlatformDelegate(Delegate* delegate);
+  // Platform specific delegate should be set during startup.
+  static void SetPlatformDelegate(std::unique_ptr<Delegate> delegate);
 
   // If |extension| has the activeTab or tabCapture permission, grants
   // tab-specific permissions to it until the next page navigation or refresh.

chrome/browser/extensions/active_tab_unittest.cc

@@ -464,17 +464,17 @@ TEST_F(ActiveTabTest, ChromeUrlGrants) {
 
 class ActiveTabDelegateTest : public ActiveTabTest {
  protected:
-  ActiveTabDelegateTest()
-      : test_delegate_(
-            std::make_unique<ActiveTabPermissionGranterTestDelegate>()) {
-    ActiveTabPermissionGranter::SetPlatformDelegate(test_delegate_.get());
+  ActiveTabDelegateTest() {
+    auto delegate = std::make_unique<ActiveTabPermissionGranterTestDelegate>();
+    test_delegate_ = delegate.get();
+    ActiveTabPermissionGranter::SetPlatformDelegate(std::move(delegate));
   }
 
   ~ActiveTabDelegateTest() override {
     ActiveTabPermissionGranter::SetPlatformDelegate(nullptr);
   }
 
-  std::unique_ptr<ActiveTabPermissionGranterTestDelegate> test_delegate_;
+  ActiveTabPermissionGranterTestDelegate* test_delegate_;
 };
 
 // Test that the custom platform delegate works as expected.

chrome/browser/extensions/extension_tab_util.cc

@@ -6,8 +6,10 @@
 
 #include <stddef.h>
 #include <algorithm>
+#include <utility>
 
 #include "base/macros.h"
+#include "base/no_destructor.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -104,7 +106,15 @@ int GetTabIdForExtensions(const WebContents* web_contents) {
   return SessionTabHelper::IdForTab(web_contents).id();
 }
 
-ExtensionTabUtil::Delegate* g_extension_tab_util_delegate = nullptr;
+std::unique_ptr<ExtensionTabUtil::Delegate>& GetDelegateWrapper() {
+  static base::NoDestructor<std::unique_ptr<ExtensionTabUtil::Delegate>>
+      delegate_wrapper;
+  return *delegate_wrapper;
+}
+
+ExtensionTabUtil::Delegate* GetDelegate() {
+  return GetDelegateWrapper().get();
+}
 
 }  // namespace
 
@@ -489,14 +499,8 @@ std::unique_ptr<api::tabs::MutedInfo> ExtensionTabUtil::CreateMutedInfo(
 }
 
 // static
-ExtensionTabUtil::Delegate* ExtensionTabUtil::SetPlatformDelegate(
-    Delegate* delegate) {
-  // Allow setting it only once (also allow reset to nullptr, but then take
-  // special care to free it).
-  CHECK(!g_extension_tab_util_delegate || !delegate);
-  Delegate* previous_delegate = g_extension_tab_util_delegate;
-  g_extension_tab_util_delegate = delegate;
-  return previous_delegate;
+void ExtensionTabUtil::SetPlatformDelegate(std::unique_ptr<Delegate> delegate) {
+  GetDelegateWrapper() = std::move(delegate);
 }
 
 // static
@@ -526,9 +530,8 @@ void ExtensionTabUtil::ScrubTabForExtension(const Extension* extension,
     tab->title.reset();
     tab->fav_icon_url.reset();
   }
-  if (g_extension_tab_util_delegate)
-    g_extension_tab_util_delegate->ScrubTabForExtension(extension, contents,
-                                                        tab);
+  if (GetDelegate())
+    GetDelegate()->ScrubTabForExtension(extension, contents, tab);
 }
 
 bool ExtensionTabUtil::GetTabStripModel(const WebContents* web_contents,

chrome/browser/extensions/extension_tab_util.h

@@ -135,8 +135,7 @@ class ExtensionTabUtil {
 
   // Platform specific logic moved to delegate. This should be set during
   // startup.
-  // |delegate| is never deleted by this class.
-  static Delegate* SetPlatformDelegate(Delegate* delegate);
+  static void SetPlatformDelegate(std::unique_ptr<Delegate> delegate);
 
   // Removes any privacy-sensitive fields from a Tab object if appropriate,
   // given the permissions of the extension and the tab in question.  The

chrome/browser/extensions/extension_tab_util_unittest.cc

@@ -35,8 +35,8 @@ class ExtensionTabUtilTestDelegate : public ExtensionTabUtil::Delegate {
 // Test that the custom ScrubTabForExtension delegate works - in this test it
 // sets URL to a custom string.
 TEST(ExtensionTabUtilTest, Delegate) {
-  auto test_delegate = std::make_unique<ExtensionTabUtilTestDelegate>();
-  ExtensionTabUtil::SetPlatformDelegate(test_delegate.get());
+  ExtensionTabUtil::SetPlatformDelegate(
+      std::make_unique<ExtensionTabUtilTestDelegate>());
 
   api::tabs::Tab tab;
   ExtensionTabUtil::ScrubTabForExtension(nullptr, nullptr, &tab);

chrome/browser/extensions/permissions_updater.cc

@@ -4,10 +4,12 @@
 
 #include "chrome/browser/extensions/permissions_updater.h"
 
+#include <set>
 #include <utility>
 
 #include "base/feature_list.h"
 #include "base/memory/ref_counted.h"
+#include "base/no_destructor.h"
 #include "base/values.h"
 #include "chrome/browser/extensions/api/permissions/permissions_api_helpers.h"
 #include "chrome/browser/extensions/extension_management.h"
@@ -72,7 +74,15 @@ std::unique_ptr<const PermissionSet> GetBoundedActivePermissions(
   return adjusted_active;
 }
 
-PermissionsUpdater::Delegate* g_delegate = nullptr;
+std::unique_ptr<PermissionsUpdater::Delegate>& GetDelegateWrapper() {
+  static base::NoDestructor<std::unique_ptr<PermissionsUpdater::Delegate>>
+      delegate_wrapper;
+  return *delegate_wrapper;
+}
+
+PermissionsUpdater::Delegate* GetDelegate() {
+  return GetDelegateWrapper().get();
+}
 
 }  // namespace
 
@@ -88,14 +98,9 @@ PermissionsUpdater::PermissionsUpdater(content::BrowserContext* browser_context,
 PermissionsUpdater::~PermissionsUpdater() {}
 
 // static
-PermissionsUpdater::Delegate* PermissionsUpdater::SetPlatformDelegate(
-    Delegate* delegate) {
-  // Make sure we're setting it only once (allow setting to nullptr, but then
-  // take special care of actually freeing it).
-  CHECK(!g_delegate || !delegate);
-  Delegate* previous_delegate = g_delegate;
-  g_delegate = delegate;
-  return previous_delegate;
+void PermissionsUpdater::SetPlatformDelegate(
+    std::unique_ptr<Delegate> delegate) {
+  GetDelegateWrapper() = std::move(delegate);
 }
 
 void PermissionsUpdater::GrantOptionalPermissions(
@@ -341,8 +346,8 @@ void PermissionsUpdater::InitializePermissions(const Extension* extension) {
   ScriptingPermissionsModifier::WithholdPermissionsIfNecessary(
       *extension, *prefs, *bounded_active, &granted_permissions);
 
-  if (g_delegate)
-    g_delegate->InitializePermissions(extension, &granted_permissions);
+  if (GetDelegate())
+    GetDelegate()->InitializePermissions(extension, &granted_permissions);
 
   bool update_active_permissions = false;
   if ((init_flag_ & INIT_FLAG_TRANSIENT) == 0) {

chrome/browser/extensions/permissions_updater.h

@@ -64,8 +64,7 @@ class PermissionsUpdater {
   // Sets a delegate to provide platform-specific logic. This should be set
   // during startup (to ensure all extensions are initialized through the
   // delegate).
-  // |delegate| is a singleton instance and is leaked.
-  static Delegate* SetPlatformDelegate(Delegate* delegate);
+  static void SetPlatformDelegate(std::unique_ptr<Delegate> delegate);
 
   // Grants |permissions| that were defined as optional in the manifest to
   // |extension|, updating the active permission set and notifying any

chrome/browser/extensions/permissions_updater_unittest.cc

@@ -443,8 +443,8 @@ TEST_F(PermissionsUpdaterTest, Delegate) {
           std::make_unique<base::ListValue>(), required_permissions.Build(),
           "My Extension");
 
-  auto test_delegate = std::make_unique<PermissionsUpdaterTestDelegate>();
-  PermissionsUpdater::SetPlatformDelegate(test_delegate.get());
+  PermissionsUpdater::SetPlatformDelegate(
+      std::make_unique<PermissionsUpdaterTestDelegate>());
   PermissionsUpdater updater(profile());
   updater.InitializePermissions(extension.get());