Defer reload in CookieControls bubble to fix race condition.
PROBLEM
A crash would occur when the user triggered the reloading UI, which
starts a spinner animation and immediately begins a page reload.
CAUSE
The spinner (Throbber) uses a timer with callbacks bound to a WeakPtr.
If Reload is triggered immediately after the user presses the button,
the associated widget/view may be destroyed before the timer stops. This
leads to a use-after-free when the Throbber tries to schedule paint
callbacks on a now-invalidated widget.
FIX
This change serializes these operations, and the `Reload()` call is now
deferred. The delayed task callback first stops the spinner animation
and begins closing the bubble. Only after the UI is safely being torn
down is the page reload initiated. A 200ms delay is used to ensure the
spinner is visible before the reload begins.
Note -- this change ends up making the reloading timeout for the TP UI
bubble obsolete (since we close the bubble after a fixed amount of
time). Once I've verified this fix in canary I'll send a followup to
clean up the timeout metrics.
Bug b:425927824
Change-Id: I5bce0bd81a92abc1222734e5ee87375d0b62dad7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6668278
Reviewed-by: Kevin Graney <kmg@google.com>
Commit-Queue: Michelle Abreo <michelleabreo@google.com>
Cr-Commit-Position: refs/heads/main@{#1478504}
diff --git a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_bubble_view_controller.cc b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_bubble_view_controller.cc
index 8e58576..c0a4420 100644
--- a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_bubble_view_controller.cc
+++ b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_bubble_view_controller.cc
@@ -43,6 +43,10 @@
// Unique identifier within the CookieControlsBubbleView hierarchy.
constexpr int kFaviconID = 1;
+// Delay in milliseconds before triggering a page reload for the tracking
+// protections UI.
+constexpr int kReloadUIDisplayDelay = 200;
+
// Expected URL types for `UrlIdentity::CreateFromUrl()`.
constexpr UrlIdentity::TypeSet kUrlIdentityAllowedTypes = {
UrlIdentity::Type::kDefault, UrlIdentity::Type::kFile,
@@ -90,23 +94,47 @@
if (!web_contents_) {
return;
}
-
- web_contents_->GetController().Reload(content::ReloadType::NORMAL, true);
-
- SwitchToReloadingView();
-}
-
-void CookieControlsBubbleViewController::SwitchToReloadingView() {
if (controller_->ShowActFeatures()) {
bubble_view_->GetContentView()
->SetTrackingProtectionsButtonReloadingState();
+ bubble_view_->GetContentView()->SetSpinnerVisible(true);
+
+ // Delay reload to avoid a crash caused by Throbber callbacks running
+ // after the UI has already been torn down.
+ base::SingleThreadTaskRunner::GetCurrentDefault()->PostDelayedTask(
+ FROM_HERE,
+ base::BindOnce(
+ &CookieControlsBubbleViewController::CloseBubbleAndReloadPage,
+ weak_factory_.GetWeakPtr()),
+ base::Milliseconds(kReloadUIDisplayDelay));
+
} else {
+ web_contents_->GetController().Reload(content::ReloadType::NORMAL, true);
+ SwitchToReloadingView();
+ }
+
+ SetReloadingTimeout();
+}
+
+void CookieControlsBubbleViewController::CloseBubbleAndReloadPage() {
+ // Stop animation and close the bubble before triggering reload.
+ if (bubble_view_ && bubble_view_->GetContentView()) {
+ bubble_view_->GetContentView()->SetSpinnerVisible(false);
+ CloseBubble();
+ }
+
+ web_contents_->GetController().Reload(content::ReloadType::NORMAL, true);
+}
+
+void CookieControlsBubbleViewController::SwitchToReloadingView() {
bubble_view_->SwitchToReloadingView();
bubble_view_->GetReloadingView()->GetViewAccessibility().AnnounceText(
l10n_util::GetStringFUTF16(IDS_COOKIE_CONTROLS_BUBBLE_RELOADING_LABEL,
GetSubjectUrlName(web_contents_.get())));
bubble_view_->GetReloadingView()->RequestFocus();
- }
+}
+
+void CookieControlsBubbleViewController::SetReloadingTimeout() {
// Set a timeout for how long the reloading UI is shown for.
base::SingleThreadTaskRunner::GetCurrentDefault()->PostDelayedTask(
FROM_HERE,
@@ -336,8 +364,8 @@
// TODO(crbug.com/388294499): Add metrics for ACT actions.
controller_->SetStateChangedViaBypass(true);
is_reloading_state_ = true;
- OnUserTriggeredReloadingAction();
controller_->OnTrackingProtectionsChangedForSite();
+ OnUserTriggeredReloadingAction();
bubble_view_->GetContentView()->NotifyAccessibilityEventDeprecated(
ax::mojom::Event::kAlert, true);
}
diff --git a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_bubble_view_controller.h b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_bubble_view_controller.h
index 4b1121a..84793320 100644
--- a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_bubble_view_controller.h
+++ b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_bubble_view_controller.h
@@ -75,6 +75,10 @@
base::Time expiration);
void FillViewForTrackingProtections();
+ void CloseBubbleAndReloadPage();
+
+ void SetReloadingTimeout();
+
void CloseBubble();
[[nodiscard]] std::unique_ptr<views::View> InitReloadingView(
diff --git a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_content_view.cc b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_content_view.cc
index cfb64739..d874351 100644
--- a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_content_view.cc
+++ b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_content_view.cc
@@ -266,12 +266,15 @@
}
void CookieControlsContentView::SetTrackingProtectionsButtonReloadingState() {
- tracking_protections_button_->SetSpinnerVisible(true);
tracking_protections_button_->SetText(l10n_util::GetStringUTF16(
IDS_TRACKING_PROTECTIONS_BUBBLE_RELOADING_SITE_LABEL));
tracking_protections_button_->SetEnabled(false);
}
+void CookieControlsContentView::SetSpinnerVisible(bool visible) {
+ tracking_protections_button_->SetSpinnerVisible(visible);
+}
+
void CookieControlsContentView::UpdateContentLabels(
const std::u16string& title,
const std::u16string& description) {
diff --git a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_content_view.h b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_content_view.h
index c22116a..4bc4dbc 100644
--- a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_content_view.h
+++ b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_content_view.h
@@ -63,6 +63,8 @@
virtual void SetTrackingProtectionsButtonReloadingState();
+ virtual void SetSpinnerVisible(bool visible);
+
base::CallbackListSubscription RegisterToggleButtonPressedCallback(
base::RepeatingCallback<void(bool)> callback);
base::CallbackListSubscription RegisterFeedbackButtonPressedCallback(
diff --git a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_interactive_uitest.cc b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_interactive_uitest.cc
index a7bb29a..01db6b7 100644
--- a/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_interactive_uitest.cc
+++ b/chrome/browser/ui/views/location_bar/cookie_controls/cookie_controls_interactive_uitest.cc
@@ -938,35 +938,3 @@
0);
EXPECT_EQ(user_actions_.GetActionCount(kUMABubbleReloadingTimeout), 0);
}
-
-IN_PROC_BROWSER_TEST_F(
- CookieControlsInteractiveUiTrackingProtectionTest,
- BubbleViewTimesOutWithoutShowingReloadingViewWhenStatusChanged) {
- // Test that opening the bubble and making a change results in the
- // reloading view not showing and the bubble closing after timing out.
- //
- // The page loaded in this test will never finish loading, so the timeout
- // must be configured shorter than the test timeout.
- BlockThirdPartyCookies();
- EnableFpProtection();
- auto* const incognito_browser = CreateIncognitoBrowser(browser()->profile());
- RunTestSequence(InContext(
- incognito_browser->window()->GetElementContext(),
- Steps(InstrumentTab(kWebContentsElementId),
- EnterText(
- kOmniboxElementId,
- base::UTF8ToUTF16(
- "https://" +
- third_party_cookie_page_url(/*slow=*/true).GetContent())),
- Confirm(kOmniboxElementId),
- InAnyContext(WaitForShow(kCookieControlsIconElementId)),
- PressButton(kCookieControlsIconElementId),
- InAnyContext(WaitForShow(CookieControlsBubbleView::kContentView)),
- PressButton(CookieControlsContentView::kTrackingProtectionsButton),
- EnsureNotPresent(CookieControlsBubbleView::kReloadingView),
- WaitForHide(CookieControlsBubbleView::kCookieControlsBubble))));
- EXPECT_EQ(user_actions_.GetActionCount(
- kUMATrackingProtectionsBubbleReloadingTimeout),
- 1);
- EXPECT_EQ(user_actions_.GetActionCount(kUMABubbleReloadingTimeout), 0);
-}
