Fixes Permission Bubbles never responding to duplicate requests

chrome/browser/ui/website_settings/permission_bubble_manager.cc

Index: chrome/browser/ui/website_settings/permission_bubble_manager.cc
diff --git a/chrome/browser/ui/website_settings/permission_bubble_manager.cc b/chrome/browser/ui/website_settings/permission_bubble_manager.cc
index 9d149df30795ba55d90688c5209ee04acaf9e8b9..d08238301fddd1c018e006c8809208e00c6a04db 100644
--- a/chrome/browser/ui/website_settings/permission_bubble_manager.cc
+++ b/chrome/browser/ui/website_settings/permission_bubble_manager.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/ui/website_settings/permission_bubble_manager.h"
 
+#include <algorithm>
+
 #include "base/command_line.h"
 #include "base/metrics/user_metrics_action.h"
 #include "build/build_config.h"
@@ -53,6 +55,17 @@ class CancelledRequest : public PermissionBubbleRequest {
   GURL origin_;
 };
 
+bool IsMessageTextEqual(PermissionBubbleRequest* a,
+                        PermissionBubbleRequest* b) {
+  if (a == b)
+    return true;
+  if (a->GetMessageTextFragment() == b->GetMessageTextFragment() &&
+      a->GetOrigin() == b->GetOrigin()) {
+    return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 // PermissionBubbleManager::Observer -------------------------------------------
@@ -83,17 +96,14 @@ PermissionBubbleManager::~PermissionBubbleManager() {
   if (view_ != NULL)
     view_->SetDelegate(NULL);
 
-  std::vector<PermissionBubbleRequest*>::iterator requests_iter;
-  for (requests_iter = requests_.begin();
-       requests_iter != requests_.end();
-       requests_iter++) {
-    (*requests_iter)->RequestFinished();
-  }
-  for (requests_iter = queued_requests_.begin();
-       requests_iter != queued_requests_.end();
-       requests_iter++) {
-    (*requests_iter)->RequestFinished();
-  }
+  for (PermissionBubbleRequest* request : requests_)
+    request->RequestFinished();
+  for (PermissionBubbleRequest* request : queued_requests_)
+    request->RequestFinished();
+  for (PermissionBubbleRequest* request : queued_frame_requests_)
+    request->RequestFinished();
+  for (const auto& entry : duplicate_requests_)
+    entry.second->RequestFinished();
 }
 
 void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
@@ -109,15 +119,18 @@ void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
                            .IsSameOriginWith(url::Origin(request->GetOrigin()));
 
   // Don't re-add an existing request or one with a duplicate text request.
-  // TODO(johnme): Instead of dropping duplicate requests, we should queue them
-  // and eventually run their PermissionGranted/PermissionDenied/Cancelled
-  // callback (crbug.com/577313).
-  bool same_object = false;
-  if (ExistingRequest(request, requests_, &same_object) ||
-      ExistingRequest(request, queued_requests_, &same_object) ||
-      ExistingRequest(request, queued_frame_requests_, &same_object)) {
-    if (!same_object)
-      request->RequestFinished();
+  PermissionBubbleRequest* existing_request = GetExistingRequest(request);
+  if (existing_request) {
+    // |request| is a duplicate. Add it to |duplicate_requests_| unless it's the
+    // same object as |existing_request| or an existing duplicate.
+    if (request == existing_request)
+      return;
+    auto range = duplicate_requests_.equal_range(existing_request);
+    for (auto it = range.first; it != range.second; ++it) {
+      if (request == it->second)
+        return;
+    }
+    duplicate_requests_.insert(std::make_pair(existing_request, request));
     return;
   }
 
@@ -148,18 +161,26 @@ void PermissionBubbleManager::AddRequest(PermissionBubbleRequest* request) {
 }
 
 void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
-  // First look in the queued requests, where we can simply delete the request
+  // First look in the queued requests, where we can simply finish the request
   // and go on.
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = queued_requests_.begin();
        requests_iter != queued_requests_.end();
        requests_iter++) {
     if (*requests_iter == request) {
-      (*requests_iter)->RequestFinished();
+      RequestFinishedIncludingDuplicates(*requests_iter);
       queued_requests_.erase(requests_iter);
       return;
     }
   }
+  for (requests_iter = queued_frame_requests_.begin();
+       requests_iter != queued_frame_requests_.end(); requests_iter++) {
+    if (*requests_iter == request) {
+      RequestFinishedIncludingDuplicates(*requests_iter);
+      queued_frame_requests_.erase(requests_iter);
+      return;
+    }
+  }
 
   std::vector<bool>::iterator accepts_iter = accept_states_.begin();
   for (requests_iter = requests_.begin(), accepts_iter = accept_states_.begin();
@@ -172,7 +193,7 @@ void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
     // showing the dialog, or if we are showing it and it can accept the update.
     bool can_erase = !IsBubbleVisible() || view_->CanAcceptRequestUpdate();
     if (can_erase) {
-      (*requests_iter)->RequestFinished();
+      RequestFinishedIncludingDuplicates(*requests_iter);
       requests_.erase(requests_iter);
       accept_states_.erase(accepts_iter);
 
@@ -187,11 +208,25 @@ void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
     // Cancel the existing request and replace it with a dummy.
     PermissionBubbleRequest* cancelled_request =
         new CancelledRequest(*requests_iter);
-    (*requests_iter)->RequestFinished();
+    RequestFinishedIncludingDuplicates(*requests_iter);
     *requests_iter = cancelled_request;
     return;
   }
 
+  // Since |request| wasn't found in queued_requests_, queued_frame_requests_ or
+  // requests_ it must have been marked as a duplicate. We can't search
+  // duplicate_requests_ by value, so instead use GetExistingRequest to find the
+  // key (request it was duped against), and iterate through duplicates of that.
+  PermissionBubbleRequest* existing_request = GetExistingRequest(request);
+  auto range = duplicate_requests_.equal_range(existing_request);
+  for (auto it = range.first; it != range.second; ++it) {
+    if (request == it->second) {
+      it->second->RequestFinished();
+      duplicate_requests_.erase(it);
+      return;
+    }
+  }
+
   NOTREACHED();  // Callers should not cancel requests that are not pending.
 }
 
@@ -285,10 +320,11 @@ void PermissionBubbleManager::Accept() {
   for (requests_iter = requests_.begin(), accepts_iter = accept_states_.begin();
        requests_iter != requests_.end();
        requests_iter++, accepts_iter++) {
-    if (*accepts_iter)
-      (*requests_iter)->PermissionGranted();
-    else
-      (*requests_iter)->PermissionDenied();
+    if (*accepts_iter) {
+      PermissionGrantedIncludingDuplicates(*requests_iter);
+    } else {
+      PermissionDeniedIncludingDuplicates(*requests_iter);
+    }
   }
   FinalizeBubble();
 }
@@ -298,7 +334,7 @@ void PermissionBubbleManager::Deny() {
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->PermissionDenied();
+    PermissionDeniedIncludingDuplicates(*requests_iter);
   }
   FinalizeBubble();
 }
@@ -308,7 +344,7 @@ void PermissionBubbleManager::Closing() {
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->Cancelled();
+    CancelledIncludingDuplicates(*requests_iter);
   }
   FinalizeBubble();
 }
@@ -372,7 +408,7 @@ void PermissionBubbleManager::FinalizeBubble() {
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->RequestFinished();
+    RequestFinishedIncludingDuplicates(*requests_iter);
   }
   requests_.clear();
   accept_states_.clear();
@@ -387,36 +423,29 @@ void PermissionBubbleManager::CancelPendingQueues() {
   for (requests_iter = queued_requests_.begin();
        requests_iter != queued_requests_.end();
        requests_iter++) {
-    (*requests_iter)->RequestFinished();
+    RequestFinishedIncludingDuplicates(*requests_iter);
   }
   for (requests_iter = queued_frame_requests_.begin();
        requests_iter != queued_frame_requests_.end();
        requests_iter++) {
-    (*requests_iter)->RequestFinished();
+    RequestFinishedIncludingDuplicates(*requests_iter);
   }
   queued_requests_.clear();
   queued_frame_requests_.clear();
 }
 
-bool PermissionBubbleManager::ExistingRequest(
-    PermissionBubbleRequest* request,
-    const std::vector<PermissionBubbleRequest*>& queue,
-    bool* same_object) {
-  CHECK(same_object);
-  *same_object = false;
-  std::vector<PermissionBubbleRequest*>::const_iterator iter;
-  for (iter = queue.begin(); iter != queue.end(); iter++) {
-    if (*iter == request) {
-      *same_object = true;
-      return true;
-    }
-    if ((*iter)->GetMessageTextFragment() ==
-            request->GetMessageTextFragment() &&
-        (*iter)->GetOrigin() == request->GetOrigin()) {
-      return true;
-    }
-  }
-  return false;
+PermissionBubbleRequest* PermissionBubbleManager::GetExistingRequest(
+    PermissionBubbleRequest* request) {
+  for (PermissionBubbleRequest* existing_request : requests_)
+    if (IsMessageTextEqual(existing_request, request))
+      return existing_request;
+  for (PermissionBubbleRequest* existing_request : queued_requests_)
+    if (IsMessageTextEqual(existing_request, request))
+      return existing_request;
+  for (PermissionBubbleRequest* existing_request : queued_frame_requests_)
+    if (IsMessageTextEqual(existing_request, request))
+      return existing_request;
+  return nullptr;
 }
 
 bool PermissionBubbleManager::HasUserGestureRequest(
@@ -429,6 +458,52 @@ bool PermissionBubbleManager::HasUserGestureRequest(
   return false;
 }
 
+void PermissionBubbleManager::PermissionGrantedIncludingDuplicates(
+    PermissionBubbleRequest* request) {
+  DCHECK_EQ(request, GetExistingRequest(request))
+      << "Only requests in [queued_[frame_]]requests_ can have duplicates";
+  request->PermissionGranted();
+  auto range = duplicate_requests_.equal_range(request);
+  for (auto it = range.first; it != range.second; ++it)
+    it->second->PermissionGranted();
+}
+void PermissionBubbleManager::PermissionDeniedIncludingDuplicates(
+    PermissionBubbleRequest* request) {
+  DCHECK_EQ(request, GetExistingRequest(request))
+      << "Only requests in [queued_[frame_]]requests_ can have duplicates";
+  request->PermissionDenied();
+  auto range = duplicate_requests_.equal_range(request);
+  for (auto it = range.first; it != range.second; ++it)
+    it->second->PermissionDenied();
+}
+void PermissionBubbleManager::CancelledIncludingDuplicates(
+    PermissionBubbleRequest* request) {
+  DCHECK_EQ(request, GetExistingRequest(request))
+      << "Only requests in [queued_[frame_]]requests_ can have duplicates";
+  request->Cancelled();
+  auto range = duplicate_requests_.equal_range(request);
+  for (auto it = range.first; it != range.second; ++it)
+    it->second->Cancelled();
+}
+void PermissionBubbleManager::RequestFinishedIncludingDuplicates(
+    PermissionBubbleRequest* request) {
+  // We can't call GetExistingRequest here, because other entries in requests_,
+  // queued_requests_ or queued_frame_requests_ might already have been deleted.
+  DCHECK_EQ(1, std::count(requests_.begin(), requests_.end(), request) +
+               std::count(queued_requests_.begin(), queued_requests_.end(),
+                          request) +
+               std::count(queued_frame_requests_.begin(),
+                          queued_frame_requests_.end(), request))
+      << "Only requests in [queued_[frame_]]requests_ can have duplicates";
+  request->RequestFinished();
+  // Beyond this point, |request| has probably been deleted.
+  auto range = duplicate_requests_.equal_range(request);
+  for (auto it = range.first; it != range.second; ++it)
+    it->second->RequestFinished();
+  // Additionally, we can now remove the duplicates.
+  duplicate_requests_.erase(request);
+}
+
 void PermissionBubbleManager::AddObserver(Observer* observer) {
   observer_list_.AddObserver(observer);
 }