Fixes Permission Bubbles never responding to duplicate requests

chrome/browser/ui/website_settings/permission_bubble_manager.cc

 // Copyright 2014 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/ui/website_settings/permission_bubble_manager.h"
 
 #include "base/command_line.h"
 #include "base/metrics/user_metrics_action.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/website_settings/permission_bubble_request.h"
@@ skipping 35 matching lines @@
   void RequestFinished() override { delete this; }
 
  private:
   int icon_;
   base::string16 message_text_;
   base::string16 message_fragment_;
   bool user_gesture_;
   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 -------------------------------------------
 
 PermissionBubbleManager::Observer::~Observer() {
 }
 
 void PermissionBubbleManager::Observer::OnBubbleAdded() {
 }
 
@@ skipping 10 matching lines @@
       view_(nullptr),
       main_frame_has_fully_loaded_(false),
       auto_response_for_test_(NONE),
       weak_factory_(this) {
 }
 
 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) {
   content::RecordAction(base::UserMetricsAction("PermissionBubbleRequest"));
   // TODO(gbillock): is there a race between an early request on a
   // newly-navigated page and the to-be-cleaned-up requests on the previous
   // page? We should maybe listen to DidStartNavigationToPendingEntry (and
   // any other renderer-side nav initiations?). Double-check this for
   // correct behavior on interstitials -- we probably want to basically queue
   // any request for which GetVisibleURL != GetLastCommittedURL.
   request_url_ = web_contents()->GetLastCommittedURL();
   bool is_main_frame = url::Origin(request_url_)
                            .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;
   }
 
   if (IsBubbleVisible()) {
     if (is_main_frame) {
       content::RecordAction(
           base::UserMetricsAction("PermissionBubbleRequestQueued"));
       queued_requests_.push_back(request);
     } else {
       content::RecordAction(
@@ skipping 10 matching lines @@
   } else {
     content::RecordAction(
         base::UserMetricsAction("PermissionBubbleIFrameRequestQueued"));
     queued_frame_requests_.push_back(request);
   }
 
   ScheduleShowBubble();
 }
 
 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();
        requests_iter != requests_.end();
        requests_iter++, accepts_iter++) {
     if (*requests_iter != request)
       continue;
 
     // We can simply erase the current entry in the request table if we aren't
     // 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);
 
       if (IsBubbleVisible()) {
         view_->Hide();
         // Will redraw the bubble if it is being shown.
         TriggerShowBubble();
       }
       return;
     }
 
     // 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.
 }
 
 void PermissionBubbleManager::HideBubble() {
   // Disengage from the existing view if there is one.
   if (!view_)
     return;
 
   view_->SetDelegate(nullptr);
   view_->Hide();
@@ skipping 73 matching lines @@
   DCHECK(request_index < static_cast<int>(accept_states_.size()));
   accept_states_[request_index] = new_value;
 }
 
 void PermissionBubbleManager::Accept() {
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   std::vector<bool>::iterator accepts_iter = accept_states_.begin();
   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();
 }
 
 void PermissionBubbleManager::Deny() {
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->PermissionDenied();
+    PermissionDeniedIncludingDuplicates(*requests_iter);
   }
   FinalizeBubble();
 }
 
 void PermissionBubbleManager::Closing() {
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->Cancelled();
+    CancelledIncludingDuplicates(*requests_iter);
   }
   FinalizeBubble();
 }
 
 void PermissionBubbleManager::ScheduleShowBubble() {
   // ::ScheduleShowBubble() will be called again when the main frame will be
   // loaded.
   if (!main_frame_has_fully_loaded_)
     return;
 
@@ skipping 43 matching lines @@
 }
 
 void PermissionBubbleManager::FinalizeBubble() {
   if (view_)
     view_->Hide();
 
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
-    (*requests_iter)->RequestFinished();
+    RequestFinishedIncludingDuplicates(*requests_iter);
   }
   requests_.clear();
   accept_states_.clear();
   if (queued_requests_.size() || queued_frame_requests_.size())
     TriggerShowBubble();
   else
     request_url_ = GURL();
 }
 
 void PermissionBubbleManager::CancelPendingQueues() {
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   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(
     const std::vector<PermissionBubbleRequest*>& queue) {
   std::vector<PermissionBubbleRequest*>::const_iterator iter;
   for (iter = queue.begin(); iter != queue.end(); iter++) {
     if ((*iter)->HasUserGesture())
       return true;
   }
   return false;
 }
 
+void PermissionBubbleManager::PermissionGrantedIncludingDuplicates(
+    PermissionBubbleRequest* request) {
+  DCHECK_EQ(request, GetExistingRequest(request))

[felt, 2016/01/28 06:18:14]

nit: wondering why this isn't DCHECKing that GetExistingRequest returns a
nullptr. i guess because it would be in requests_. anyways it is a slightly
counterintuitive DCHECK for me.

[johnme, 2016/01/28 15:40:11]

I've changed the DCHECK comment to "Only requests in [queued_[frame_]]requests_
can have duplicates" to make this clearer.

+      << "Request must not be a duplicate";
+  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))
+      << "Request must not be a duplicate";
+  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))
+      << "Request must not be a duplicate";
+  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) {
+  DCHECK_EQ(request, GetExistingRequest(request))
+      << "Request must not be a duplicate";
+  request->RequestFinished();
+  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);
 }
 
 void PermissionBubbleManager::RemoveObserver(Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }
 
 void PermissionBubbleManager::NotifyBubbleAdded() {
   FOR_EACH_OBSERVER(Observer, observer_list_, OnBubbleAdded());
 }
 
 void PermissionBubbleManager::DoAutoResponseForTesting() {
   switch (auto_response_for_test_) {
     case ACCEPT_ALL:
       Accept();
       break;
     case DENY_ALL:
       Deny();
       break;
     case DISMISS:
       Closing();
       break;
     case NONE:
       NOTREACHED();
   }
 }