Reland "Kill quiet permission requests if a new request is made"

This is a reland of 55a4bb68

The original CL caused a crash (crbug.com/1016233)

The first patchset is the original CL, unmodified.

https://chromium-review.googlesource.com/c/chromium/src/+/1860015
introduced a crash because InfoBarService::RemoveInfoBar DCHECKS that
the infobar is part of the owned list of infobars. When the tab is
closed, the infobar is removed but then the PermissionPromptAndroid
object destructor attempts to remove it again. This CL ensures that
that this situation does not happen by listening for the infobar
remove event and ensuring we don't try to remove the infobar that
was already removed.

Original change's description:
> Kill quiet permission requests if a new request is made
>
> Bug: 1014026
> Change-Id: I5fe93b9bc7b39873900d9e15dcba66dfddd7c4ef
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1860015
> Commit-Queue: Andy Paicu <andypaicu@chromium.org>
> Reviewed-by: Balazs Engedy <engedy@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#706445}

Bug: 1014026, 1016233
Change-Id: Iaa857811c6617afd97af9708a51ea49ea98fe385
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1871698
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708160}

chrome/browser/permissions/mock_permission_request.cc

@@ -19,7 +19,8 @@ MockPermissionRequest::MockPermissionRequest()
                             "button",
                             GURL("http://www.google.com"),
                             PermissionRequestType::PERMISSION_NOTIFICATIONS,
-                            PermissionRequestGestureType::UNKNOWN) {}
+                            PermissionRequestGestureType::UNKNOWN,
+                            CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {}
 
 MockPermissionRequest::MockPermissionRequest(const std::string& text)
     : MockPermissionRequest(text,
@@ -27,7 +28,8 @@ MockPermissionRequest::MockPermissionRequest(const std::string& text)
                             "button",
                             GURL("http://www.google.com"),
                             PermissionRequestType::PERMISSION_NOTIFICATIONS,
-                            PermissionRequestGestureType::UNKNOWN) {}
+                            PermissionRequestGestureType::UNKNOWN,
+                            CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {}
 
 MockPermissionRequest::MockPermissionRequest(
     const std::string& text,
@@ -36,9 +38,10 @@ MockPermissionRequest::MockPermissionRequest(
     : MockPermissionRequest(text,
                             "button",
                             "button",
-                             GURL("http://www.google.com"),
-                             request_type,
-                             gesture_type) {}
+                            GURL("http://www.google.com"),
+                            request_type,
+                            gesture_type,
+                            CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {}
 
 MockPermissionRequest::MockPermissionRequest(const std::string& text,
                                              PermissionRequestType request_type,
@@ -48,7 +51,8 @@ MockPermissionRequest::MockPermissionRequest(const std::string& text,
                             "button",
                             url,
                             request_type,
-                            PermissionRequestGestureType::UNKNOWN) {}
+                            PermissionRequestGestureType::UNKNOWN,
+                            CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {}
 
 MockPermissionRequest::MockPermissionRequest(const std::string& text,
                                              const std::string& accept_label,
@@ -58,7 +62,18 @@ MockPermissionRequest::MockPermissionRequest(const std::string& text,
                             deny_label,
                             GURL("http://www.google.com"),
                             PermissionRequestType::PERMISSION_NOTIFICATIONS,
-                            PermissionRequestGestureType::UNKNOWN) {}
+                            PermissionRequestGestureType::UNKNOWN,
+                            CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {}
+MockPermissionRequest::MockPermissionRequest(
+    const std::string& text,
+    ContentSettingsType content_settings_type_)
+    : MockPermissionRequest(text,
+                            "button",
+                            "button",
+                            GURL("http://www.google.com"),
+                            PermissionRequestType::PERMISSION_NOTIFICATIONS,
+                            PermissionRequestGestureType::UNKNOWN,
+                            content_settings_type_) {}
 
 MockPermissionRequest::~MockPermissionRequest() {}
 
@@ -115,6 +130,10 @@ PermissionRequestGestureType MockPermissionRequest::GetGestureType()
   return gesture_type_;
 }
 
+ContentSettingsType MockPermissionRequest::GetContentSettingsType() const {
+  return content_settings_type_;
+}
+
 bool MockPermissionRequest::granted() {
   return granted_;
 }
@@ -133,12 +152,14 @@ MockPermissionRequest::MockPermissionRequest(
     const std::string& deny_label,
     const GURL& origin,
     PermissionRequestType request_type,
-    PermissionRequestGestureType gesture_type)
+    PermissionRequestGestureType gesture_type,
+    ContentSettingsType content_settings_type)
     : granted_(false),
       cancelled_(false),
       finished_(false),
       request_type_(request_type),
-      gesture_type_(gesture_type) {
+      gesture_type_(gesture_type),
+      content_settings_type_(content_settings_type) {
   text_ = base::UTF8ToUTF16(text);
   accept_label_ = base::UTF8ToUTF16(accept_label);
   deny_label_ = base::UTF8ToUTF16(deny_label);

chrome/browser/permissions/mock_permission_request.h

@@ -22,6 +22,8 @@ class MockPermissionRequest : public PermissionRequest {
   MockPermissionRequest(const std::string& text,
                         const std::string& accept_label,
                         const std::string& deny_label);
+  MockPermissionRequest(const std::string& text,
+                        ContentSettingsType content_settings_type_);
 
   ~MockPermissionRequest() override;
 
@@ -39,6 +41,7 @@ class MockPermissionRequest : public PermissionRequest {
   void RequestFinished() override;
   PermissionRequestType GetPermissionRequestType() const override;
   PermissionRequestGestureType GetGestureType() const override;
+  ContentSettingsType GetContentSettingsType() const override;
 
   bool granted();
   bool cancelled();
@@ -50,12 +53,14 @@ class MockPermissionRequest : public PermissionRequest {
                         const std::string& deny_label,
                         const GURL& url,
                         PermissionRequestType request_type,
-                        PermissionRequestGestureType gesture_type);
+                        PermissionRequestGestureType gesture_type,
+                        ContentSettingsType content_settings_type);
   bool granted_;
   bool cancelled_;
   bool finished_;
   PermissionRequestType request_type_;
   PermissionRequestGestureType gesture_type_;
+  ContentSettingsType content_settings_type_;
 
   base::string16 text_;
   base::string16 accept_label_;

chrome/browser/permissions/permission_prompt_android.cc

@@ -23,6 +23,7 @@ PermissionPromptAndroid::PermissionPromptAndroid(
     : web_contents_(web_contents),
       delegate_(delegate),
       permission_request_notification_(nullptr),
+      permission_infobar_(nullptr),
       weak_factory_(this) {
   DCHECK(web_contents);
 
@@ -31,8 +32,9 @@ PermissionPromptAndroid::PermissionPromptAndroid(
   if (infobar_service &&
       GroupedPermissionInfoBarDelegate::ShouldShowMiniInfobar(
           GetContentSettingType(0u /* position */))) {
-    GroupedPermissionInfoBarDelegate::Create(weak_factory_.GetWeakPtr(),
-                                             infobar_service);
+    permission_infobar_ = GroupedPermissionInfoBarDelegate::Create(
+        weak_factory_.GetWeakPtr(), infobar_service);
+    infobar_service->AddObserver(this);
     return;
   }
 
@@ -46,7 +48,17 @@ PermissionPromptAndroid::PermissionPromptAndroid(
   PermissionDialogDelegate::Create(web_contents_, this);
 }
 
-PermissionPromptAndroid::~PermissionPromptAndroid() {}
+PermissionPromptAndroid::~PermissionPromptAndroid() {
+  if (permission_infobar_) {
+    InfoBarService* infobar_service =
+        InfoBarService::FromWebContents(web_contents_);
+
+    // RemoveObserver before RemoveInfoBar to not get notified about the removal
+    // of the `permission_infobar_` infobar.
+    infobar_service->RemoveObserver(this);
+    infobar_service->RemoveInfoBar(permission_infobar_);
+  }
+}
 
 void PermissionPromptAndroid::UpdateAnchorPosition() {
   NOTREACHED() << "UpdateAnchorPosition is not implemented";
@@ -131,6 +143,17 @@ base::string16 PermissionPromptAndroid::GetMessageText() const {
           url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC));
 }
 
+void PermissionPromptAndroid::OnInfoBarRemoved(infobars::InfoBar* infobar,
+                                               bool animate) {
+  if (infobar != permission_infobar_)
+    return;
+
+  permission_infobar_ = nullptr;
+  InfoBarService* infobar_service =
+      InfoBarService::FromWebContents(web_contents_);
+  infobar_service->RemoveObserver(this);
+}
+
 // static
 std::unique_ptr<PermissionPrompt> PermissionPrompt::Create(
     content::WebContents* web_contents,

chrome/browser/permissions/permission_prompt_android.h

@@ -13,13 +13,18 @@
 #include "chrome/browser/permissions/permission_request_notification_android.h"
 #include "chrome/browser/ui/permission_bubble/permission_prompt.h"
 #include "components/content_settings/core/common/content_settings_types.h"
+#include "components/infobars/core/infobar_manager.h"
 
 namespace content {
 class WebContents;
 }
+namespace infobars {
+class InfoBar;
+}
 class PermissionRequestNotificationAndroid;
 
-class PermissionPromptAndroid : public PermissionPrompt {
+class PermissionPromptAndroid : public PermissionPrompt,
+                                public infobars::InfoBarManager::Observer {
  public:
   PermissionPromptAndroid(content::WebContents* web_contents,
                           Delegate* delegate);
@@ -42,6 +47,9 @@ class PermissionPromptAndroid : public PermissionPrompt {
   base::string16 GetTitleText() const;
   base::string16 GetMessageText() const;
 
+  // InfoBar::Manager:
+  void OnInfoBarRemoved(infobars::InfoBar* infobar, bool animate) override;
+
  private:
   // PermissionPromptAndroid is owned by PermissionRequestManager, so it should
   // be safe to hold a raw WebContents pointer here because this class is
@@ -55,6 +63,10 @@ class PermissionPromptAndroid : public PermissionPrompt {
   std::unique_ptr<PermissionRequestNotificationAndroid>
       permission_request_notification_;
 
+  // The infobar used to display the permission request, if displayed in that
+  // format. Never assume that this pointer is currently alive.
+  infobars::InfoBar* permission_infobar_;
+
   base::WeakPtrFactory<PermissionPromptAndroid> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(PermissionPromptAndroid);

chrome/browser/permissions/permission_prompt_android_unittest.cc

+// Copyright 2019 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/permissions/permission_prompt_android.h"
+
+#include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/infobars/mock_infobar_service.h"
+#include "chrome/browser/permissions/mock_permission_request.h"
+#include "chrome/browser/permissions/permission_features.h"
+#include "chrome/browser/permissions/permission_request_manager.h"
+#include "chrome/common/chrome_features.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+
+class PermissionPromptAndroidTest : public ChromeRenderViewHostTestHarness {
+ public:
+  PermissionRequestManager* permission_request_manager() {
+    return permission_request_manager_;
+  }
+
+ private:
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+    // Ensure that the test uses the mini-infobar variant.
+    base::FieldTrialParams params;
+    params[kQuietNotificationPromptsUIFlavourParameterName] =
+        kQuietNotificationPromptsMiniInfobar;
+    scoped_feature_list_.InitAndEnableFeatureWithParameters(
+        features::kQuietNotificationPrompts, params);
+
+    MockInfoBarService::CreateForWebContents(web_contents());
+
+    PermissionRequestManager::CreateForWebContents(web_contents());
+    permission_request_manager_ =
+        PermissionRequestManager::FromWebContents(web_contents());
+  }
+
+  base::test::ScopedFeatureList scoped_feature_list_;
+  PermissionRequestManager* permission_request_manager_;
+};
+
+TEST_F(PermissionPromptAndroidTest, TabCloseMiniInfoBarClosesCleanly) {
+  // Create a notification request. This causes an infobar to appear.
+  MockPermissionRequest request("test", CONTENT_SETTINGS_TYPE_NOTIFICATIONS);
+  permission_request_manager()->AddRequest(&request);
+
+  permission_request_manager()->DocumentOnLoadCompletedInMainFrame();
+  base::RunLoop().RunUntilIdle();
+
+  // Now remove the infobar from the infobar service.
+  InfoBarService::FromWebContents(web_contents())->RemoveAllInfoBars(false);
+
+  // At this point close the permission prompt (after the infobar has been
+  // removed already).
+  permission_request_manager()->Deny();
+
+  // If no DCHECK has been hit, and the infobar has been closed, the test
+  // passes.
+  EXPECT_TRUE(request.finished());
+}

chrome/browser/permissions/permission_request_manager.cc

@@ -80,7 +80,6 @@ PermissionRequestManager::~PermissionRequestManager() {
 }
 
 void PermissionRequestManager::AddRequest(PermissionRequest* request) {
-
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDenyPermissionPrompts)) {
     request->PermissionDenied();
@@ -140,6 +139,12 @@ void PermissionRequestManager::AddRequest(PermissionRequest* request) {
   }
   queued_requests_.push_back(request);
 
+  // If we're displaying a quiet permission request, kill it in favor of this
+  // permission request.
+  if (ShouldShowQuietPermissionPrompt()) {
+    FinalizeBubble(PermissionAction::IGNORED);
+  }
+
   if (!IsBubbleVisible())
     ScheduleShowBubble();
 }
@@ -553,17 +558,21 @@ void PermissionRequestManager::RemoveObserver(Observer* observer) {
 }
 
 bool PermissionRequestManager::ShouldShowQuietPermissionPrompt() {
-  if (!requests_.size())
+  if (!requests_.size() ||
+      requests_.front()->GetPermissionRequestType() !=
+          PermissionRequestType::PERMISSION_NOTIFICATIONS) {
     return false;
+  }
 
-#if !defined(OS_ANDROID)
   const auto ui_flavor = QuietNotificationsPromptConfig::UIFlavorToUse();
-  return (requests_.front()->GetPermissionRequestType() ==
-              PermissionRequestType::PERMISSION_NOTIFICATIONS &&
-          (ui_flavor == QuietNotificationsPromptConfig::STATIC_ICON ||
-           ui_flavor == QuietNotificationsPromptConfig::ANIMATED_ICON));
+#if !defined(OS_ANDROID)
+  return ui_flavor == QuietNotificationsPromptConfig::STATIC_ICON ||
+         ui_flavor == QuietNotificationsPromptConfig::ANIMATED_ICON;
 #else   // OS_ANDROID
-  return false;
+  return ui_flavor == QuietNotificationsPromptConfig::QUIET_NOTIFICATION ||
+         ui_flavor == QuietNotificationsPromptConfig::HEADS_UP_NOTIFICATION ||
+         ui_flavor == QuietNotificationsPromptConfig::MINI_INFOBAR;
+
 #endif  // OS_ANDROID
 }
 

chrome/test/BUILD.gn

@@ -3636,6 +3636,7 @@ test("unit_tests") {
       "../browser/password_manager/save_password_infobar_delegate_android_unittest.cc",
       "../browser/password_manager/touch_to_fill_controller_unittest.cc",
       "../browser/password_manager/update_password_infobar_delegate_android_unittest.cc",
+      "../browser/permissions/permission_prompt_android_unittest.cc",
       "../browser/permissions/permission_request_notification_android_unittest.cc",
       "../browser/permissions/permission_request_notification_handler_unittest.cc",
       "../browser/translate/translate_manager_render_view_host_android_unittest.cc",