Handles iframe permissions requests separately, in a subsequent bubble.

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 "chrome/browser/ui/website_settings/permission_bubble_request.h"
 #include "chrome/common/chrome_switches.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_details.h"
 #include "content/public/browser/user_metrics.h"
 
 namespace {
 
 class CancelledRequest : public PermissionBubbleRequest {
  public:
   explicit CancelledRequest(PermissionBubbleRequest* cancelled)
       : icon_(cancelled->GetIconID()),
         message_text_(cancelled->GetMessageText()),
         message_fragment_(cancelled->GetMessageTextFragment()),
@@ skipping 72 matching lines @@
 
 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 = request->GetRequestingHostname() == request_url_;

[Greg Billock, 2014/05/21 17:09:19]

Do we need to extract just the hostport from the request_url_ to do this?

[leng, 2014/05/22 00:08:47]

I have no idea.  I don't know enough about main frame vs. iframe requests.

[Greg Billock, 2014/05/22 02:06:31]

I think GetLastCommittedURL() is the full url, and GetRequestingHostname() is
just the hostname portion of the url (formatted as a url), so the root of the
security context. So in our test cases of url == just a host it would work, but
I'm suspicious it won't for urls with paths.

[leng, 2014/05/22 23:57:05]

Got it.  I changed it to use GetOrigin() for both - we only care about same
domain vs. other domain, right?

 
   // Don't re-add an existing request or one with a duplicate text request.
-  std::vector<PermissionBubbleRequest*>::iterator requests_iter;
-  for (requests_iter = requests_.begin();
-       requests_iter != requests_.end();
-       requests_iter++) {
-    if (*requests_iter == request)
-      return;
-    // TODO(gbillock): worry about the requesting host name as well.
-    if ((*requests_iter)->GetMessageTextFragment() ==
-        request->GetMessageTextFragment()) {
+  bool exact_duplicate = false;
+  if (ExistingRequest(request, requests_, &exact_duplicate) ||

[Greg Billock, 2014/05/21 17:09:19]

Maybe exact_duplicate --> same_object ?

[leng, 2014/05/22 00:08:47]

Good call.  Done.

+      ExistingRequest(request, queued_requests_, &exact_duplicate) ||
+      ExistingRequest(request, queued_frame_requests_, &exact_duplicate)) {
+    if (!exact_duplicate)
       request->RequestFinished();
-      return;
-    }
-  }
-  for (requests_iter = queued_requests_.begin();
-       requests_iter != queued_requests_.end();
-       requests_iter++) {
-    if (*requests_iter == request)
-      return;
-    if ((*requests_iter)->GetMessageTextFragment() ==
-        request->GetMessageTextFragment()) {
-      request->RequestFinished();
-      return;
-    }
+    return;
   }
 
   if (bubble_showing_) {
     content::RecordAction(
         base::UserMetricsAction("PermissionBubbleRequestQueued"));
-    queued_requests_.push_back(request);
+    if (is_main_frame)
+      queued_requests_.push_back(request);
+    else
+      queued_frame_requests_.push_back(request);
     return;
   }
 
-  requests_.push_back(request);
+  if (is_main_frame)
+    requests_.push_back(request);
+  else
+    queued_frame_requests_.push_back(request);
+
   // TODO(gbillock): do we need to make default state a request property?
   accept_states_.push_back(true);

[Greg Billock, 2014/05/21 17:09:19]

should this only execute if is_main_frame, right?

[leng, 2014/05/22 00:08:47]

Done.

 
   if (request->HasUserGesture())
-    ShowBubble();
+    TriggerShowBubble();

[Greg Billock, 2014/05/21 17:09:19]

Should this be ScheduleShowBubble, right? (and was it an error before?) to let
any simultaneous pending registrations complete? Kind of a race condition, so
maybe no observable difference...

[leng, 2014/05/22 00:08:47]

I see no harm in making it ScheduleShowBubble(), and a slight possible upside if
there is a race.  Changed.

 }
 
 void PermissionBubbleManager::CancelRequest(PermissionBubbleRequest* request) {
   // First look in the queued requests, where we can simply delete 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) {
@@ skipping 11 matching lines @@
       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 = !bubble_showing_ ||
                      !view_ || view_->CanAcceptRequestUpdate();
     if (can_erase) {
       (*requests_iter)->RequestFinished();
       requests_.erase(requests_iter);
       accept_states_.erase(accepts_iter);
-      ShowBubble();  // Will redraw the bubble if it is being shown.
+      TriggerShowBubble();  // Will redraw the bubble if it is being shown.
       return;
     }
 
     // Cancel the existing request and replace it with a dummy.
     PermissionBubbleRequest* cancelled_request =
         new CancelledRequest(*requests_iter);
     (*requests_iter)->RequestFinished();
     *requests_iter = cancelled_request;
     return;
   }
@@ skipping 10 matching lines @@
     view_->SetDelegate(NULL);
     view_->Hide();
     bubble_showing_ = false;
   }
 
   view_ = view;
   if (!view)
     return;
 
   view->SetDelegate(this);
-  ShowBubble();
+  TriggerShowBubble();
 }
 
 void PermissionBubbleManager::DocumentOnLoadCompletedInMainFrame() {
   request_url_has_loaded_ = true;
   // This is scheduled because while all calls to the browser have been
   // issued at DOMContentLoaded, they may be bouncing around in scheduled
   // callbacks finding the UI thread still. This makes sure we allow those
   // scheduled calls to AddRequest to complete before we show the page-load
   // permissions bubble.
-  // TODO(gbillock): make this bind safe with a weak ptr.
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&PermissionBubbleManager::ShowBubble,
-                 weak_factory_.GetWeakPtr()));
+  ScheduleShowBubble();
+}
+
+void PermissionBubbleManager::DocumentLoadedInFrame(
+    int64 frame_id,
+    content::RenderViewHost* render_view_host) {
+  if (request_url_has_loaded_) {

[Greg Billock, 2014/05/21 17:09:19]

no braces

[leng, 2014/05/22 00:08:47]

Done.

+    ScheduleShowBubble();
+  }
 }
 
 void PermissionBubbleManager::NavigationEntryCommitted(
     const content::LoadCommittedDetails& details) {
   // No permissions requests pending.
   if (request_url_.is_empty())
     return;
 
-  // If we have navigated to a new url...
-  if (request_url_ != web_contents()->GetLastCommittedURL()) {
+  // If we have navigated to a new url or reloaded the page...
+  if (request_url_ != web_contents()->GetLastCommittedURL() ||
+      details.type == content::NAVIGATION_TYPE_EXISTING_PAGE) {

[Greg Billock, 2014/05/21 17:09:19]

Makes sense. We then get the re-notifications for any permissions requests
issued by the page, right?

[leng, 2014/05/22 00:08:47]

Correct.  I tested it manually.

     // Kill off existing bubble and cancel any pending requests.
     CancelPendingQueue();
     FinalizeBubble();
   }
 }
 
 void PermissionBubbleManager::WebContentsDestroyed() {
   // If the web contents has been destroyed, treat the bubble as cancelled.
   CancelPendingQueue();
   FinalizeBubble();
@@ skipping 44 matching lines @@
 void PermissionBubbleManager::Closing() {
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
     (*requests_iter)->Cancelled();
   }
   FinalizeBubble();
 }
 
-// TODO(gbillock): find a better name for this method.
-void PermissionBubbleManager::ShowBubble() {
+void PermissionBubbleManager::ScheduleShowBubble() {
+  content::BrowserThread::PostTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&PermissionBubbleManager::TriggerShowBubble,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void PermissionBubbleManager::TriggerShowBubble() {
   if (!view_)
     return;
-  if (requests_.empty())
-    return;
   if (bubble_showing_)
     return;
   if (!request_url_has_loaded_)
     return;
+  if (requests_.empty() &&
+      queued_requests_.empty() &&
+      queued_frame_requests_.empty())
+    return;
+
+  if (requests_.empty()) {
+    if (queued_requests_.size())
+      requests_.swap(queued_requests_);
+    else
+      requests_.swap(queued_frame_requests_);
+    accept_states_.resize(requests_.size(), true);

[Greg Billock, 2014/05/21 17:09:19]

Maybe a comment that the 'true' here is setting their value? I felt like I
wanted to look that up to make sure I had it right.

[leng, 2014/05/22 00:08:47]

Done.

+  }
 
   // Note: this should appear above Show() for testing, since in that
   // case we may do in-line calling of finalization.
   bubble_showing_ = true;
   view_->Show(requests_, accept_states_, customization_mode_);
 }
 
 void PermissionBubbleManager::FinalizeBubble() {
   if (view_)
     view_->Hide();
   bubble_showing_ = false;
 
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
        requests_iter++) {
     (*requests_iter)->RequestFinished();
   }
   requests_.clear();
   accept_states_.clear();
-  if (queued_requests_.size()) {
-    requests_ = queued_requests_;
-    accept_states_.resize(requests_.size(), true);
-    queued_requests_.clear();
-    ShowBubble();
+  if (queued_requests_.size() || queued_frame_requests_.size()) {
+    TriggerShowBubble();
   } else {
     request_url_ = GURL();
   }
 }
 
 void PermissionBubbleManager::CancelPendingQueue() {
   std::vector<PermissionBubbleRequest*>::iterator requests_iter;
   for (requests_iter = queued_requests_.begin();
        requests_iter != queued_requests_.end();
        requests_iter++) {
     (*requests_iter)->RequestFinished();
   }
 }
+
+bool PermissionBubbleManager::ExistingRequest(
+    PermissionBubbleRequest* request,
+    const std::vector<PermissionBubbleRequest*>& queue,
+    bool* exact_duplicate) {
+  std::vector<PermissionBubbleRequest*>::const_iterator iter;
+  for (iter = queue.begin();

[Greg Billock, 2014/05/21 17:09:19]

fits on one line?

[leng, 2014/05/22 00:08:47]

Done.

+       iter != queue.end();
+       iter++) {
+    if (*iter == request) {
+      if (exact_duplicate)

[Greg Billock, 2014/05/21 17:09:19]

we can stipulate that CHECK(exact_duplicate)

[leng, 2014/05/22 00:08:47]

Done.

+        *exact_duplicate = true;
+      return true;
+    }
+    if ((*iter)->GetMessageTextFragment() ==
+        request->GetMessageTextFragment() &&
+        (*iter)->GetRequestingHostname() ==
+        request->GetRequestingHostname()) {
+      if (exact_duplicate)
+        *exact_duplicate = false;
+      return true;
+    }
+  }
+  return false;
+}