Improve Mojo permission service and chrome::PermissionManager observing.

This is fixing two bugs:
- mojo callbacks were not always run, which is now required.
- the service was not always unsubscribing from the PermissionManager,
  keeping stalled subscriptions alive.

BUG=430238, 437770

Review URL: https://codereview.chromium.org/1054653002

Cr-Commit-Position: refs/heads/master@{#323368}

5 files changed, 102 insertions, 51 deletions

diff --git a/chrome/browser/permissions/permission_manager.cc b/chrome/browser/permissions/permission_manager.cc
index 44f281a..217ec7f 100644
--- a/chrome/browser/permissions/permission_manager.cc
+++ b/chrome/browser/permissions/permission_manager.cc
@@ -207,6 +207,8 @@ void PermissionManager::OnContentSettingChanged(
     const ContentSettingsPattern& secondary_pattern,
     ContentSettingsType content_type,
     std::string resource_identifier) {
+  std::list<base::Closure> callbacks;
+
   for (SubscriptionsMap::iterator iter(&subscriptions_);
        !iter.IsAtEnd(); iter.Advance()) {
     Subscription* subscription = iter.GetCurrentValue();
@@ -230,6 +232,14 @@ void PermissionManager::OnContentSettingChanged(
       continue;
 
     subscription->current_value = new_value;
-    subscription->callback.Run(ContentSettingToPermissionStatus(new_value));
+
+    // Add the callback to |callbacks| which will be run after the loop to
+    // prevent re-entrance issues.
+    callbacks.push_back(
+        base::Bind(subscription->callback,
+                   ContentSettingToPermissionStatus(new_value)));
   }
+
+  for (const auto& callback : callbacks)
+    callback.Run();
 }
diff --git a/content/browser/permissions/permission_service_context.cc b/content/browser/permissions/permission_service_context.cc
index 50c73a9..6922fb6 100644
--- a/content/browser/permissions/permission_service_context.cc
+++ b/content/browser/permissions/permission_service_context.cc
@@ -47,7 +47,7 @@ void PermissionServiceContext::ServiceHadConnectionError(
 
 void PermissionServiceContext::RenderFrameDeleted(
     RenderFrameHost* render_frame_host) {
-  CancelPendingRequests(render_frame_host);
+  CancelPendingOperations(render_frame_host);
 }
 
 void PermissionServiceContext::DidNavigateAnyFrame(
@@ -57,16 +57,16 @@ void PermissionServiceContext::DidNavigateAnyFrame(
   if (details.is_in_page)
     return;
 
-  CancelPendingRequests(render_frame_host);
+  CancelPendingOperations(render_frame_host);
 }
 
-void PermissionServiceContext::CancelPendingRequests(
+void PermissionServiceContext::CancelPendingOperations(
     RenderFrameHost* render_frame_host) const {
   if (render_frame_host != render_frame_host_)
     return;
 
   for (auto* service : services_)
-    service->CancelPendingRequests();
+    service->CancelPendingOperations();
 }
 
 BrowserContext* PermissionServiceContext::GetBrowserContext() const {
diff --git a/content/browser/permissions/permission_service_context.h b/content/browser/permissions/permission_service_context.h
index c14b40c..355a572 100644
--- a/content/browser/permissions/permission_service_context.h
+++ b/content/browser/permissions/permission_service_context.h
@@ -43,7 +43,7 @@ class PermissionServiceContext : public WebContentsObserver {
                            const LoadCommittedDetails& details,
                            const FrameNavigateParams& params) override;
 
-  void CancelPendingRequests(RenderFrameHost*) const;
+  void CancelPendingOperations(RenderFrameHost*) const;
 
   RenderFrameHost* render_frame_host_;
   RenderProcessHost* render_process_host_;
diff --git a/content/browser/permissions/permission_service_impl.cc b/content/browser/permissions/permission_service_impl.cc
index fa2e102..24c3a40 100644
--- a/content/browser/permissions/permission_service_impl.cc
+++ b/content/browser/permissions/permission_service_impl.cc
@@ -36,7 +36,7 @@ PermissionType PermissionNameToPermissionType(PermissionName name) {
 PermissionServiceImpl::PendingRequest::PendingRequest(
     PermissionType permission,
     const GURL& origin,
-    const mojo::Callback<void(PermissionStatus)>& callback)
+    const PermissionStatusCallback& callback)
     : permission(permission),
       origin(origin),
       callback(callback) {
@@ -48,12 +48,27 @@ PermissionServiceImpl::PendingRequest::~PendingRequest() {
   }
 }
 
+PermissionServiceImpl::PendingSubscription::PendingSubscription(
+    PermissionType permission,
+    const GURL& origin,
+    const PermissionStatusCallback& callback)
+    : permission(permission),
+      origin(origin),
+      callback(callback) {
+}
+
+PermissionServiceImpl::PendingSubscription::~PendingSubscription() {
+  DCHECK(callback.is_null());
+}
+
 PermissionServiceImpl::PermissionServiceImpl(PermissionServiceContext* context)
     : context_(context),
       weak_factory_(this) {
 }
 
 PermissionServiceImpl::~PermissionServiceImpl() {
+  DCHECK(pending_requests_.IsEmpty());
+  DCHECK(pending_subscriptions_.IsEmpty());
 }
 
 void PermissionServiceImpl::OnConnectionError() {
@@ -65,7 +80,7 @@ void PermissionServiceImpl::RequestPermission(
     PermissionName permission,
     const mojo::String& origin,
     bool user_gesture,
-    const mojo::Callback<void(PermissionStatus)>& callback) {
+    const PermissionStatusCallback& callback) {
   // This condition is valid if the call is coming from a ChildThread instead of
   // a RenderFrame. Some consumers of the service run in Workers and some in
   // Frames. In the context of a Worker, it is not possible to show a
@@ -106,13 +121,13 @@ void PermissionServiceImpl::OnRequestPermissionResponse(
     int request_id,
     PermissionStatus status) {
   PendingRequest* request = pending_requests_.Lookup(request_id);
-  mojo::Callback<void(PermissionStatus)> callback(request->callback);
+  PermissionStatusCallback callback(request->callback);
   request->callback.reset();
   pending_requests_.Remove(request_id);
   callback.Run(status);
 }
 
-void PermissionServiceImpl::CancelPendingRequests() {
+void PermissionServiceImpl::CancelPendingOperations() {
   DCHECK(context_->web_contents());
   DCHECK(context_->GetBrowserContext());
 
@@ -121,6 +136,7 @@ void PermissionServiceImpl::CancelPendingRequests() {
   if (!permission_manager)
     return;
 
+  // Cancel pending requests.
   for (RequestsMap::Iterator<PendingRequest> it(&pending_requests_);
        !it.IsAtEnd(); it.Advance()) {
     permission_manager->CancelPermissionRequest(
@@ -129,23 +145,30 @@ void PermissionServiceImpl::CancelPendingRequests() {
         it.GetCurrentKey(),
         it.GetCurrentValue()->origin);
   }
-
   pending_requests_.Clear();
+
+  // Cancel pending subscriptions.
+  for (SubscriptionsMap::Iterator<PendingSubscription>
+          it(&pending_subscriptions_); !it.IsAtEnd(); it.Advance()) {
+    it.GetCurrentValue()->callback.Run(GetPermissionStatusFromType(
+        it.GetCurrentValue()->permission, it.GetCurrentValue()->origin));
+    it.GetCurrentValue()->callback.reset();
+    permission_manager->UnsubscribePermissionStatusChange(it.GetCurrentKey());
+  }
+  pending_subscriptions_.Clear();
 }
 
 void PermissionServiceImpl::HasPermission(
     PermissionName permission,
     const mojo::String& origin,
-    const mojo::Callback<void(PermissionStatus)>& callback) {
-  DCHECK(context_->GetBrowserContext());
-
+    const PermissionStatusCallback& callback) {
   callback.Run(GetPermissionStatusFromName(permission, GURL(origin)));
 }
 
 void PermissionServiceImpl::RevokePermission(
     PermissionName permission,
     const mojo::String& origin,
-    const mojo::Callback<void(PermissionStatus)>& callback) {
+    const PermissionStatusCallback& callback) {
   GURL origin_url(origin);
   PermissionType permission_type = PermissionNameToPermissionType(permission);
   PermissionStatus status = GetPermissionStatusFromType(permission_type,
@@ -165,11 +188,12 @@ void PermissionServiceImpl::RevokePermission(
 
 void PermissionServiceImpl::GetNextPermissionChange(
     PermissionName permission,
-    const mojo::String& origin,
+    const mojo::String& mojo_origin,
     PermissionStatus last_known_status,
-    const mojo::Callback<void(PermissionStatus)>& callback) {
+    const PermissionStatusCallback& callback) {
+  GURL origin(mojo_origin);
   PermissionStatus current_status =
-      GetPermissionStatusFromName(permission, GURL(origin));
+      GetPermissionStatusFromName(permission, origin);
   if (current_status != last_known_status) {
     callback.Run(current_status);
     return;
@@ -183,18 +207,22 @@ void PermissionServiceImpl::GetNextPermissionChange(
   }
 
   int* subscription_id = new int();
+  PermissionType permission_type = PermissionNameToPermissionType(permission);
 
   GURL embedding_origin = context_->GetEmbeddingOrigin();
   *subscription_id =
       browser_context->GetPermissionManager()->SubscribePermissionStatusChange(
-          PermissionNameToPermissionType(permission),
-          GURL(origin),
+          permission_type,
+          origin,
           // If the embedding_origin is empty, we,ll use the |origin| instead.
-          embedding_origin.is_empty() ? GURL(origin) : embedding_origin,
+          embedding_origin.is_empty() ? origin : embedding_origin,
           base::Bind(&PermissionServiceImpl::OnPermissionStatusChanged,
                      weak_factory_.GetWeakPtr(),
-                     callback,
                      base::Owned(subscription_id)));
+
+  pending_subscriptions_.AddWithID(
+      new PendingSubscription(permission_type, origin, callback),
+                              *subscription_id);
 }
 
 PermissionStatus PermissionServiceImpl::GetPermissionStatusFromName(
@@ -230,10 +258,12 @@ void PermissionServiceImpl::ResetPermissionStatus(PermissionType type,
 }
 
 void PermissionServiceImpl::OnPermissionStatusChanged(
-    const mojo::Callback<void(PermissionStatus)>& callback,
     const int* subscription_id,
     PermissionStatus status) {
-  callback.Run(status);
+  PendingSubscription* subscription =
+      pending_subscriptions_.Lookup(*subscription_id);
+
+  subscription->callback.Run(status);
 
   BrowserContext* browser_context = context_->GetBrowserContext();
   DCHECK(browser_context);
@@ -241,6 +271,9 @@ void PermissionServiceImpl::OnPermissionStatusChanged(
     return;
   browser_context->GetPermissionManager()->UnsubscribePermissionStatusChange(
       *subscription_id);
+
+  subscription->callback.reset();
+  pending_subscriptions_.Remove(*subscription_id);
 }
 
 }  // namespace content
diff --git a/content/browser/permissions/permission_service_impl.h b/content/browser/permissions/permission_service_impl.h
index e3c3fb4..0d81976 100644
--- a/content/browser/permissions/permission_service_impl.h
+++ b/content/browser/permissions/permission_service_impl.h
@@ -26,9 +26,10 @@ class PermissionServiceImpl : public mojo::InterfaceImpl<PermissionService> {
  public:
   ~PermissionServiceImpl() override;
 
-  // Clear pending permissions associated with a given frame and request the
-  // browser to cancel the permission requests.
-  void CancelPendingRequests();
+  // Clear pending operations currently run by the service. This will be called
+  // by PermissionServiceContext when it will need the service to clear its
+  // state for example, if the frame changes.
+  void CancelPendingOperations();
 
  protected:
   friend PermissionServiceContext;
@@ -36,43 +37,51 @@ class PermissionServiceImpl : public mojo::InterfaceImpl<PermissionService> {
   PermissionServiceImpl(PermissionServiceContext* context);
 
  private:
+  using PermissionStatusCallback = mojo::Callback<void(PermissionStatus)>;
+
   struct PendingRequest {
     PendingRequest(PermissionType permission, const GURL& origin,
-                   const mojo::Callback<void(PermissionStatus)>& callback);
+                   const PermissionStatusCallback& callback);
     ~PendingRequest();
 
     PermissionType permission;
     GURL origin;
-    mojo::Callback<void(PermissionStatus)> callback;
+    PermissionStatusCallback callback;
+  };
+  using RequestsMap = IDMap<PendingRequest, IDMapOwnPointer>;
+
+  struct PendingSubscription {
+    PendingSubscription(PermissionType permission, const GURL& origin,
+                        const PermissionStatusCallback& callback);
+    ~PendingSubscription();
+
+    PermissionType permission;
+    GURL origin;
+    PermissionStatusCallback callback;
   };
-  typedef IDMap<PendingRequest, IDMapOwnPointer> RequestsMap;
+  using SubscriptionsMap = IDMap<PendingSubscription, IDMapOwnPointer>;
 
   // PermissionService.
-  void HasPermission(
-      PermissionName permission,
-      const mojo::String& origin,
-      const mojo::Callback<void(PermissionStatus)>& callback) override;
-  void RequestPermission(
-      PermissionName permission,
-      const mojo::String& origin,
-      bool user_gesture,
-      const mojo::Callback<void(PermissionStatus)>& callback) override;
-  void RevokePermission(
-      PermissionName permission,
-      const mojo::String& origin,
-      const mojo::Callback<void(PermissionStatus)>& callback) override;
+  void HasPermission(PermissionName permission,
+                     const mojo::String& origin,
+                     const PermissionStatusCallback& callback) override;
+  void RequestPermission(PermissionName permission,
+                         const mojo::String& origin,
+                         bool user_gesture,
+                         const PermissionStatusCallback& callback) override;
+  void RevokePermission(PermissionName permission,
+                        const mojo::String& origin,
+                        const PermissionStatusCallback& callback) override;
   void GetNextPermissionChange(
       PermissionName permission,
       const mojo::String& origin,
       PermissionStatus last_known_status,
-      const mojo::Callback<void(PermissionStatus)>& callback) override;
+      const PermissionStatusCallback& callback) override;
 
   // mojo::InterfaceImpl.
   void OnConnectionError() override;
 
-  void OnRequestPermissionResponse(
-      int request_id,
-      PermissionStatus status);
+  void OnRequestPermissionResponse(int request_id, PermissionStatus status);
 
   PermissionStatus GetPermissionStatusFromName(PermissionName permission,
                                                const GURL& origin);
@@ -80,12 +89,11 @@ class PermissionServiceImpl : public mojo::InterfaceImpl<PermissionService> {
                                                const GURL& origin);
   void ResetPermissionStatus(PermissionType type, const GURL& origin);
 
-  void OnPermissionStatusChanged(
-      const mojo::Callback<void(PermissionStatus)>& callback,
-      const int* subscription_id,
-      PermissionStatus status);
+  void OnPermissionStatusChanged(const int* subscription_id,
+                                 PermissionStatus status);
 
   RequestsMap pending_requests_;
+  SubscriptionsMap pending_subscriptions_;
   // context_ owns |this|.
   PermissionServiceContext* context_;
   base::WeakPtrFactory<PermissionServiceImpl> weak_factory_;