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

This reverts commit 905e57cc.

Reason for revert: Still causing crashes in canary.

Original change's description:
> 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}

TBR=engedy@chromium.org,andypaicu@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1014026, 1016233
Change-Id: If930a94246d4805d95ca77bc5d5354f643b25d71
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1888385Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Reviewed-by: Ben Mason <benmason@chromium.org>
Commit-Queue: Andy Paicu <andypaicu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#710740}

chrome/browser/permissions/mock_permission_request.cc

@@ -19,8 +19,7 @@ MockPermissionRequest::MockPermissionRequest()
                             "button",
                             GURL("http://www.google.com"),
                             PermissionRequestType::PERMISSION_NOTIFICATIONS,
-                            PermissionRequestGestureType::UNKNOWN,
-                            ContentSettingsType::NOTIFICATIONS) {}
+                            PermissionRequestGestureType::UNKNOWN) {}
 
 MockPermissionRequest::MockPermissionRequest(const std::string& text)
     : MockPermissionRequest(text,
@@ -28,8 +27,7 @@ MockPermissionRequest::MockPermissionRequest(const std::string& text)
                             "button",
                             GURL("http://www.google.com"),
                             PermissionRequestType::PERMISSION_NOTIFICATIONS,
-                            PermissionRequestGestureType::UNKNOWN,
-                            ContentSettingsType::NOTIFICATIONS) {}
+                            PermissionRequestGestureType::UNKNOWN) {}
 
 MockPermissionRequest::MockPermissionRequest(
     const std::string& text,
@@ -40,8 +38,7 @@ MockPermissionRequest::MockPermissionRequest(
                             "button",
                             GURL("http://www.google.com"),
                             request_type,
-                            gesture_type,
-                            ContentSettingsType::NOTIFICATIONS) {}
+                            gesture_type) {}
 
 MockPermissionRequest::MockPermissionRequest(const std::string& text,
                                              PermissionRequestType request_type,
@@ -51,8 +48,7 @@ MockPermissionRequest::MockPermissionRequest(const std::string& text,
                             "button",
                             url,
                             request_type,
-                            PermissionRequestGestureType::UNKNOWN,
-                            ContentSettingsType::NOTIFICATIONS) {}
+                            PermissionRequestGestureType::UNKNOWN) {}
 
 MockPermissionRequest::MockPermissionRequest(const std::string& text,
                                              const std::string& accept_label,
@@ -62,18 +58,7 @@ MockPermissionRequest::MockPermissionRequest(const std::string& text,
                             deny_label,
                             GURL("http://www.google.com"),
                             PermissionRequestType::PERMISSION_NOTIFICATIONS,
-                            PermissionRequestGestureType::UNKNOWN,
-                            ContentSettingsType::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_) {}
+                            PermissionRequestGestureType::UNKNOWN) {}
 
 MockPermissionRequest::~MockPermissionRequest() {}
 
@@ -130,10 +115,6 @@ PermissionRequestGestureType MockPermissionRequest::GetGestureType()
   return gesture_type_;
 }
 
-ContentSettingsType MockPermissionRequest::GetContentSettingsType() const {
-  return content_settings_type_;
-}
-
 bool MockPermissionRequest::granted() {
   return granted_;
 }
@@ -152,14 +133,12 @@ MockPermissionRequest::MockPermissionRequest(
     const std::string& deny_label,
     const GURL& origin,
     PermissionRequestType request_type,
-    PermissionRequestGestureType gesture_type,
-    ContentSettingsType content_settings_type)
+    PermissionRequestGestureType gesture_type)
     : granted_(false),
       cancelled_(false),
       finished_(false),
       request_type_(request_type),
-      gesture_type_(gesture_type),
-      content_settings_type_(content_settings_type) {
+      gesture_type_(gesture_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,8 +22,6 @@ 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;
 
@@ -41,7 +39,6 @@ class MockPermissionRequest : public PermissionRequest {
   void RequestFinished() override;
   PermissionRequestType GetPermissionRequestType() const override;
   PermissionRequestGestureType GetGestureType() const override;
-  ContentSettingsType GetContentSettingsType() const override;
 
   bool granted();
   bool cancelled();
@@ -53,14 +50,12 @@ class MockPermissionRequest : public PermissionRequest {
                         const std::string& deny_label,
                         const GURL& url,
                         PermissionRequestType request_type,
-                        PermissionRequestGestureType gesture_type,
-                        ContentSettingsType content_settings_type);
+                        PermissionRequestGestureType gesture_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,7 +23,6 @@ PermissionPromptAndroid::PermissionPromptAndroid(
     : web_contents_(web_contents),
       delegate_(delegate),
       permission_request_notification_(nullptr),
-      permission_infobar_(nullptr),
       weak_factory_(this) {
   DCHECK(web_contents);
 
@@ -32,9 +31,8 @@ PermissionPromptAndroid::PermissionPromptAndroid(
   if (infobar_service &&
       GroupedPermissionInfoBarDelegate::ShouldShowMiniInfobar(
           GetContentSettingType(0u /* position */))) {
-    permission_infobar_ = GroupedPermissionInfoBarDelegate::Create(
-        weak_factory_.GetWeakPtr(), infobar_service);
-    infobar_service->AddObserver(this);
+    GroupedPermissionInfoBarDelegate::Create(weak_factory_.GetWeakPtr(),
+                                             infobar_service);
     return;
   }
 
@@ -48,17 +46,7 @@ PermissionPromptAndroid::PermissionPromptAndroid(
   PermissionDialogDelegate::Create(web_contents_, this);
 }
 
-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_);
-  }
-}
+PermissionPromptAndroid::~PermissionPromptAndroid() {}
 
 void PermissionPromptAndroid::UpdateAnchorPosition() {
   NOTREACHED() << "UpdateAnchorPosition is not implemented";
@@ -143,17 +131,6 @@ 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,18 +13,13 @@
 #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,
-                                public infobars::InfoBarManager::Observer {
+class PermissionPromptAndroid : public PermissionPrompt {
  public:
   PermissionPromptAndroid(content::WebContents* web_contents,
                           Delegate* delegate);
@@ -47,9 +42,6 @@ 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
@@ -63,10 +55,6 @@ 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", ContentSettingsType::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

@@ -138,12 +138,6 @@ 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();
 }
@@ -557,21 +551,17 @@ void PermissionRequestManager::RemoveObserver(Observer* observer) {
 }
 
 bool PermissionRequestManager::ShouldShowQuietPermissionPrompt() {
-  if (!requests_.size() ||
-      requests_.front()->GetPermissionRequestType() !=
-          PermissionRequestType::PERMISSION_NOTIFICATIONS) {
+  if (!requests_.size())
     return false;
-  }
 
-  const auto ui_flavor = QuietNotificationsPromptConfig::UIFlavorToUse();
 #if !defined(OS_ANDROID)
-  return ui_flavor == QuietNotificationsPromptConfig::STATIC_ICON ||
-         ui_flavor == QuietNotificationsPromptConfig::ANIMATED_ICON;
+  const auto ui_flavor = QuietNotificationsPromptConfig::UIFlavorToUse();
+  return (requests_.front()->GetPermissionRequestType() ==
+              PermissionRequestType::PERMISSION_NOTIFICATIONS &&
+          (ui_flavor == QuietNotificationsPromptConfig::STATIC_ICON ||
+           ui_flavor == QuietNotificationsPromptConfig::ANIMATED_ICON));
 #else   // OS_ANDROID
-  return ui_flavor == QuietNotificationsPromptConfig::QUIET_NOTIFICATION ||
-         ui_flavor == QuietNotificationsPromptConfig::HEADS_UP_NOTIFICATION ||
-         ui_flavor == QuietNotificationsPromptConfig::MINI_INFOBAR;
-
+  return false;
 #endif  // OS_ANDROID
 }
 

chrome/browser/permissions/permission_request_manager_browsertest.cc

@@ -723,9 +723,10 @@ class PermissionRequestManagerBrowserTest_AnimatedIcon
   }
 };
 
+// Re-enable when 1016233 is fixed.
 // Quiet permission requests are cancelled when a new request is made.
 IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest_StaticIcon,
-                       QuietPendingRequestsKilledOnNewRequest) {
+                       DISABLED_QuietPendingRequestsKilledOnNewRequest) {
   // First add a quiet permission request. Ensure that this request is decided
   // by the end of this test.
   MockPermissionRequest request_quiet(
@@ -752,9 +753,10 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest_StaticIcon,
   EXPECT_EQ(0u, GetPermissionRequestManager()->Requests().size());
 }
 
+// Re-enable when 1016233 is fixed.
 // Quiet permission requests are cancelled when a new request is made.
 IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest_AnimatedIcon,
-                       QuietPendingRequestsKilledOnNewRequest) {
+                       DISABLED_QuietPendingRequestsKilledOnNewRequest) {
   // First add a quiet permission request. Ensure that this request is decided
   // by the end of this test.
   MockPermissionRequest request_quiet(

chrome/test/BUILD.gn

@@ -3637,7 +3637,6 @@ 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",