permissions: handle request ids for permissions in permission manager

android_webview/browser/aw_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 "android_webview/browser/aw_permission_manager.h"
 
 #include <string>
 
 #include "android_webview/browser/aw_browser_permission_request_delegate.h"
 #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"
 #include "content/public/browser/web_contents.h"
 
 using content::PermissionStatus;
 using content::PermissionType;
 
 namespace android_webview {
 
 class LastRequestResultCache {
  public:
-  LastRequestResultCache() : weak_factory_(this) {}
+  LastRequestResultCache() = default;
 
   void SetResult(PermissionType permission,
                  const GURL& requesting_origin,
                  const GURL& embedding_origin,
                  PermissionStatus status) {
     DCHECK(status == content::PERMISSION_STATUS_GRANTED ||
            status == content::PERMISSION_STATUS_DENIED);
 
     // TODO(ddorwin): We should be denying empty origins at a higher level.
     if (requesting_origin.is_empty() || embedding_origin.is_empty()) {
@@ skipping 70 matching lines @@
 
     if (permission != PermissionType::PROTECTED_MEDIA_IDENTIFIER) {
       // Other permissions are not cached, so nothing to clear.
       return;
     }
 
     std::string key = GetCacheKey(requesting_origin, embedding_origin);
     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.
   static std::string GetCacheKey(const GURL& requesting_origin,
                                  const GURL& embedding_origin) {
     const std::string& requesting = requesting_origin.spec();
     const std::string& embedding = embedding_origin.spec();
     if (requesting.empty() || embedding.empty())
       return std::string();
     return requesting + "," + embedding;
   }
 
   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();
   int render_frame_id = render_frame_host->GetRoutingID();
   AwBrowserPermissionRequestDelegate* delegate =
       AwBrowserPermissionRequestDelegate::FromID(render_process_id,
                                                  render_frame_id);
   if (!delegate) {
     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:
     case PermissionType::NOTIFICATIONS:
     case PermissionType::PUSH_MESSAGING:
     case PermissionType::DURABLE_STORAGE:
       NOTIMPLEMENTED() << "RequestPermission is not implemented for "
                        << static_cast<int>(permission);
       callback.Run(content::PERMISSION_STATUS_DENIED);
       break;
     case PermissionType::MIDI:
       callback.Run(content::PERMISSION_STATUS_GRANTED);
       break;
     case PermissionType::NUM:
       NOTREACHED() << "PermissionType::NUM was not expected here.";
       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:
     case PermissionType::DURABLE_STORAGE:
     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.
       break;
     case PermissionType::NUM:
       NOTREACHED() << "PermissionType::NUM was not expected here.";
       break;
   }
+
+  pending_requests_.Remove(request_id);
 }
 
 void AwPermissionManager::ResetPermission(PermissionType permission,
                                           const GURL& requesting_origin,
                                           const GURL& embedding_origin) {
   result_cache_->ClearResult(permission, requesting_origin, embedding_origin);
 }
 
 PermissionStatus AwPermissionManager::GetPermissionStatus(
     PermissionType permission,
@@ skipping 14 matching lines @@
     PermissionType permission,
     const GURL& requesting_origin,
     const GURL& embedding_origin) {
 }
 
 int AwPermissionManager::SubscribePermissionStatusChange(
     PermissionType permission,
     const GURL& requesting_origin,
     const GURL& embedding_origin,
     const base::Callback<void(PermissionStatus)>& callback) {
-  return -1;
+  return kNoPendingOperation;
 }
 
 void AwPermissionManager::UnsubscribePermissionStatusChange(
     int subscription_id) {
 }
 
 }  // namespace android_webview