Use IDType for permission change subscriptions.

Bug: 1025683
Change-Id: I3b44ba7833138e8a657a4192e1a36c978695db32
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791431
Reviewed-by: Richard Coles <torne@chromium.org>
Reviewed-by: Yuchen Liu <yucliu@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Fabrice de Gans-Riberi <fdegans@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Illia Klimov <elklm@google.com>
Auto-Submit: Balazs Engedy <engedy@chromium.org>
Commit-Queue: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867999}

android_webview/browser/aw_permission_manager.cc

@@ -470,16 +470,17 @@ PermissionStatus AwPermissionManager::GetPermissionStatusForFrame(
           .GetOrigin());
 }
 
-int AwPermissionManager::SubscribePermissionStatusChange(
+AwPermissionManager::SubscriptionId
+AwPermissionManager::SubscribePermissionStatusChange(
     PermissionType permission,
     content::RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
     base::RepeatingCallback<void(PermissionStatus)> callback) {
-  return content::PermissionController::kNoPendingOperation;
+  return SubscriptionId();
 }
 
 void AwPermissionManager::UnsubscribePermissionStatusChange(
-    int subscription_id) {}
+    SubscriptionId subscription_id) {}
 
 void AwPermissionManager::CancelPermissionRequest(int request_id) {
   PendingRequest* pending_request = pending_requests_.Lookup(request_id);

android_webview/browser/aw_permission_manager.h

@@ -49,13 +49,14 @@ class AwPermissionManager : public content::PermissionControllerDelegate {
       content::PermissionType permission,
       content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin) override;
-  int SubscribePermissionStatusChange(
+  SubscriptionId SubscribePermissionStatusChange(
       content::PermissionType permission,
       content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
       base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback)
       override;
-  void UnsubscribePermissionStatusChange(int subscription_id) override;
+  void UnsubscribePermissionStatusChange(
+      SubscriptionId subscription_id) override;
 
  protected:
   void CancelPermissionRequest(int request_id);

chrome/browser/permissions/permission_manager_browsertest.cc

@@ -49,13 +49,13 @@ class SubscriptionInterceptingPermissionManager
     callback_ = std::move(callback);
   }
 
-  int SubscribePermissionStatusChange(
+  SubscriptionId SubscribePermissionStatusChange(
       content::PermissionType permission,
       content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
       base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback)
       override {
-    int result =
+    SubscriptionId result =
         permissions::PermissionManager::SubscribePermissionStatusChange(
             permission, render_frame_host, requesting_origin, callback);
     std::move(callback_).Run();

chromecast/browser/cast_permission_manager.cc

@@ -63,17 +63,17 @@ CastPermissionManager::GetPermissionStatusForFrame(
   return blink::mojom::PermissionStatus::GRANTED;
 }
 
-int CastPermissionManager::SubscribePermissionStatusChange(
+CastPermissionManager::SubscriptionId
+CastPermissionManager::SubscribePermissionStatusChange(
     content::PermissionType permission,
     content::RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
     base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback) {
-  return content::PermissionController::kNoPendingOperation;
+  return SubscriptionId();
 }
 
 void CastPermissionManager::UnsubscribePermissionStatusChange(
-    int subscription_id) {
-}
+    SubscriptionId subscription_id) {}
 
 }  // namespace shell
 }  // namespace chromecast

chromecast/browser/cast_permission_manager.h

@@ -43,13 +43,14 @@ class CastPermissionManager : public content::PermissionControllerDelegate {
       content::PermissionType permission,
       content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin) override;
-  int SubscribePermissionStatusChange(
+  SubscriptionId SubscribePermissionStatusChange(
       content::PermissionType permission,
       content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
       base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback)
       override;
-  void UnsubscribePermissionStatusChange(int subscription_id) override;
+  void UnsubscribePermissionStatusChange(
+      SubscriptionId subscription_id) override;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CastPermissionManager);

components/permissions/permission_manager.cc

@@ -537,14 +537,15 @@ bool PermissionManager::IsPermissionOverridableByDevTools(
                                                             origin->GetURL());
 }
 
-int PermissionManager::SubscribePermissionStatusChange(
+PermissionManager::SubscriptionId
+PermissionManager::SubscribePermissionStatusChange(
     PermissionType permission,
     content::RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
     base::RepeatingCallback<void(PermissionStatus)> callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (is_shutting_down_)
-    return 0;
+    return SubscriptionId();
 
   if (subscriptions_.IsEmpty())
     PermissionsClient::Get()
@@ -581,16 +582,20 @@ int PermissionManager::SubscribePermissionStatusChange(
   subscription->callback =
       base::BindRepeating(&SubscriptionCallbackWrapper, std::move(callback));
 
-  return subscriptions_.Add(std::move(subscription));
+  auto id = subscription_id_generator_.GenerateNextId();
+  subscriptions_.AddWithID(std::move(subscription), id);
+  return id;
 }
 
-void PermissionManager::UnsubscribePermissionStatusChange(int subscription_id) {
+void PermissionManager::UnsubscribePermissionStatusChange(
+    SubscriptionId subscription_id) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (is_shutting_down_)
     return;
 
-  // Whether |subscription_id| is known will be checked by the Remove() call.
-  subscriptions_.Remove(subscription_id);
+  if (subscriptions_.Lookup(subscription_id)) {
+    subscriptions_.Remove(subscription_id);
+  }
 
   if (subscriptions_.IsEmpty()) {
     PermissionsClient::Get()

components/permissions/permission_manager.h

@@ -114,13 +114,14 @@ class PermissionManager : public KeyedService,
   bool IsPermissionOverridableByDevTools(
       content::PermissionType permission,
       const base::Optional<url::Origin>& origin) override;
-  int SubscribePermissionStatusChange(
+  SubscriptionId SubscribePermissionStatusChange(
       content::PermissionType permission,
       content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
       base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback)
       override;
-  void UnsubscribePermissionStatusChange(int subscription_id) override;
+  void UnsubscribePermissionStatusChange(
+      SubscriptionId subscription_id) override;
 
   // TODO(raymes): Rather than exposing this, use the denial reason from
   // GetPermissionStatus in callers to determine whether a permission is
@@ -153,7 +154,8 @@ class PermissionManager : public KeyedService,
   class PermissionResponseCallback;
 
   struct Subscription;
-  using SubscriptionsMap = base::IDMap<std::unique_ptr<Subscription>>;
+  using SubscriptionsMap =
+      base::IDMap<std::unique_ptr<Subscription>, SubscriptionId>;
 
   PermissionContextBase* GetPermissionContext(ContentSettingsType type);
 
@@ -185,6 +187,7 @@ class PermissionManager : public KeyedService,
   content::BrowserContext* browser_context_;
   PendingRequestsMap pending_requests_;
   SubscriptionsMap subscriptions_;
+  SubscriptionId::Generator subscription_id_generator_;
 
   PermissionContextMap permission_contexts_;
   using ContentSettingsTypeOverrides =

components/permissions/permission_manager_unittest.cc

@@ -343,7 +343,7 @@ TEST_F(PermissionManagerTest, SubscriptionDestroyedCleanlyWithoutUnsubscribe) {
 }
 
 TEST_F(PermissionManagerTest, SubscribeUnsubscribeAfterShutdown) {
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -358,7 +358,7 @@ TEST_F(PermissionManagerTest, SubscribeUnsubscribeAfterShutdown) {
       subscription_id);
 
   // Check that subscribe/unsubscribe after shutdown don't crash.
-  int subscription2_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription2_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -369,7 +369,7 @@ TEST_F(PermissionManagerTest, SubscribeUnsubscribeAfterShutdown) {
 }
 
 TEST_F(PermissionManagerTest, SameTypeChangeNotifies) {
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -386,7 +386,7 @@ TEST_F(PermissionManagerTest, SameTypeChangeNotifies) {
 }
 
 TEST_F(PermissionManagerTest, DifferentTypeChangeDoesNotNotify) {
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -402,7 +402,7 @@ TEST_F(PermissionManagerTest, DifferentTypeChangeDoesNotNotify) {
 }
 
 TEST_F(PermissionManagerTest, ChangeAfterUnsubscribeDoesNotNotify) {
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -418,7 +418,7 @@ TEST_F(PermissionManagerTest, ChangeAfterUnsubscribeDoesNotNotify) {
 }
 
 TEST_F(PermissionManagerTest, DifferentPrimaryUrlDoesNotNotify) {
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -435,7 +435,7 @@ TEST_F(PermissionManagerTest, DifferentPrimaryUrlDoesNotNotify) {
 }
 
 TEST_F(PermissionManagerTest, DifferentSecondaryUrlDoesNotNotify) {
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::STORAGE_ACCESS_GRANT, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -452,7 +452,7 @@ TEST_F(PermissionManagerTest, DifferentSecondaryUrlDoesNotNotify) {
 }
 
 TEST_F(PermissionManagerTest, WildCardPatternNotifies) {
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -472,7 +472,7 @@ TEST_F(PermissionManagerTest, ClearSettingsNotifies) {
   GetHostContentSettingsMap()->SetContentSettingDefaultScope(
       url(), url(), ContentSettingsType::GEOLOCATION, CONTENT_SETTING_ALLOW);
 
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -489,7 +489,7 @@ TEST_F(PermissionManagerTest, ClearSettingsNotifies) {
 }
 
 TEST_F(PermissionManagerTest, NewValueCorrectlyPassed) {
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -509,7 +509,7 @@ TEST_F(PermissionManagerTest, ChangeWithoutPermissionChangeDoesNotNotify) {
   GetHostContentSettingsMap()->SetContentSettingDefaultScope(
       url(), url(), ContentSettingsType::GEOLOCATION, CONTENT_SETTING_ALLOW);
 
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -528,7 +528,7 @@ TEST_F(PermissionManagerTest, ChangesBackAndForth) {
   GetHostContentSettingsMap()->SetContentSettingDefaultScope(
       url(), url(), ContentSettingsType::GEOLOCATION, CONTENT_SETTING_ASK);
 
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -556,7 +556,7 @@ TEST_F(PermissionManagerTest, ChangesBackAndForthWorker) {
   GetHostContentSettingsMap()->SetContentSettingDefaultScope(
       url(), url(), ContentSettingsType::GEOLOCATION, CONTENT_SETTING_ASK);
 
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, nullptr, url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -581,7 +581,7 @@ TEST_F(PermissionManagerTest, ChangesBackAndForthWorker) {
 }
 
 TEST_F(PermissionManagerTest, SubscribeMIDIPermission) {
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::MIDI, main_rfh(), url(),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,
@@ -793,7 +793,7 @@ TEST_F(PermissionManagerTest, SubscribeWithPermissionDelegation) {
   content::RenderFrameHost* parent = main_rfh();
   content::RenderFrameHost* child = AddChildRFH(parent, kOrigin2);
 
-  int subscription_id =
+  content::PermissionControllerDelegate::SubscriptionId subscription_id =
       GetPermissionControllerDelegate()->SubscribePermissionStatusChange(
           PermissionType::GEOLOCATION, child, GURL(kOrigin2),
           base::BindRepeating(&PermissionManagerTest::OnPermissionChange,

content/browser/android/nfc_host.cc

@@ -50,7 +50,7 @@ void NFCHost::GetNFC(RenderFrameHost* render_frame_host,
     return;
   }
 
-  if (subscription_id_ == PermissionController::kNoPendingOperation) {
+  if (!subscription_id_) {
     // base::Unretained() is safe here because the subscription is canceled when
     // this object is destroyed.
     subscription_id_ = permission_controller_->SubscribePermissionStatusChange(
@@ -106,10 +106,8 @@ void NFCHost::OnPermissionStatusChange(blink::mojom::PermissionStatus status) {
 
 void NFCHost::Close() {
   nfc_provider_.reset();
-  if (subscription_id_ != PermissionController::kNoPendingOperation) {
-    permission_controller_->UnsubscribePermissionStatusChange(subscription_id_);
-    subscription_id_ = PermissionController::kNoPendingOperation;
-  }
+  permission_controller_->UnsubscribePermissionStatusChange(subscription_id_);
+  subscription_id_ = PermissionController::SubscriptionId();
 }
 
 }  // namespace content

content/browser/android/nfc_host.h

@@ -44,7 +44,7 @@ class NFCHost : public WebContentsObserver {
   mojo::Remote<device::mojom::NFCProvider> nfc_provider_;
 
   // Permission change subscription ID provided by |permission_controller_|.
-  int subscription_id_ = PermissionController::kNoPendingOperation;
+  PermissionController::SubscriptionId subscription_id_;
 
   DISALLOW_COPY_AND_ASSIGN(NFCHost);
 };

content/browser/android/nfc_host_unittest.cc

@@ -48,7 +48,7 @@ class NFCHostTest : public RenderViewHostImplTestHarness {
 };
 
 TEST_F(NFCHostTest, GetNFCTwice) {
-  constexpr int kSubscriptionId = 42;
+  constexpr MockPermissionManager::SubscriptionId kSubscriptionId(42);
 
   NavigateAndCommit(GURL(kTestUrl));
 

content/browser/permissions/permission_controller_impl.cc

@@ -132,7 +132,8 @@ struct PermissionControllerImpl::Subscription {
   int render_frame_id = -1;
   int render_process_id = -1;
   base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback;
-  int delegate_subscription_id;
+  // This is default-initialized to an invalid ID.
+  PermissionControllerDelegate::SubscriptionId delegate_subscription_id;
 };
 
 PermissionControllerImpl::~PermissionControllerImpl() {
@@ -388,7 +389,8 @@ void PermissionControllerImpl::OnDelegatePermissionStatusChange(
     subscription->callback.Run(status);
 }
 
-int PermissionControllerImpl::SubscribePermissionStatusChange(
+PermissionControllerImpl::SubscriptionId
+PermissionControllerImpl::SubscribePermissionStatusChange(
     PermissionType permission,
     RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
@@ -422,21 +424,21 @@ int PermissionControllerImpl::SubscribePermissionStatusChange(
             base::BindRepeating(
                 &PermissionControllerImpl::OnDelegatePermissionStatusChange,
                 base::Unretained(this), subscription.get()));
-  } else {
-    subscription->delegate_subscription_id = kNoPendingOperation;
   }
-  return subscriptions_.Add(std::move(subscription));
+
+  auto id = subscription_id_generator_.GenerateNextId();
+  subscriptions_.AddWithID(std::move(subscription), id);
+  return id;
 }
 
 void PermissionControllerImpl::UnsubscribePermissionStatusChange(
-    int subscription_id) {
+    SubscriptionId subscription_id) {
   Subscription* subscription = subscriptions_.Lookup(subscription_id);
   if (!subscription)
     return;
   PermissionControllerDelegate* delegate =
       browser_context_->GetPermissionControllerDelegate();
-  if (delegate &&
-      subscription->delegate_subscription_id != kNoPendingOperation) {
+  if (delegate) {
     delegate->UnsubscribePermissionStatusChange(
         subscription->delegate_subscription_id);
   }