Commit cd10c5b3 authored by Evan Stade's avatar Evan Stade Committed by Commit Bot

Fix CollectedCookiesView crash.

CollectedCookiesViews::CreateAndShowForWebContents may be called
while an existing dialog (widget) is in the process of closing. In
this case, exit gracefully instead of crashing.

The only way I can think of that might be possible to repro this
IRL (i.e. aside from clusterfuzz) would be to:
a) open page info bubble
b) click "cookies"
c) open page info bubble again
d) click "cookies", then focus cookies dialog, press escape,
   all in rapid succession
e) cookies dialog starts closing
f) click on "cookies" is a posted task, and is handled after
   cookies dialog starts closing, but before finishing closing

This is probably exceedingly rare, enough so that it's not worth
being concerned with re-opening the cookies dialog. (And arguably,
it shouldn't re-open anyway, since the last command the user issued
was "escape" (close).)

Bug: 989888
Change-Id: Ic4e3274128f5530b621d9dfe4e99dd6d24bddc21
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1731914Reviewed-by: default avatarScott Violet <sky@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683396}
parent 1783c67c
...@@ -279,13 +279,23 @@ CollectedCookiesViews::~CollectedCookiesViews() { ...@@ -279,13 +279,23 @@ CollectedCookiesViews::~CollectedCookiesViews() {
void CollectedCookiesViews::CreateAndShowForWebContents( void CollectedCookiesViews::CreateAndShowForWebContents(
content::WebContents* web_contents) { content::WebContents* web_contents) {
CollectedCookiesViews* instance = FromWebContents(web_contents); CollectedCookiesViews* instance = FromWebContents(web_contents);
if (instance) { if (!instance) {
web_modal::WebContentsModalDialogManager::FromWebContents(web_contents) CreateForWebContents(web_contents);
->FocusTopmostDialog();
return; return;
} }
CreateForWebContents(web_contents); // On rare occasions, |instance| may have started, but not finished,
// closing. In this case, the modal dialog manager will have removed the
// dialog from its list of tracked dialogs, and therefore might not have any
// active dialog. This should be rare enough that it's not worth trying to
// re-open the dialog. See https://crbug.com/989888
if (instance->GetWidget()->IsClosed())
return;
auto* dialog_manager =
web_modal::WebContentsModalDialogManager::FromWebContents(web_contents);
CHECK(dialog_manager->IsDialogActive());
dialog_manager->FocusTopmostDialog();
} }
/////////////////////////////////////////////////////////////////////////////// ///////////////////////////////////////////////////////////////////////////////
......
...@@ -41,7 +41,7 @@ class CollectedCookiesViewsTest : public InProcessBrowserTest { ...@@ -41,7 +41,7 @@ class CollectedCookiesViewsTest : public InProcessBrowserTest {
// Closing dialog with modified data will shows infobar. // Closing dialog with modified data will shows infobar.
void SetDialogChanged() { cookies_dialog_->status_changed_ = true; } void SetDialogChanged() { cookies_dialog_->status_changed_ = true; }
void CloseCookiesDialog() { cookies_dialog_->Close(); } void CloseCookiesDialog() { cookies_dialog_->GetWidget()->Close(); }
size_t infobar_count() const { size_t infobar_count() const {
content::WebContents* web_contents = content::WebContents* web_contents =
...@@ -93,3 +93,14 @@ IN_PROC_BROWSER_TEST_F(CollectedCookiesViewsTest, ChangeAndCloseTab) { ...@@ -93,3 +93,14 @@ IN_PROC_BROWSER_TEST_F(CollectedCookiesViewsTest, ChangeAndCloseTab) {
EXPECT_EQ(0u, infobar_count()); EXPECT_EQ(0u, infobar_count());
} }
// Closing the widget asynchronously destroys the CollectedCookiesViews object,
// but synchronously removes it from the WebContentsModalDialogManager. Make
// sure there's no crash when trying to re-open the CollectedCookiesViews right
// after closing it. Regression test for https://crbug.com/989888
IN_PROC_BROWSER_TEST_F(CollectedCookiesViewsTest, CloseDialogAndReopen) {
CloseCookiesDialog();
auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents();
CollectedCookiesViews::CreateAndShowForWebContents(web_contents);
// If the test didn't crash, it has passed.
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment