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 65 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->GetRequestingHostname()));
 
   // 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;
   }
 
+  // Finally look through the duplicate requests, which can simply be finished.
+  PermissionBubbleRequest* existing_request = GetExistingRequest(request);

[felt, 2016/01/23 16:51:43]

If RequestFinishedIncludingDuplicates removes the duplicates do you need to do
it here too?

[johnme, 2016/01/26 15:57:55]

Yes - normally duplicate requests only finish when the request they were duped
against finishes, but this is the special case where we are asked to cancel a
duplicate request before the request it was duped against finished.

I've improved the comment, since it's nonobvious why I had to use
GetExistingRequest here. It now says:

// 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.

+  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)->GetRequestingHostname() == request->GetRequestingHostname()) {
-      return true;
-    }
+bool MessageTextIsEqual(PermissionBubbleRequest* a,

[felt, 2016/01/23 16:51:43]

nit: IsMessageTextEqual

[johnme, 2016/01/26 15:57:55]

Done.

+                        PermissionBubbleRequest* b) {
+  if (a == b)
+    return true;
+  if (a->GetMessageTextFragment() == b->GetMessageTextFragment() &&
+      a->GetRequestingHostname() == b->GetRequestingHostname()) {

[felt, 2016/01/23 16:57:28]

This equality check is bugging me a little bit. I know you didn't introduce it,
but before we start using it for more things it would be good to make sure it's
correct.

My concern is that each permission request implementation defines the requesting
hostname in their own way. So it might be equivalent to the security origin, or
it might not be.

If it is equivalent to the precise security origin, then I suppose comparing the
two GURLs does what we want.

If it is different... for example, if some portion of the URL has been removed
for the sake of display... then could that lead to an inaccurate comparison?

Perhaps the root cause of the issue is that the code around the hostnames here
should be more clear about whether it's about a hostname for display, or a
security origin for meaningful security boundaries. (And whether we care about
the distinction. Maybe it's reasonable for the hostname for display to map to
multiple different origins.)

Have you given this any thought?

[johnme, 2016/01/26 15:57:55]

Hmm, interesting. I've put together https://codereview.chromium.org/1637913002
which renames GetRequestingHostname to GetOrigin, and rebased this onto that
patch.

Is this better? It still leaves us doing `a->GetOrigin() == b->GetOrigin()`
here. I wasn't sure if that was better or worse than doing:

url::Origin(a->GetOrigin()).IsSameOriginWith(url::Origin(b->GetOrigin()))

Possibly neither is quite right, since the former doesn't handle unique origins,
whereas the latter is perhaps overly generous in treating all file: URLs the
same (though maybe the former does that too?).

+    return true;
   }
   return false;
 }
 
+PermissionBubbleRequest* PermissionBubbleManager::GetExistingRequest(
+    PermissionBubbleRequest* request) {
+  for (PermissionBubbleRequest* existing_request : requests_)
+    if (MessageTextIsEqual(existing_request, request))
+      return existing_request;
+  for (PermissionBubbleRequest* existing_request : queued_requests_)
+    if (MessageTextIsEqual(existing_request, request))
+      return existing_request;
+  for (PermissionBubbleRequest* existing_request : queued_frame_requests_)
+    if (MessageTextIsEqual(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))
+      << "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);

[felt, 2016/01/23 16:51:43]

Should you remove the duplicates in the other XIncludingDuplicates as well?

[johnme, 2016/01/26 15:57:55]

No, RequestFinished is special. It is documented as:

"It is safe for the request to be deleted at this point -- it will receive no
further message from the permission bubble system. This method will eventually
be called on every request which is not unregistered."

and calls to it are always followed by the request pointer being removed from
requests_, queued_requests_, or queued_frame_requests_.

i.e. we can think of it as PermissionBubbleManager releasing a ref on the
request; and when this happens, it makes sense for the request to similarly
release the refs on the duplicates.

+}
+
 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();
   }
 }