diff --git a/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc b/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc index c1c0da48439e7e..79a7f7bb5605e4 100644 --- a/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc +++ b/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.cc @@ -27,6 +27,7 @@ void ExclusiveAccessControllerBase::OnTabDeactivated( if (web_contents != exclusive_access_tab()) { return; } + // May reach Widget::SetFullscreen(), which can synchronously destroy `this`. ExitExclusiveAccessIfNecessary(); } @@ -40,7 +41,13 @@ void ExclusiveAccessControllerBase::OnTabClosing(WebContents* web_contents) { return; } + // May reach Widget::SetFullscreen(), which can synchronously destroy `this` + // (see crbug.com/1506535). + auto weak_this = base_weak_factory_.GetWeakPtr(); ExitExclusiveAccessIfNecessary(); + if (!weak_this) { + return; + } // The call to exit exclusive access may result in asynchronous notification // of state change (e.g. fullscreen change on Linux). We don't want to rely diff --git a/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h b/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h index b6b2b31c3a4bb0..bcef78dd0e019d 100644 --- a/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h +++ b/chrome/browser/ui/exclusive_access/exclusive_access_controller_base.h @@ -6,6 +6,7 @@ #define CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_CONTROLLER_BASE_H_ #include "base/memory/raw_ptr.h" +#include "base/memory/weak_ptr.h" #include "chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.h" #include "content/public/browser/web_contents_observer.h" #include "url/origin.h" @@ -85,6 +86,11 @@ class ExclusiveAccessControllerBase { private: const raw_ref controller_; } web_contents_observer_{*this}; + + // ExitExclusiveAccessIfNecessary() may reach Widget::SetFullscreen(), which + // can synchronously destroy the owning ExclusiveAccessManager (and thus + // `this`). See crbug.com/1506535. + base::WeakPtrFactory base_weak_factory_{this}; }; #endif // CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_CONTROLLER_BASE_H_ diff --git a/chrome/browser/ui/exclusive_access/exclusive_access_manager.cc b/chrome/browser/ui/exclusive_access/exclusive_access_manager.cc index 717054bcd3f69e..3f10205b3d9afc 100644 --- a/chrome/browser/ui/exclusive_access/exclusive_access_manager.cc +++ b/chrome/browser/ui/exclusive_access/exclusive_access_manager.cc @@ -133,20 +133,36 @@ url::Origin ExclusiveAccessManager::GetExclusiveAccessBubbleOrigin() const { void ExclusiveAccessManager::OnTabDeactivated(WebContents* web_contents) { + // FullscreenController may reach Widget::SetFullscreen(), which can + // synchronously destroy `this` (see crbug.com/1506535). + auto weak_this = weak_factory_.GetWeakPtr(); for (auto controller : exclusive_access_controllers_) { controller->OnTabDeactivated(web_contents); + if (!weak_this) { + return; + } } } void ExclusiveAccessManager::OnTabDetachedFromView(WebContents* web_contents) { + auto weak_this = weak_factory_.GetWeakPtr(); for (auto controller : exclusive_access_controllers_) { controller->OnTabDetachedFromView(web_contents); + if (!weak_this) { + return; + } } } void ExclusiveAccessManager::OnTabClosing(WebContents* web_contents) { + // FullscreenController may reach Widget::SetFullscreen(), which can + // synchronously destroy `this` (see crbug.com/1506535). + auto weak_this = weak_factory_.GetWeakPtr(); for (auto controller : exclusive_access_controllers_) { controller->OnTabClosing(web_contents); + if (!weak_this) { + return; + } } } @@ -173,8 +189,12 @@ bool ExclusiveAccessManager::HandleUserKeyEvent( esc_key_hold_timer_.IsRunning()) { esc_key_hold_timer_.Stop(); show_exit_bubble_timer_.Stop(); + auto weak_this = weak_factory_.GetWeakPtr(); for (auto controller : exclusive_access_controllers_) { controller->HandleUserReleasedEscapeEarly(); + if (!weak_this) { + return false; + } } } else if (IsUnmodifiedEscKeyDownEvent(event) && !esc_key_hold_timer_.IsRunning()) { @@ -205,10 +225,14 @@ bool ExclusiveAccessManager::HandleUserKeyEvent( } bool handled = false; + auto weak_this = weak_factory_.GetWeakPtr(); for (auto controller : exclusive_access_controllers_) { if (controller->HandleUserPressedEscape()) { handled = true; } + if (!weak_this) { + return handled; + } } return handled; } @@ -218,13 +242,21 @@ void ExclusiveAccessManager::OnUserInput() { } void ExclusiveAccessManager::ExitExclusiveAccess() { + auto weak_this = weak_factory_.GetWeakPtr(); for (auto controller : exclusive_access_controllers_) { controller->ExitExclusiveAccessToPreviousState(); + if (!weak_this) { + return; + } } } void ExclusiveAccessManager::HandleUserHeldEscape() { + auto weak_this = weak_factory_.GetWeakPtr(); for (auto controller : exclusive_access_controllers_) { controller->HandleUserHeldEscape(); + if (!weak_this) { + return; + } } } diff --git a/chrome/browser/ui/exclusive_access/exclusive_access_manager.h b/chrome/browser/ui/exclusive_access/exclusive_access_manager.h index 8cdc94a488bcb1..b6f864e52c0343 100644 --- a/chrome/browser/ui/exclusive_access/exclusive_access_manager.h +++ b/chrome/browser/ui/exclusive_access/exclusive_access_manager.h @@ -6,6 +6,7 @@ #define CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_MANAGER_H_ #include "base/memory/raw_ptr.h" +#include "base/memory/weak_ptr.h" #include "chrome/browser/ui/exclusive_access/exclusive_access_bubble_type.h" #include "chrome/browser/ui/exclusive_access/exclusive_access_permission_manager.h" #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h" @@ -107,6 +108,11 @@ class ExclusiveAccessManager { base::flat_set> exclusive_access_controllers_; ExclusiveAccessPermissionManager permission_manager_; + + // FullscreenController's per-controller dispatch can reach + // Widget::SetFullscreen(), which may synchronously destroy `this` (see + // crbug.com/1506535). The fan-out loops must check this after each call. + base::WeakPtrFactory weak_factory_{this}; }; #endif // CHROME_BROWSER_UI_EXCLUSIVE_ACCESS_EXCLUSIVE_ACCESS_MANAGER_H_ diff --git a/chrome/browser/ui/exclusive_access/fullscreen_controller.cc b/chrome/browser/ui/exclusive_access/fullscreen_controller.cc index ca3d7deded5482..2ace0581a395b1 100644 --- a/chrome/browser/ui/exclusive_access/fullscreen_controller.cc +++ b/chrome/browser/ui/exclusive_access/fullscreen_controller.cc @@ -337,9 +337,18 @@ void FullscreenController::FullscreenTabOpeningPopup( void FullscreenController::OnTabDeactivated( content::WebContents* web_contents) { - base::AutoReset> auto_resetter( - &deactivated_contents_, web_contents); + // OnTabDeactivated() may reach Widget::SetFullscreen(), which can + // synchronously destroy `this` (see crbug.com/1506535). Avoid + // base::AutoReset, which would write to freed `&deactivated_contents_` on + // unwind. + raw_ptr previous = deactivated_contents_; + deactivated_contents_ = web_contents; + auto weak_this = ptr_factory_.GetWeakPtr(); ExclusiveAccessControllerBase::OnTabDeactivated(web_contents); + if (!weak_this) { + return; + } + deactivated_contents_ = previous; } void FullscreenController::OnTabDetachedFromView(WebContents* old_contents) { @@ -642,7 +651,14 @@ void FullscreenController::ExitFullscreenModeInternal() { // keep the state coherent. NotifyTabExclusiveAccessLost(); #endif + // ExitFullscreen() reaches Widget::SetFullscreen(), which may synchronously + // destroy the owning ExclusiveAccessManager (and thus `this`). See + // crbug.com/1506535. + auto weak_this = ptr_factory_.GetWeakPtr(); exclusive_access_manager()->context()->ExitFullscreen(); + if (!weak_this) { + return; + } extension_url_.reset(); exclusive_access_manager()->UpdateBubble(base::NullCallback()); }