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

chrome/browser/permissions/permission_manager.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/permissions/permission_manager.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/callback.h"
@@ skipping 178 matching lines @@
 
  private:
   int render_process_id_;
   int render_frame_id_;
   const base::Callback<void(const std::vector<ContentSetting>&)> callback_;
   std::vector<ContentSettingsType> permissions_;
   std::vector<ContentSetting> results_;
   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;
   GURL embedding_origin;
   base::Callback<void(ContentSetting)> callback;
   ContentSetting current_value;
 };
 
 // static
 PermissionManager* PermissionManager::Get(Profile* profile) {
@@ skipping 34 matching lines @@
           profile, CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
   permission_contexts_[CONTENT_SETTINGS_TYPE_BACKGROUND_SYNC] =
       base::MakeUnique<BackgroundSyncPermissionContext>(profile);
 #if BUILDFLAG(ENABLE_PLUGINS)
   permission_contexts_[CONTENT_SETTINGS_TYPE_PLUGINS] =
       base::MakeUnique<FlashPermissionContext>(profile);
 #endif
 }
 
 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());
 }
 
 void PermissionManager::Shutdown() {
   if (!subscriptions_.IsEmpty()) {
     HostContentSettingsMapFactory::GetForProfile(profile_)
         ->RemoveObserver(this);
     subscriptions_.Clear();
   }
 }
@@ skipping 38 matching lines @@
   int request_id = pending_requests_.Add(base::MakeUnique<PendingRequest>(
       render_frame_host, permissions, callback));
 
   const PermissionRequestID request(render_frame_host, request_id);
 
   for (size_t i = 0; i < permissions.size(); ++i) {
     const ContentSettingsType permission = permissions[i];
 
     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.
   if (!pending_requests_.Lookup(request_id))
     return kNoPendingOperation;
 
   return request_id;
 }
 
 PermissionResult PermissionManager::GetPermissionStatus(
@@ skipping 81 matching lines @@
 
   const PermissionRequestID request(pending_request->render_process_id(),
                                     pending_request->render_frame_id(),
                                     request_id);
   for (ContentSettingsType permission : pending_request->permissions()) {
     PermissionContextBase* context = GetPermissionContext(permission);
     if (!context)
       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,
                                         const GURL& requesting_origin,
                                         const GURL& embedding_origin) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   PermissionContextBase* context =
       GetPermissionContext(PermissionTypeToContentSetting(permission));
   if (!context)
     return;
@@ skipping 109 matching lines @@
     const GURL& embedding_origin) {
   PermissionContextBase* context = GetPermissionContext(permission);
   PermissionResult result = context->GetPermissionStatus(
       render_frame_host, requesting_origin.GetOrigin(),
       embedding_origin.GetOrigin());
   DCHECK(result.content_setting == CONTENT_SETTING_ALLOW ||
          result.content_setting == CONTENT_SETTING_ASK ||
          result.content_setting == CONTENT_SETTING_BLOCK);
   return result;
 }