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);
   }

content/browser/permissions/permission_controller_impl.h

@@ -72,18 +72,19 @@ class CONTENT_EXPORT PermissionControllerImpl : public PermissionController {
                        const GURL& requesting_origin,
                        const GURL& embedding_origin);
 
-  int SubscribePermissionStatusChange(
+  SubscriptionId SubscribePermissionStatusChange(
       PermissionType permission,
       RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
       const base::RepeatingCallback<void(blink::mojom::PermissionStatus)>&
           callback);
 
-  void UnsubscribePermissionStatusChange(int subscription_id);
+  void UnsubscribePermissionStatusChange(SubscriptionId subscription_id);
 
  private:
   struct Subscription;
-  using SubscriptionsMap = base::IDMap<std::unique_ptr<Subscription>>;
+  using SubscriptionsMap =
+      base::IDMap<std::unique_ptr<Subscription>, SubscriptionId>;
   using SubscriptionsStatusMap =
       base::flat_map<SubscriptionsMap::KeyType, blink::mojom::PermissionStatus>;
 
@@ -98,7 +99,13 @@ class CONTENT_EXPORT PermissionControllerImpl : public PermissionController {
       const base::Optional<url::Origin>& origin);
 
   DevToolsPermissionOverrides devtools_permission_overrides_;
+
+  // Note that SubscriptionId is distinct from
+  // PermissionControllerDelegate::SubscriptionId, and the concrete ID values
+  // may be different as well.
   SubscriptionsMap subscriptions_;
+  SubscriptionId::Generator subscription_id_generator_;
+
   BrowserContext* browser_context_;
 
   DISALLOW_COPY_AND_ASSIGN(PermissionControllerImpl);

content/browser/permissions/permission_service_context.cc

@@ -11,7 +11,6 @@
 #include "content/browser/permissions/permission_service_impl.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_handle.h"
-#include "content/public/browser/permission_controller.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
@@ -32,7 +31,7 @@ class PermissionServiceContext::PermissionSubscription {
   PermissionSubscription& operator=(const PermissionSubscription&) = delete;
 
   ~PermissionSubscription() {
-    DCHECK_NE(id_, 0);
+    DCHECK(id_);
     BrowserContext* browser_context = context_->GetBrowserContext();
     if (browser_context) {
       PermissionControllerImpl::FromBrowserContext(browser_context)
@@ -41,7 +40,7 @@ class PermissionServiceContext::PermissionSubscription {
   }
 
   void OnConnectionError() {
-    DCHECK_NE(id_, 0);
+    DCHECK(id_);
     context_->ObserverHadConnectionError(id_);
   }
 
@@ -49,12 +48,12 @@ class PermissionServiceContext::PermissionSubscription {
     observer_->OnPermissionStatusChange(status);
   }
 
-  void set_id(int id) { id_ = id; }
+  void set_id(PermissionController::SubscriptionId id) { id_ = id; }
 
  private:
   PermissionServiceContext* const context_;
   mojo::Remote<blink::mojom::PermissionObserver> observer_;
-  int id_ = 0;
+  PermissionController::SubscriptionId id_;
 };
 
 RENDER_DOCUMENT_HOST_USER_DATA_KEY_IMPL(PermissionServiceContext)
@@ -104,7 +103,7 @@ void PermissionServiceContext::CreateSubscription(
   }
 
   GURL requesting_origin(origin.Serialize());
-  int subscription_id =
+  auto subscription_id =
       PermissionControllerImpl::FromBrowserContext(browser_context)
           ->SubscribePermissionStatusChange(
               permission_type, render_frame_host_, requesting_origin,
@@ -115,7 +114,8 @@ void PermissionServiceContext::CreateSubscription(
   subscriptions_[subscription_id] = std::move(subscription);
 }
 
-void PermissionServiceContext::ObserverHadConnectionError(int subscription_id) {
+void PermissionServiceContext::ObserverHadConnectionError(
+    PermissionController::SubscriptionId subscription_id) {
   size_t erased = subscriptions_.erase(subscription_id);
   DCHECK_EQ(1u, erased);
 }

content/browser/permissions/permission_service_context.h

@@ -9,6 +9,7 @@
 #include <unordered_map>
 
 #include "content/common/content_export.h"
+#include "content/public/browser/permission_controller.h"
 #include "content/public/browser/permission_type.h"
 #include "content/public/browser/render_document_host_user_data.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
@@ -58,7 +59,8 @@ class CONTENT_EXPORT PermissionServiceContext
       mojo::PendingRemote<blink::mojom::PermissionObserver> observer);
 
   // Called when the connection to a PermissionObserver has an error.
-  void ObserverHadConnectionError(int subscription_id);
+  void ObserverHadConnectionError(
+      PermissionController::SubscriptionId subscription_id);
 
   // May return nullptr during teardown, or when showing an interstitial.
   BrowserContext* GetBrowserContext() const;
@@ -81,7 +83,8 @@ class CONTENT_EXPORT PermissionServiceContext
   RenderFrameHost* const render_frame_host_;
   RenderProcessHost* const render_process_host_;
   mojo::UniqueReceiverSet<blink::mojom::PermissionService> services_;
-  std::unordered_map<int, std::unique_ptr<PermissionSubscription>>
+  std::unordered_map<PermissionController::SubscriptionId,
+                     std::unique_ptr<PermissionSubscription>>
       subscriptions_;
 };
 

content/browser/renderer_host/media/media_stream_manager.cc

@@ -676,9 +676,9 @@ class MediaStreamManager::DeviceRequest {
 
   std::string tab_capture_device_id;
 
-  int audio_subscription_id = PermissionControllerImpl::kNoPendingOperation;
+  PermissionController::SubscriptionId audio_subscription_id;
 
-  int video_subscription_id = PermissionControllerImpl::kNoPendingOperation;
+  PermissionController::SubscriptionId video_subscription_id;
 
  private:
   std::vector<MediaRequestState> state_;
@@ -2733,8 +2733,8 @@ void MediaStreamManager::SubscribeToPermissionControllerOnUIThread(
   if (!controller)
     return;
 
-  int audio_subscription_id = PermissionControllerImpl::kNoPendingOperation;
-  int video_subscription_id = PermissionControllerImpl::kNoPendingOperation;
+  PermissionController::SubscriptionId audio_subscription_id;
+  PermissionController::SubscriptionId video_subscription_id;
 
   if (is_audio_request) {
     // It is safe to bind base::Unretained(this) because MediaStreamManager is
@@ -2776,8 +2776,8 @@ void MediaStreamManager::SetPermissionSubscriptionIDs(
     const std::string& label,
     int requesting_process_id,
     int requesting_frame_id,
-    int audio_subscription_id,
-    int video_subscription_id) {
+    PermissionController::SubscriptionId audio_subscription_id,
+    PermissionController::SubscriptionId video_subscription_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   DeviceRequest* const request = FindRequest(label);
@@ -2804,8 +2804,8 @@ void MediaStreamManager::SetPermissionSubscriptionIDs(
 void MediaStreamManager::UnsubscribeFromPermissionControllerOnUIThread(
     int requesting_process_id,
     int requesting_frame_id,
-    int audio_subscription_id,
-    int video_subscription_id) {
+    PermissionController::SubscriptionId audio_subscription_id,
+    PermissionController::SubscriptionId video_subscription_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   PermissionControllerImpl* controller =

content/browser/renderer_host/media/media_stream_manager.h

@@ -50,6 +50,7 @@
 #include "content/public/browser/desktop_media_id.h"
 #include "content/public/browser/media_request_state.h"
 #include "content/public/browser/media_stream_request.h"
+#include "content/public/browser/permission_controller.h"
 #include "media/base/video_facing.h"
 #include "third_party/blink/public/common/mediastream/media_devices.h"
 #include "third_party/blink/public/common/mediastream/media_stream_controls.h"
@@ -570,19 +571,20 @@ class CONTENT_EXPORT MediaStreamManager
 
   // Store the subscription ids on a DeviceRequest in order to allow
   // unsubscribing when the request is deleted.
-  void SetPermissionSubscriptionIDs(const std::string& label,
-                                    int requesting_process_id,
-                                    int requesting_frame_id,
-                                    int audio_subscription_id,
-                                    int video_subscription_id);
+  void SetPermissionSubscriptionIDs(
+      const std::string& label,
+      int requesting_process_id,
+      int requesting_frame_id,
+      PermissionController::SubscriptionId audio_subscription_id,
+      PermissionController::SubscriptionId video_subscription_id);
 
   // Unsubscribe from following permission updates for the two specified
   // subscription IDs. Called when a request is deleted.
   static void UnsubscribeFromPermissionControllerOnUIThread(
       int requesting_process_id,
       int requesting_frame_id,
-      int audio_subscription_id,
-      int video_subscription_id);
+      PermissionController::SubscriptionId audio_subscription_id,
+      PermissionController::SubscriptionId video_subscription_id);
 
   // Callback that the PermissionController calls when a permission is updated.
   void PermissionChangedCallback(int requesting_process_id,

content/public/browser/permission_controller.h

@@ -6,6 +6,7 @@
 #define CONTENT_PUBLIC_BROWSER_PERMISSION_CONTROLLER_H_
 
 #include "base/supports_user_data.h"
+#include "base/util/type_safety/id_type.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/permission_type.h"
 #include "third_party/blink/public/mojom/permissions/permission_status.mojom.h"
@@ -20,8 +21,13 @@ class RenderFrameHost;
 class CONTENT_EXPORT PermissionController
     : public base::SupportsUserData::Data {
  public:
-  // Constant retured when registering and subscribing if
-  // cancelling/unsubscribing at a later stage would have no effect.
+  // Identifier for an active subscription. This is intentionally a distinct
+  // type from PermissionControllerDelegate::SubscriptionId as the concrete
+  // identifier values may be different.
+  using SubscriptionId = util::IdType64<PermissionController>;
+
+  // Constant returned when requesting a permission if cancelling at a later
+  // stage would have no effect.
   static const int kNoPendingOperation = -1;
 
   ~PermissionController() override {}
@@ -48,4 +54,17 @@ class CONTENT_EXPORT PermissionController
 
 }  // namespace content
 
+namespace std {
+
+template <>
+struct hash<content::PermissionController::SubscriptionId> {
+  std::size_t operator()(
+      const content::PermissionController::SubscriptionId& v) const {
+    content::PermissionController::SubscriptionId::Hasher hasher;
+    return hasher(v);
+  }
+};
+
+}  // namespace std
+
 #endif  // CONTENT_PUBLIC_BROWSER_PERMISSION_CONTROLLER_H_

content/public/browser/permission_controller_delegate.h

@@ -5,6 +5,7 @@
 #ifndef CONTENT_PUBLIC_BROWSER_PERMISSION_CONTROLLER_DELEGATE_H_
 #define CONTENT_PUBLIC_BROWSER_PERMISSION_CONTROLLER_DELEGATE_H_
 
+#include "base/util/type_safety/id_type.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/devtools_permission_overrides.h"
 #include "third_party/blink/public/mojom/permissions/permission_status.mojom.h"
@@ -18,6 +19,10 @@ class RenderFrameHost;
 class CONTENT_EXPORT PermissionControllerDelegate {
  public:
   using PermissionOverrides = DevToolsPermissionOverrides::PermissionOverrides;
+
+  // Identifier for an active subscription.
+  using SubscriptionId = util::IdType64<PermissionControllerDelegate>;
+
   virtual ~PermissionControllerDelegate() = default;
 
   // Requests a permission on behalf of a frame identified by
@@ -80,21 +85,21 @@ class CONTENT_EXPORT PermissionControllerDelegate {
 
   // Runs the given |callback| whenever the |permission| associated with the
   // given RenderFrameHost changes. A nullptr should be passed if the request
-  // is from a worker. Returns the subscription_id to be used to unsubscribe.
-  // Can be kNoPendingOperation if the subscribe was not successful.
-  virtual int SubscribePermissionStatusChange(
+  // is from a worker. Returns the ID to be used to unsubscribe, which can be
+  // `is_null()` if the subscribe was not successful.
+  virtual SubscriptionId SubscribePermissionStatusChange(
       content::PermissionType permission,
       content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin,
       base::RepeatingCallback<void(blink::mojom::PermissionStatus)>
           callback) = 0;
 
-  // Unregisters from permission status change notifications.
-  // The |subscription_id| must match the value returned by the
-  // SubscribePermissionStatusChange call. Unsubscribing
-  // an already unsubscribed |subscription_id| or providing the
-  // |subscription_id| kNoPendingOperation is a no-op.
-  virtual void UnsubscribePermissionStatusChange(int subscription_id) = 0;
+  // Unregisters from permission status change notifications. The
+  // |subscription_id| must match the value returned by the
+  // SubscribePermissionStatusChange call. Unsubscribing an already
+  // unsubscribed |subscription_id| or an `is_null()` ID is a no-op.
+  virtual void UnsubscribePermissionStatusChange(
+      SubscriptionId subscription_id) = 0;
 
   // Manually overrides default permission settings of delegate, if overrides
   // are tracked by the delegate. This method should only be called by the
@@ -116,4 +121,17 @@ class CONTENT_EXPORT PermissionControllerDelegate {
 
 }  // namespace content
 
+namespace std {
+
+template <>
+struct hash<content::PermissionControllerDelegate::SubscriptionId> {
+  std::size_t operator()(
+      const content::PermissionControllerDelegate::SubscriptionId& v) const {
+    content::PermissionControllerDelegate::SubscriptionId::Hasher hasher;
+    return hasher(v);
+  }
+};
+
+}  // namespace std
+
 #endif  // CONTENT_PUBLIC_BROWSER_PERMISSION_CONTROLLER_DELEGATE_H_

content/public/test/mock_permission_manager.h

@@ -50,12 +50,14 @@ class MockPermissionManager : public PermissionControllerDelegate {
                        const GURL& requesting_origin,
                        const GURL& embedding_origin) override {}
   MOCK_METHOD4(SubscribePermissionStatusChange,
-               int(PermissionType permission,
+               SubscriptionId(
+                   PermissionType permission,
                    RenderFrameHost* render_frame_host,
                    const GURL& requesting_origin,
                    base::RepeatingCallback<void(blink::mojom::PermissionStatus)>
                        callback));
-  MOCK_METHOD1(UnsubscribePermissionStatusChange, void(int subscription_id));
+  MOCK_METHOD1(UnsubscribePermissionStatusChange,
+               void(SubscriptionId subscription_id));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockPermissionManager);

content/shell/browser/shell_permission_manager.cc

@@ -134,16 +134,16 @@ ShellPermissionManager::GetPermissionStatusForFrame(
           .GetOrigin());
 }
 
-int ShellPermissionManager::SubscribePermissionStatusChange(
+ShellPermissionManager::SubscriptionId
+ShellPermissionManager::SubscribePermissionStatusChange(
     PermissionType permission,
     RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
     base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback) {
-  return PermissionController::kNoPendingOperation;
+  return SubscriptionId();
 }
 
 void ShellPermissionManager::UnsubscribePermissionStatusChange(
-    int subscription_id) {
-}
+    SubscriptionId subscription_id) {}
 
 }  // namespace content

content/shell/browser/shell_permission_manager.h

@@ -42,13 +42,14 @@ class ShellPermissionManager : public PermissionControllerDelegate {
       content::PermissionType permission,
       content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin) override;
-  int SubscribePermissionStatusChange(
+  SubscriptionId SubscribePermissionStatusChange(
       PermissionType permission,
       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(ShellPermissionManager);

content/web_test/browser/web_test_permission_manager.cc

@@ -147,7 +147,8 @@ WebTestPermissionManager::GetPermissionStatusForFrame(
           .GetOrigin());
 }
 
-int WebTestPermissionManager::SubscribePermissionStatusChange(
+WebTestPermissionManager::SubscriptionId
+WebTestPermissionManager::SubscribePermissionStatusChange(
     PermissionType permission,
     RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
@@ -170,14 +171,18 @@ int WebTestPermissionManager::SubscribePermissionStatusChange(
       GetPermissionStatus(permission, subscription->permission.origin,
                           subscription->permission.embedding_origin);
 
-  return subscriptions_.Add(std::move(subscription));
+  auto id = subscription_id_generator_.GenerateNextId();
+  subscriptions_.AddWithID(std::move(subscription), id);
+  return id;
 }
 
 void WebTestPermissionManager::UnsubscribePermissionStatusChange(
-    int subscription_id) {
+    SubscriptionId subscription_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  // Whether |subscription_id| is known will be checked by the Remove() call.
+  if (!subscriptions_.Lookup(subscription_id))
+    return;
+
   subscriptions_.Remove(subscription_id);
 }
 

content/web_test/browser/web_test_permission_manager.h

@@ -52,13 +52,14 @@ class WebTestPermissionManager
       content::PermissionType permission,
       content::RenderFrameHost* render_frame_host,
       const GURL& requesting_origin) override;
-  int SubscribePermissionStatusChange(
+  SubscriptionId SubscribePermissionStatusChange(
       PermissionType permission,
       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;
 
   void SetPermission(PermissionType permission,
                      blink::mojom::PermissionStatus status,
@@ -98,7 +99,8 @@ class WebTestPermissionManager
   };
 
   struct Subscription;
-  using SubscriptionsMap = base::IDMap<std::unique_ptr<Subscription>>;
+  using SubscriptionsMap =
+      base::IDMap<std::unique_ptr<Subscription>, SubscriptionId>;
   using PermissionsMap = std::unordered_map<PermissionDescription,
                                             blink::mojom::PermissionStatus,
                                             PermissionDescription::Hash>;
@@ -116,6 +118,7 @@ class WebTestPermissionManager
 
   // List of subscribers currently listening to permission changes.
   SubscriptionsMap subscriptions_;
+  SubscriptionId::Generator subscription_id_generator_;
 
   mojo::ReceiverSet<blink::test::mojom::PermissionAutomation> receivers_;
 

fuchsia/engine/browser/web_engine_permission_delegate.cc

@@ -85,20 +85,21 @@ WebEnginePermissionDelegate::GetPermissionStatusForFrame(
       permission, url::Origin::Create(requesting_origin));
 }
 
-int WebEnginePermissionDelegate::SubscribePermissionStatusChange(
+WebEnginePermissionDelegate::SubscriptionId
+WebEnginePermissionDelegate::SubscribePermissionStatusChange(
     content::PermissionType permission,
     content::RenderFrameHost* render_frame_host,
     const GURL& requesting_origin,
     base::RepeatingCallback<void(blink::mojom::PermissionStatus)> callback) {
   // TODO(crbug.com/1063094): Implement permission status subscription. It's
   // used in blink to emit PermissionStatus.onchange notifications.
-  NOTIMPLEMENTED() << ": " << static_cast<int>(permission);
-  return content::PermissionController::kNoPendingOperation;
+  NOTIMPLEMENTED_LOG_ONCE() << ": " << static_cast<int>(permission);
+  return SubscriptionId();
 }
 
 void WebEnginePermissionDelegate::UnsubscribePermissionStatusChange(
-    int subscription_id) {
+    SubscriptionId subscription_id) {
   // TODO(crbug.com/1063094): Implement permission status subscription. It's
   // used in blink to emit PermissionStatus.onchange notifications.
-  NOTREACHED();
+  NOTIMPLEMENTED_LOG_ONCE();
 }

fuchsia/engine/browser/web_engine_permission_delegate.h

@@ -45,13 +45,14 @@ class WebEnginePermissionDelegate
       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;
 };
 
 #endif  // FUCHSIA_ENGINE_BROWSER_WEB_ENGINE_PERMISSION_DELEGATE_H_

headless/lib/browser/headless_permission_manager.cc

@@ -71,15 +71,16 @@ HeadlessPermissionManager::GetPermissionStatusForFrame(
   return blink::mojom::PermissionStatus::ASK;
 }
 
-int HeadlessPermissionManager::SubscribePermissionStatusChange(
+HeadlessPermissionManager::SubscriptionId
+HeadlessPermissionManager::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 HeadlessPermissionManager::UnsubscribePermissionStatusChange(
-    int subscription_id) {}
+    SubscriptionId subscription_id) {}
 
 }  // namespace headless

headless/lib/browser/headless_permission_manager.h

@@ -46,13 +46,14 @@ class HeadlessPermissionManager : 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:
   content::BrowserContext* browser_context_;