diff --git a/ui/views/focus/focus_manager.cc b/ui/views/focus/focus_manager.cc index 8c154ec47cef9..06521b1f2a39e 100644 --- a/ui/views/focus/focus_manager.cc +++ b/ui/views/focus/focus_manager.cc @@ -388,12 +388,13 @@ void FocusManager::SetFocusedView(View* view) { } void FocusManager::ClearFocus() { - // SetFocusedView(nullptr) is going to clear out the stored view to. We need - // to persist it in this case. - views::View* focused_view = GetStoredFocusView(); + // SetFocusedView(nullptr) is going to clear out the stored view too. We need + // to persist it in this case. Track it across the call below since blur + // callbacks may synchronously destroy the view. + ViewTracker focused_view(GetStoredFocusView()); SetFocusedView(nullptr); ClearNativeFocus(); - SetStoredFocusView(focused_view); + SetStoredFocusView(focused_view.view()); } void FocusManager::AdvanceFocusIfNecessary() { @@ -414,14 +415,15 @@ void FocusManager::AdvanceFocusIfNecessary() { } void FocusManager::StoreFocusedView(bool clear_native_focus) { - View* focused_view = focused_view_; // Don't do anything if no focused view. Storing the view (which is nullptr), // in this case, would clobber the view that was previously saved. if (!focused_view_) { return; } - View* v = focused_view_; + // Blur callbacks triggered by SetFocusedView()/ClearFocus() below may + // synchronously destroy the currently focused view. + ViewTracker focused_view(focused_view_); if (clear_native_focus) { // Temporarily disable notification. ClearFocus() will set the focus to the @@ -433,11 +435,11 @@ void FocusManager::StoreFocusedView(bool clear_native_focus) { ClearFocus(); } else { SetFocusedView(nullptr); - SetStoredFocusView(focused_view); + SetStoredFocusView(focused_view.view()); } - if (v) { - v->SchedulePaint(); // Remove focus border. + if (focused_view.view()) { + focused_view.view()->SchedulePaint(); // Remove focus border. } } diff --git a/ui/views/focus/focus_manager_unittest.cc b/ui/views/focus/focus_manager_unittest.cc index a0af398e50633..95d86fbb2825f 100644 --- a/ui/views/focus/focus_manager_unittest.cc +++ b/ui/views/focus/focus_manager_unittest.cc @@ -1003,6 +1003,46 @@ TEST_F(FocusManagerTest, StoreFocusedView) { GetFocusManager()->ClearFocus(); } +namespace { + +// A view that synchronously removes and deletes itself when it loses focus. +class SelfDeletingOnBlurView : public View { + METADATA_HEADER(SelfDeletingOnBlurView, View) + + public: + SelfDeletingOnBlurView() { SetFocusBehavior(FocusBehavior::ALWAYS); } + + void OnBlur() override { parent()->RemoveChildViewT(this); } +}; + +BEGIN_METADATA(SelfDeletingOnBlurView) +END_METADATA + +} // namespace + +// Exercises StoreFocusedView() when the focused view deletes itself while +// being blurred. The stored focused view should be cleared rather than left +// referencing the deleted view. +TEST_F(FocusManagerTest, StoreFocusedViewWithSelfDeletingBlur) { + View* view = GetContentsView()->AddChildView( + std::make_unique()); + + GetFocusManager()->SetFocusedView(view); + ASSERT_EQ(view, GetFocusManager()->GetFocusedView()); + GetFocusManager()->StoreFocusedView(false); + EXPECT_EQ(nullptr, GetFocusManager()->GetFocusedView()); + EXPECT_EQ(nullptr, GetFocusManager()->GetStoredFocusView()); + + // Repeat with |true|, which routes through ClearFocus(). + view = GetContentsView()->AddChildView( + std::make_unique()); + GetFocusManager()->SetFocusedView(view); + ASSERT_EQ(view, GetFocusManager()->GetFocusedView()); + GetFocusManager()->StoreFocusedView(true); + EXPECT_EQ(nullptr, GetFocusManager()->GetFocusedView()); + EXPECT_EQ(nullptr, GetFocusManager()->GetStoredFocusView()); +} + #if BUILDFLAG(IS_MAC) // Test that the correct view is restored if full keyboard access is changed. TEST_F(FocusManagerTest, StoreFocusedViewFullKeyboardAccess) {