permissions: handle request ids for permissions in permission manager

android_webview/browser/aw_permission_manager.cc

Index: android_webview/browser/aw_permission_manager.cc
diff --git a/android_webview/browser/aw_permission_manager.cc b/android_webview/browser/aw_permission_manager.cc
index 6fe1e1e400a906ab5816b4c019c3057bae5bda3f..4e7b7192abab5fd525baad3484b8270a9aa015b1 100644
--- a/android_webview/browser/aw_permission_manager.cc
+++ b/android_webview/browser/aw_permission_manager.cc
@@ -10,7 +10,6 @@
 #include "base/callback.h"
 #include "base/containers/hash_tables.h"
 #include "base/logging.h"
-#include "base/memory/weak_ptr.h"
 #include "content/public/browser/permission_type.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -23,7 +22,7 @@ namespace android_webview {
 
 class LastRequestResultCache {
  public:
-  LastRequestResultCache() : weak_factory_(this) {}
+  LastRequestResultCache() = default;
 
   void SetResult(PermissionType permission,
                  const GURL& requesting_origin,
@@ -114,10 +113,6 @@ class LastRequestResultCache {
     pmi_result_cache_.erase(key);
   }
 
-  base::WeakPtr<LastRequestResultCache> GetWeakPtr() {
-    return weak_factory_.GetWeakPtr();
-  }
-
  private:
   // Returns a concatenation of the origins to be used as the index.
   // Returns the empty string if either origin is invalid or empty.
@@ -133,44 +128,42 @@ class LastRequestResultCache {
   using StatusMap = base::hash_map<std::string, PermissionStatus>;
   StatusMap pmi_result_cache_;
 
-  base::WeakPtrFactory<LastRequestResultCache> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(LastRequestResultCache);
 };
 
-namespace {
-
-void CallbackPermisisonStatusWrapper(
-    const base::WeakPtr<LastRequestResultCache>& result_cache,
-    const base::Callback<void(PermissionStatus)>& callback,
-    PermissionType permission,
-    const GURL& requesting_origin,
-    const GURL& embedding_origin,
-    bool allowed) {
-  PermissionStatus status = allowed ? content::PERMISSION_STATUS_GRANTED
-                                    : content::PERMISSION_STATUS_DENIED;
-  if (result_cache.get()) {
-    result_cache->SetResult(permission, requesting_origin, embedding_origin,
-                            status);
+class AwPermissionManager::PendingRequest {
+ public:
+  PendingRequest(PermissionType permission,
+                 GURL requesting_origin,
+                 GURL embedding_origin,
+                 content::RenderFrameHost* render_frame_host)
+    : permission(permission),
+      requesting_origin(requesting_origin),
+      embedding_origin(embedding_origin),
+      render_process_id(render_frame_host->GetProcess()->GetID()),
+      render_frame_id(render_frame_host->GetRoutingID()) {
   }
 
-  callback.Run(status);
-}
-
-}  // anonymous namespace
+  PermissionType permission;
+  GURL requesting_origin;
+  GURL embedding_origin;
+  int render_process_id;
+  int render_frame_id;

[michaelbai, 2015/09/23 16:57:26]

It seemed that all those data members can not be used to identify the permission
request, i.e. different request id could map to same pending request (have same
data members)

If so, using PendingRequest in CancelPermissionRequest() seems not right.

Did I miss something?

[Lalit Maganti, 2015/09/23 17:02:07]

This has now changed. Since PermissionManager implentations now return an int,
the request_id is unique with respect to the PermissionManager and
PermissionService will use the request_id returned when cancelling.

For context, most of this work is being done so that multiple permissions can be
requested at the same time. See https://codereview.chromium.org/1316863010/ for
more info on that.

[michaelbai, 2015/09/23 17:56:26]

My question is
Can the data members of PendingRequest be used to identify a permission request?
(in multiple permission case, using vector<PermissionType>)

If so, why request id needed?

If not, the implementation of AwPermissionManager::CancelPermissionRequest(int
request_id) is wrong.

[Lalit Maganti, 2015/09/23 18:02:37]

No these members alone cannot identify a request in the multiple request case.
Theoretically they cannot in the single request case either because there could
be subsequent requests which request the same permission from the same frame.
request_id alone uniquely identifies a request.

Would appreciate advice on how to go about fixing CancelPermissionRequest. I
have not looked into how details of the WebView permission system works so I do
not understand the role of AwBrowserPermissionRequestDelegate etc but pointers
on why it is broken and how I could investigate the fix are much appreciated :)

[michaelbai, 2015/09/23 18:08:05]

In the cases, WebView ignore the second one, in which use case, this could
happen?

[Lalit Maganti, 2015/09/23 18:20:05]

With the permissions api you could have a request for a permission and then
immediately after have another request for the same permission in JavaScript.

[michaelbai, 2015/09/23 18:29:53]

It seemed that the requests are actually duplicated, it make no sense to prompt
duplicate request to user in both browser and WebView, I suggest to merge the
duplicate requests in content layer, then, PermissionManager
implementations(browser and webview) don't need to handle the duplicate requests
at all?

+};
 
 AwPermissionManager::AwPermissionManager()
-    : content::PermissionManager(), result_cache_(new LastRequestResultCache) {
+  : content::PermissionManager(),
+    result_cache_(new LastRequestResultCache),
+    weak_ptr_factory_(this) {
 }
 
 AwPermissionManager::~AwPermissionManager() {
 }
 
-void AwPermissionManager::RequestPermission(
+int AwPermissionManager::RequestPermission(
     PermissionType permission,
     content::RenderFrameHost* render_frame_host,
-    int request_id,
-    const GURL& origin,
+    const GURL& requesting_origin,
     bool user_gesture,
     const base::Callback<void(PermissionStatus)>& callback) {
   int render_process_id = render_frame_host->GetProcess()->GetID();
@@ -182,31 +175,44 @@ void AwPermissionManager::RequestPermission(
     DVLOG(0) << "Dropping permission request for "
              << static_cast<int>(permission);
     callback.Run(content::PERMISSION_STATUS_DENIED);
-    return;
+    return kNoPendingOperation;
   }
 
   const GURL& embedding_origin =
       content::WebContents::FromRenderFrameHost(render_frame_host)
           ->GetLastCommittedURL().GetOrigin();
 
+  int request_id = kNoPendingOperation;
   switch (permission) {
     case PermissionType::GEOLOCATION:
+      request_id = pending_requests_.Add(new PendingRequest(
+          permission, requesting_origin,
+          embedding_origin, render_frame_host));
       delegate->RequestGeolocationPermission(
-          origin, base::Bind(&CallbackPermisisonStatusWrapper,
-                             result_cache_->GetWeakPtr(), callback, permission,
-                             origin, embedding_origin));
+          requesting_origin,
+          base::Bind(&AwPermissionManager::OnRequestResponse,
+                     weak_ptr_factory_.GetWeakPtr(), request_id,
+                     callback));
       break;
     case PermissionType::PROTECTED_MEDIA_IDENTIFIER:
+      request_id = pending_requests_.Add(new PendingRequest(
+          permission, requesting_origin,
+          embedding_origin, render_frame_host));
       delegate->RequestProtectedMediaIdentifierPermission(
-          origin, base::Bind(&CallbackPermisisonStatusWrapper,
-                             result_cache_->GetWeakPtr(), callback, permission,
-                             origin, embedding_origin));
+          requesting_origin,
+          base::Bind(&AwPermissionManager::OnRequestResponse,
+                     weak_ptr_factory_.GetWeakPtr(), request_id,
+                     callback));
       break;
     case PermissionType::MIDI_SYSEX:
+      request_id = pending_requests_.Add(new PendingRequest(
+          permission, requesting_origin,
+          embedding_origin, render_frame_host));
       delegate->RequestMIDISysexPermission(
-          origin, base::Bind(&CallbackPermisisonStatusWrapper,
-                             result_cache_->GetWeakPtr(), callback, permission,
-                             origin, embedding_origin));
+          requesting_origin,
+          base::Bind(&AwPermissionManager::OnRequestResponse,
+                     weak_ptr_factory_.GetWeakPtr(), request_id,
+                     callback));
       break;
     case PermissionType::AUDIO_CAPTURE:
     case PermissionType::VIDEO_CAPTURE:
@@ -225,38 +231,75 @@ void AwPermissionManager::RequestPermission(
       callback.Run(content::PERMISSION_STATUS_DENIED);
       break;
   }
+  return request_id;
 }
 
-void AwPermissionManager::CancelPermissionRequest(
-    PermissionType permission,
-    content::RenderFrameHost* render_frame_host,
+// static
+void AwPermissionManager::OnRequestResponse(
+    const base::WeakPtr<AwPermissionManager>& manager,
     int request_id,
-    const GURL& origin) {
+    const base::Callback<void(PermissionStatus)>& callback,
+    bool allowed) {
+  PermissionStatus status = allowed ? content::PERMISSION_STATUS_GRANTED
+                                    : content::PERMISSION_STATUS_DENIED;
+  if (manager.get()) {
+    PendingRequest* pending_request =
+        manager->pending_requests_.Lookup(request_id);
+    manager->result_cache_->SetResult(
+        pending_request->permission,
+        pending_request->requesting_origin,
+        pending_request->embedding_origin,
+        status);
+    manager->pending_requests_.Remove(request_id);
+  }
+  callback.Run(status);
+}
+
+void AwPermissionManager::CancelPermissionRequest(int request_id) {
+  PendingRequest* pending_request = pending_requests_.Lookup(request_id);
+  if (!pending_request)
+    return;
+
+  content::RenderFrameHost* render_frame_host =
+      content::RenderFrameHost::FromID(pending_request->render_process_id,
+          pending_request->render_frame_id);
+  DCHECK(render_frame_host);
+
+  content::WebContents* web_contents =
+      content::WebContents::FromRenderFrameHost(render_frame_host);
+  DCHECK(web_contents);
+
   // The caller is canceling (presumably) the most recent request. Assuming the
   // request did not complete, the user did not respond to the requset.
   // Thus, assume we do not know the result.
-  const GURL& embedding_origin =
-      content::WebContents::FromRenderFrameHost(render_frame_host)
+  const GURL& embedding_origin = web_contents
           ->GetLastCommittedURL().GetOrigin();
-  result_cache_->ClearResult(permission, origin, embedding_origin);
+  result_cache_->ClearResult(
+      pending_request->permission,
+      pending_request->requesting_origin,
+      embedding_origin);
 
-  int render_process_id = render_frame_host->GetProcess()->GetID();
-  int render_frame_id = render_frame_host->GetRoutingID();
   AwBrowserPermissionRequestDelegate* delegate =
-      AwBrowserPermissionRequestDelegate::FromID(render_process_id,
-                                                 render_frame_id);
-  if (!delegate)
+      AwBrowserPermissionRequestDelegate::FromID(
+          pending_request->render_process_id,
+          pending_request->render_frame_id);
+  if (!delegate) {
+    pending_requests_.Remove(request_id);
     return;
+  }
 
-  switch (permission) {
+  switch (pending_request->permission) {
     case PermissionType::GEOLOCATION:
-      delegate->CancelGeolocationPermissionRequests(origin);
+      delegate->CancelGeolocationPermissionRequests(
+          pending_request->requesting_origin);
       break;
     case PermissionType::PROTECTED_MEDIA_IDENTIFIER:
-      delegate->CancelProtectedMediaIdentifierPermissionRequests(origin);
+      delegate->CancelProtectedMediaIdentifierPermissionRequests(
+          pending_request->requesting_origin);
       break;
     case PermissionType::MIDI_SYSEX:
-      delegate->CancelMIDISysexPermissionRequests(origin);
+      delegate->CancelMIDISysexPermissionRequests(
+          pending_request->requesting_origin);
       break;
     case PermissionType::NOTIFICATIONS:
     case PermissionType::PUSH_MESSAGING:
@@ -264,7 +307,7 @@ void AwPermissionManager::CancelPermissionRequest(
     case PermissionType::AUDIO_CAPTURE:
     case PermissionType::VIDEO_CAPTURE:
       NOTIMPLEMENTED() << "CancelPermission not implemented for "
-                       << static_cast<int>(permission);
+                       << static_cast<int>(pending_request->permission);
       break;
     case PermissionType::MIDI:
       // There is nothing to cancel so this is simply ignored.
@@ -273,6 +316,8 @@ void AwPermissionManager::CancelPermissionRequest(
       NOTREACHED() << "PermissionType::NUM was not expected here.";
       break;
   }
+
+  pending_requests_.Remove(request_id);
 }
 
 void AwPermissionManager::ResetPermission(PermissionType permission,
@@ -307,7 +352,7 @@ int AwPermissionManager::SubscribePermissionStatusChange(
     const GURL& requesting_origin,
     const GURL& embedding_origin,
     const base::Callback<void(PermissionStatus)>& callback) {
-  return -1;
+  return kNoPendingOperation;
 }
 
 void AwPermissionManager::UnsubscribePermissionStatusChange(