Skip to content

Commit

Permalink
[Merge 116] Fix Chromeguard crash
Browse files Browse the repository at this point in the history
A CHECK is hit when the CookieControlsObserver fires an update event
in close proximity to the close of the CookieControlsBubbleView.

In particular, this can occur when the active tab changes to a tab
which has cookie controls disabled (such as a chrome:// URL). When
this happens, it breaks the expectation that the UI is not shown over such pages, and the CHECK enforcing that expectation is hit.

The UI is not _really_ shown, as it is in the process of closing
(i.e. WindowClosing() has already been called), but an attempt to
update the title is made based on the new CookieControlsObserver
information is made anyway.

This CL ensures that the bubble stops observing the CookieControls
when CloseBubble is called, preventing future updates be handled.

The test introduced in this CL reliably fails without the associated
fix.

(cherry picked from commit 77610c2)

Bug: 1459383
Change-Id: I3bb6fc01c1057ac606c0136ace5a303036e58eb9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4660440
Commit-Queue: Theodore Olsauskas-Warren <sauski@google.com>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Olesia Marukhno <olesiamarukhno@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1164622}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4661099
Cr-Commit-Position: refs/branch-heads/5845@{#283}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
  • Loading branch information
sauski-alternative authored and Chromium LUCI CQ committed Jul 3, 2023
1 parent 41d0c23 commit 08d04e4
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,10 @@ void OldCookieControlsBubbleView::UpdateUi() {
}

void OldCookieControlsBubbleView::CloseBubble() {
// Widget's Close() is async, but we don't want to use cookie_bubble_ after
// this. Additionally web_contents() may have been destroyed.
// Widget's Close() is async, but we don't want to use cookie_bubble_, or
// receive CookieControls updates after this. Additionally web_contents() may
// have been destroyed.
controller_observation_.Reset();
g_instance = nullptr;
LocationBarBubbleDelegateView::CloseBubble();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,31 @@ IN_PROC_BROWSER_TEST_F(CookieControlsBubbleViewTest, BlockingDisabled) {
EXPECT_FALSE(cookie_settings()->IsThirdPartyAccessAllowed(origin, nullptr));
}

IN_PROC_BROWSER_TEST_F(CookieControlsBubbleViewTest, NonAllowedCookieSite) {
// Regression test for crbug.com/1459383. Activating a tab where cookies are
// blocked, such as an internal chrome:// url, while the UI is shown, should
// not crash.
SetThirdPartyCookieBlocking(true);
NavigateToUrlWithThirdPartyCookies();

// Open chrome://about in the background.
ASSERT_TRUE(ui_test_utils::NavigateToURLWithDisposition(
browser(), GURL("chrome://about"),
WindowOpenDisposition::NEW_BACKGROUND_TAB,
ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP));

// Open bubble on the page with 3PC blocked.
ShowUi("NotWorkingClicked");

// While bubble is open, activate the chrome://about tab.
browser()->tab_strip_model()->ActivateTabAt(1);

// The crash would have already occurred, but navigate to another chrome://
// url to be sure.
ASSERT_TRUE(
ui_test_utils::NavigateToURL(browser(), GURL("chrome://version")));
}

// ==================== Pixel tests ====================

// Test opening cookie controls bubble with blocked cookies present.
Expand Down

0 comments on commit 08d04e4

Please sign in to comment.