Ensure media permission requests are correctly cancelled when permission prompts are ignored

chrome/browser/permissions/permission_manager.cc

Index: chrome/browser/permissions/permission_manager.cc
diff --git a/chrome/browser/permissions/permission_manager.cc b/chrome/browser/permissions/permission_manager.cc
index 5f205178591939e9455d83bf4ca505034c7557e6..f28500639bb6323abc9dcf1d8b973a8ec811887d 100644
--- a/chrome/browser/permissions/permission_manager.cc
+++ b/chrome/browser/permissions/permission_manager.cc
@@ -196,6 +196,46 @@ class PermissionManager::PendingRequest {
   size_t remaining_results_;
 };
 
+// Object to track the callback passed to
+// PermissionContextBase::RequestPermission. The callback passed in will never
+// be run when a permission prompt has been ignored, but it's important that we
+// know when a prompt is ignored to clean up |pending_requests_| correctly.
+// If the callback is destroyed without being run, the destructor here will
+// cancel the request to clean up.
+class PermissionManager::PermissionResponseCallback {
+ public:
+  PermissionResponseCallback(
+      const base::WeakPtr<PermissionManager>& permission_manager,
+      int request_id,
+      int permission_id)
+      : permission_manager_(permission_manager),
+        request_id_(request_id),
+        permission_id_(permission_id),
+        request_answered_(false) {}
+
+  ~PermissionResponseCallback() {
+    if (!request_answered_ &&
+        permission_manager_->pending_requests_.Lookup(request_id_)) {

[Timothy Loh, 2017/07/05 07:32:49]

Is this check needed?

[raymes, 2017/07/06 00:05:43]

The request may contain multiple permissions and currently if any one them is
ignored, we remove the entire request, so when we get here, the request may have
been removed already. It didn't seem worth tracking ignores because ignore
implies that any future prompts from the same request would also be ignored.

+      permission_manager_->pending_requests_.Remove(request_id_);
+    }
+  }
+
+  void OnPermissionsRequestResponseStatus(ContentSetting content_setting) {
+    request_answered_ = true;
+    permission_manager_->OnPermissionsRequestResponseStatus(
+        request_id_, permission_id_, content_setting);
+  }
+
+ private:
+  base::WeakPtr<PermissionManager> permission_manager_;
+  int request_id_;
+  int permission_id_;
+
+  bool request_answered_;
+
+  DISALLOW_COPY_AND_ASSIGN(PermissionResponseCallback);
+};
+
 struct PermissionManager::Subscription {
   ContentSettingsType permission;
   GURL requesting_origin;
@@ -250,6 +290,7 @@ PermissionManager::PermissionManager(Profile* profile)
 }
 
 PermissionManager::~PermissionManager() {
+  DCHECK(pending_requests_.IsEmpty());

[Timothy Loh, 2017/07/05 07:32:49]

I'm not sure this works, do we need to move the logic in
~PermissionRequestManager() to PermissionRequestManager::ShutDown().

[raymes, 2017/07/06 00:05:43]

The PermissionManager is a profile-keyed service which should outlast
PermissionRequestManager which has the same lifetime as a WebContents. So I
think this should be fine.

   DCHECK(subscriptions_.IsEmpty());
 }
 
@@ -308,10 +349,13 @@ int PermissionManager::RequestPermissions(
 
     PermissionContextBase* context = GetPermissionContext(permission);
     DCHECK(context);
+    auto callback = base::MakeUnique<PermissionResponseCallback>(
+        weak_ptr_factory_.GetWeakPtr(), request_id, i);
     context->RequestPermission(
         web_contents, request, requesting_origin, user_gesture,
-        base::Bind(&PermissionManager::OnPermissionsRequestResponseStatus,
-                   weak_ptr_factory_.GetWeakPtr(), request_id, i));
+        base::Bind(
+            &PermissionResponseCallback::OnPermissionsRequestResponseStatus,
+            base::Passed(&callback)));
   }
 
   // The request might have been resolved already.
@@ -413,7 +457,8 @@ void PermissionManager::CancelPermissionRequest(int request_id) {
       continue;
     context->CancelPermissionRequest(web_contents, request);
   }
-  pending_requests_.Remove(request_id);
+  // The request should be automatically removed from |pending_requests_| as a
+  // result of it being cancelled but not necessarily immediately.
 }
 
 void PermissionManager::ResetPermission(PermissionType permission,