Scope PermissionPrompt lifetime to UI visibility

This patch make the lifetime of PermissionPrompt objects more sensible
and match what we occasionally think it actually is. Currently these
objects exist when a tab is active, or on Android all the time. This
patch changes it so that, as the name might suggest, it corresponds to
when a prompt is actually visible. We retain the distinction between
desktop/Android where desktop hides the UI (now deletes the object) on
tab switching, while Android retains the UI (keeps the object alive)
as the InfoBar system manages hiding the prompt.

Since we don't explicitly manage lifetimes of the actual UI surface on
Android (infobars in particular, as it's clearer for modals when the UI
is actually destroyed) from the PermissionPrompt, we change the pointer
to a WeakPtr for safety. When the PermissionPrompt is destroyed due to
navigation, if an infobar happens to still persist due to a bug it
won't be able to be resolve different permission requests.

Bug: 606138, 737102
Change-Id: I26a8843b9b446e91282a9b0ce67c4b442bf440e9
Reviewed-on: https://chromium-review.googlesource.com/567940
Commit-Queue: Timothy Loh <timloh@chromium.org>
Reviewed-by: Raymes Khoury <raymes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487029}

chrome/browser/permissions/grouped_permission_infobar_delegate_android.cc

@@ -21,7 +21,7 @@ GroupedPermissionInfoBarDelegate::~GroupedPermissionInfoBarDelegate() {}
 
 // static
 infobars::InfoBar* GroupedPermissionInfoBarDelegate::Create(
-    PermissionPromptAndroid* permission_prompt,
+    const base::WeakPtr<PermissionPromptAndroid>& permission_prompt,
     InfoBarService* infobar_service,
     const GURL& requesting_origin) {
   return infobar_service->AddInfoBar(base::MakeUnique<GroupedPermissionInfoBar>(
@@ -34,6 +34,8 @@ size_t GroupedPermissionInfoBarDelegate::PermissionCount() const {
 }
 
 bool GroupedPermissionInfoBarDelegate::ShouldShowPersistenceToggle() const {
+  if (!permission_prompt_)
+    return false;
   return permission_prompt_->ShouldShowPersistenceToggle();
 }
 
@@ -78,7 +80,7 @@ base::string16 GroupedPermissionInfoBarDelegate::GetLinkText() const {
 }
 
 GroupedPermissionInfoBarDelegate::GroupedPermissionInfoBarDelegate(
-    PermissionPromptAndroid* permission_prompt,
+    const base::WeakPtr<PermissionPromptAndroid>& permission_prompt,
     const GURL& requesting_origin)
     : requesting_origin_(requesting_origin),
       persist_(true),

chrome/browser/permissions/grouped_permission_infobar_delegate_android.h

@@ -24,9 +24,10 @@ class GroupedPermissionInfoBarDelegate : public ConfirmInfoBarDelegate {
   // Public so we can have std::unique_ptr<GroupedPermissionInfoBarDelegate>.
   ~GroupedPermissionInfoBarDelegate() override;
 
-  static infobars::InfoBar* Create(PermissionPromptAndroid* permission_prompt,
-                                   InfoBarService* infobar_service,
-                                   const GURL& requesting_origin);
+  static infobars::InfoBar* Create(
+      const base::WeakPtr<PermissionPromptAndroid>& permission_prompt,
+      InfoBarService* infobar_service,
+      const GURL& requesting_origin);
 
   bool persist() const { return persist_; }
   void set_persist(bool persist) { persist_ = persist; }
@@ -52,8 +53,9 @@ class GroupedPermissionInfoBarDelegate : public ConfirmInfoBarDelegate {
   bool GetAcceptState(size_t position);
 
  private:
-  GroupedPermissionInfoBarDelegate(PermissionPromptAndroid* permission_prompt,
-                                   const GURL& requesting_origin);
+  GroupedPermissionInfoBarDelegate(
+      const base::WeakPtr<PermissionPromptAndroid>& permission_prompt,
+      const GURL& requesting_origin);
 
   // ConfirmInfoBarDelegate:
   InfoBarIdentifier GetIdentifier() const override;
@@ -68,7 +70,7 @@ class GroupedPermissionInfoBarDelegate : public ConfirmInfoBarDelegate {
   const GURL requesting_origin_;
   // Whether the accept/deny decision is persisted.
   bool persist_;
-  PermissionPromptAndroid* permission_prompt_;
+  base::WeakPtr<PermissionPromptAndroid> permission_prompt_;
 
   DISALLOW_COPY_AND_ASSIGN(GroupedPermissionInfoBarDelegate);
 };

chrome/browser/permissions/permission_dialog_delegate.h

@@ -100,9 +100,9 @@ class PermissionDialogDelegate : public content::WebContentsObserver {
   // delete the PermissionQueueController.
   std::unique_ptr<PermissionInfoBarDelegate> infobar_delegate_;
 
-  // The PermissionPromptAndroid is alive until the tab navigates or is closed.
-  // We close the prompt on DidFinishNavigation and WebContentsDestroyed, so it
-  // should always be safe to use this pointer.
+  // The PermissionPromptAndroid is deleted when either the dialog is resolved
+  // or the tab is navigated/closed. We close the prompt on DidFinishNavigation
+  // and WebContentsDestroyed, so it should always be safe to use this pointer.
   PermissionPromptAndroid* permission_prompt_;
 
   DISALLOW_COPY_AND_ASSIGN(PermissionDialogDelegate);

chrome/browser/permissions/permission_prompt_android.cc

@@ -19,7 +19,10 @@
 
 PermissionPromptAndroid::PermissionPromptAndroid(
     content::WebContents* web_contents)
-    : web_contents_(web_contents), delegate_(nullptr), persist_(true) {
+    : web_contents_(web_contents),
+      delegate_(nullptr),
+      persist_(true),
+      weak_factory_(this) {
   DCHECK(web_contents);
 }
 
@@ -46,7 +49,8 @@ void PermissionPromptAndroid::Show() {
     return;
 
   GroupedPermissionInfoBarDelegate::Create(
-      this, infobar_service, delegate_->Requests()[0]->GetOrigin());
+      weak_factory_.GetWeakPtr(), infobar_service,
+      delegate_->Requests()[0]->GetOrigin());
 }
 
 bool PermissionPromptAndroid::CanAcceptRequestUpdate() {

chrome/browser/permissions/permission_prompt_android.h

@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/ui/permission_bubble/permission_prompt.h"
 #include "components/content_settings/core/common/content_settings_types.h"
@@ -56,6 +57,8 @@ class PermissionPromptAndroid : public PermissionPrompt {
 
   bool persist_;
 
+  base::WeakPtrFactory<PermissionPromptAndroid> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(PermissionPromptAndroid);
 };
 

chrome/browser/permissions/permission_request_manager.cc

@@ -112,12 +112,12 @@ PermissionRequestManager::PermissionRequestManager(
       view_factory_(base::Bind(&PermissionPrompt::Create)),
       view_(nullptr),
       main_frame_has_fully_loaded_(false),
+      tab_can_show_prompts_(false),
       persist_(true),
       auto_response_for_test_(NONE),
       weak_factory_(this) {
 #if defined(OS_ANDROID)
-  view_ = view_factory_.Run(web_contents);
-  view_->SetDelegate(this);
+  tab_can_show_prompts_ = true;
 #endif
 }
 
@@ -195,16 +195,18 @@ void PermissionRequestManager::CancelRequest(PermissionRequest* request) {
 
     // 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 = !view_ || view_->CanAcceptRequestUpdate();
+    bool can_erase = view_->CanAcceptRequestUpdate();
     if (can_erase) {
+      DeleteBubble();
+
       RequestFinishedIncludingDuplicates(*requests_iter);
       requests_.erase(requests_iter);
 
-      if (view_) {
-        view_->Hide();
-        // Will redraw the bubble if it is being shown.
+      if (requests_.empty())
         DequeueRequestsAndShowBubble();
-      }
+      else
+        ShowBubble();
+
       return;
     }
 
@@ -234,22 +236,14 @@ void PermissionRequestManager::CancelRequest(PermissionRequest* request) {
 }
 
 void PermissionRequestManager::HideBubble() {
-  // Disengage from the existing view if there is one and it doesn't manage
-  // its own visibility.
-  if (!view_ || view_->HidesAutomatically())
-    return;
+  tab_can_show_prompts_ = false;
 
-  view_->SetDelegate(nullptr);
-  view_->Hide();
-  view_.reset();
+  if (view_)
+    DeleteBubble();
 }
 
 void PermissionRequestManager::DisplayPendingRequests() {
-  if (IsBubbleVisible())
-    return;
-
-  view_ = view_factory_.Run(web_contents());
-  view_->SetDelegate(this);
+  tab_can_show_prompts_ = true;
 
   if (!main_frame_has_fully_loaded_)
     return;
@@ -294,8 +288,7 @@ void PermissionRequestManager::DidFinishNavigation(
     return;
   }
 
-  CancelPendingQueues();
-  FinalizeBubble();
+  CleanUpRequests();
   main_frame_has_fully_loaded_ = false;
 }
 
@@ -316,8 +309,7 @@ void PermissionRequestManager::DocumentLoadedInFrame(
 
 void PermissionRequestManager::WebContentsDestroyed() {
   // If the web contents has been destroyed, treat the bubble as cancelled.
-  CancelPendingQueues();
-  FinalizeBubble();
+  CleanUpRequests();
 
   // The WebContents is going away; be aggressively paranoid and delete
   // ourselves lest other parts of the system attempt to add permission bubbles
@@ -359,6 +351,11 @@ void PermissionRequestManager::Deny() {
 }
 
 void PermissionRequestManager::Closing() {
+#if defined(OS_MACOSX)
+  // Mac calls this whenever you press Esc.
+  if (!view_)
+    return;
+#endif
   std::vector<PermissionRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
        requests_iter != requests_.end();
@@ -381,15 +378,14 @@ void PermissionRequestManager::ScheduleShowBubble() {
 }
 
 void PermissionRequestManager::DequeueRequestsAndShowBubble() {
-  if (!view_)
-    return;
-  if (!requests_.empty())
+  if (view_)
     return;
-  if (!main_frame_has_fully_loaded_)
+  if (!main_frame_has_fully_loaded_ || !tab_can_show_prompts_)
     return;
   if (queued_requests_.empty())
     return;
 
+  DCHECK(requests_.empty());
   requests_.push_back(queued_requests_.front());
   queued_requests_.pop_front();
 
@@ -403,10 +399,13 @@ void PermissionRequestManager::DequeueRequestsAndShowBubble() {
 }
 
 void PermissionRequestManager::ShowBubble() {
-  DCHECK(view_);
+  DCHECK(!view_);
   DCHECK(!requests_.empty());
   DCHECK(main_frame_has_fully_loaded_);
+  DCHECK(tab_can_show_prompts_);
 
+  view_ = view_factory_.Run(web_contents());
+  view_->SetDelegate(this);
   view_->Show();
   PermissionUmaUtil::PermissionPromptShown(requests_);
   NotifyBubbleAdded();
@@ -416,9 +415,19 @@ void PermissionRequestManager::ShowBubble() {
     DoAutoResponseForTesting();
 }
 
-void PermissionRequestManager::FinalizeBubble() {
-  if (view_ && !view_->HidesAutomatically())
+void PermissionRequestManager::DeleteBubble() {
+  DCHECK(view_);
+  if (!view_->HidesAutomatically())
     view_->Hide();
+  view_->SetDelegate(nullptr);
+  view_.reset();
+}
+
+void PermissionRequestManager::FinalizeBubble() {
+  DCHECK(view_);
+  DCHECK(!requests_.empty());
+
+  DeleteBubble();
 
   std::vector<PermissionRequest*>::iterator requests_iter;
   for (requests_iter = requests_.begin();
@@ -431,7 +440,7 @@ void PermissionRequestManager::FinalizeBubble() {
     DequeueRequestsAndShowBubble();
 }
 
-void PermissionRequestManager::CancelPendingQueues() {
+void PermissionRequestManager::CleanUpRequests() {
   std::deque<PermissionRequest*>::iterator requests_iter;
   for (requests_iter = queued_requests_.begin();
        requests_iter != queued_requests_.end();
@@ -439,6 +448,9 @@ void PermissionRequestManager::CancelPendingQueues() {
     RequestFinishedIncludingDuplicates(*requests_iter);
   }
   queued_requests_.clear();
+
+  if (view_)
+    FinalizeBubble();
 }
 
 PermissionRequest* PermissionRequestManager::GetExistingRequest(

chrome/browser/permissions/permission_request_manager.h

@@ -77,6 +77,7 @@ class PermissionRequestManager
   // Temporarily hides the bubble, and destroys the prompt UI surface. Any
   // existing requests will be reshown when DisplayPendingRequests is called
   // (e.g. when switching tabs away and back to a page with a prompt).
+  // TODO(timloh): Rename this to something more fitting (e.g. TabSwitchedAway).
   void HideBubble();
 
   // Will show a permission bubble if there is a pending permission request on
@@ -152,13 +153,17 @@ class PermissionRequestManager
   // bubble after switching tabs away and back.
   void ShowBubble();
 
-  // Finalize the pending permissions request.
+  // Delete the view object
+  void DeleteBubble();
+
+  // Delete the view object, finalize requests, asynchronously show a queued
+  // request if present.
   void FinalizeBubble();
 
-  // Cancel any pending requests. This is called if the WebContents
-  // on which permissions calls are pending is destroyed or navigated away
-  // from the requesting page.
-  void CancelPendingQueues();
+  // Cancel all pending or active requests and destroy the PermissionPrompt if
+  // one exists. This is called if the WebContents is destroyed or navigates its
+  // main frame.
+  void CleanUpRequests();
 
   // Searches |requests_|, |queued_requests_| and |queued_frame_requests_| - but
   // *not* |duplicate_requests_| - for a request matching |request|, and returns
@@ -182,8 +187,13 @@ class PermissionRequestManager
   // Factory to be used to create views when needed.
   PermissionPrompt::Factory view_factory_;
 
-  // The UI surface to be used to display the permissions requests.
+  // The UI surface for an active permission prompt if we're displaying one.
   std::unique_ptr<PermissionPrompt> view_;
+  // We only show prompts when both of these are true. On Desktop, we hide any
+  // active prompt on tab switching, while on Android we let the infobar system
+  // handle it.
+  bool main_frame_has_fully_loaded_;
+  bool tab_can_show_prompts_;
 
   std::vector<PermissionRequest*> requests_;
   std::deque<PermissionRequest*> queued_requests_;
@@ -192,8 +202,6 @@ class PermissionRequestManager
   std::unordered_multimap<PermissionRequest*, PermissionRequest*>
       duplicate_requests_;
 
-  bool main_frame_has_fully_loaded_;
-
   // Whether the response to each request should be persisted.
   bool persist_;
 

chrome/browser/permissions/permission_request_manager_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/histogram_tester.h"
+#include "build/build_config.h"
 #include "chrome/browser/permissions/mock_permission_request.h"
 #include "chrome/browser/permissions/permission_request.h"
 #include "chrome/browser/permissions/permission_request_manager.h"
@@ -199,12 +200,14 @@ TEST_F(PermissionRequestManagerTest, NoRequests) {
   EXPECT_FALSE(prompt_factory_->is_visible());
 }
 
-TEST_F(PermissionRequestManagerTest, NoView) {
+#if !defined(OS_ANDROID)
+TEST_F(PermissionRequestManagerTest, PermissionRequestWhileTabSwitchedAway) {
   manager_->AddRequest(&request1_);
-  // Don't display the pending requests.
+  // Don't mark the tab as active.
   WaitForBubbleToBeShown();
   EXPECT_FALSE(prompt_factory_->is_visible());
 }
+#endif
 
 TEST_F(PermissionRequestManagerTest, TwoRequestsDoNotCoalesce) {
   manager_->DisplayPendingRequests();