Skip to content

Commit

Permalink
Avoid FocusSearch crash in case of focus cycles
Browse files Browse the repository at this point in the history 
When a sequence of Views defined by
View::Get{Next,Previous}FocusableView() forms a cycle, FocusSearch can
crash due to infinite recursion if none of the Views are focusable
candidates. Although View focus cycles are prohibited, we should
handle this gracefully to avoid a crash. This change does this by
tracking which Views have been seen in
FocusSearch::Find{Next,Previous}FocusableViewImpl().

Bug: 924327
Change-Id: Ibdbb0fbceed585fdf689672cc47a08bfeff58294
Reviewed-on: https://chromium-review.googlesource.com/c/1453066
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#629362}(cherry picked from commit c213e8c)
Reviewed-on: https://chromium-review.googlesource.com/c/1461317
Reviewed-by: Collin Baker <collinbaker@chromium.org>
Cr-Commit-Position: refs/branch-heads/3683@{#317}
Cr-Branched-From: e510299-refs/heads/master@{#625896}
  • Loading branch information
@chbaker0
chbaker0 committed Feb 8, 2019
1 parent 3de8b98 commit 7063804
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 17 deletions.
34 changes: 34 additions & 0 deletions ui/views/focus/focus_manager_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1153,4 +1153,38 @@ TEST_F(DesktopWidgetFocusManagerTest, AnchoredDialogInDesktopNativeWidgetAura) {
}
#endif // defined(USE_AURA)

// Ensures graceful failure if there is a focus cycle.
TEST_F(FocusManagerTest, HandlesFocusCycles) {
// Create two side-by-side views.
View* root_view = GetWidget()->GetRootView();
View* left = root_view->AddChildView(std::make_unique<View>());
View* right = root_view->AddChildView(std::make_unique<View>());

// Create a cycle where the left view is focusable and the right isn't.
left->SetFocusBehavior(View::FocusBehavior::ALWAYS);
right->SetFocusBehavior(View::FocusBehavior::NEVER);
left->SetNextFocusableView(right);
right->SetNextFocusableView(left);

// Set focus on the left view then make it unfocusable, which both advances
// focus and ensures there's no candidate for focusing.
left->RequestFocus();
EXPECT_TRUE(left->HasFocus());
left->SetFocusBehavior(View::FocusBehavior::NEVER);

// At this point, we didn't crash. Just as a sanity check, ensure neither of
// our views were incorrectly focused.
EXPECT_FALSE(left->HasFocus());
EXPECT_FALSE(right->HasFocus());

// Now test focusing in reverse.
GetFocusManager()->SetFocusedView(right);
EXPECT_TRUE(right->HasFocus());
GetFocusManager()->AdvanceFocus(true);

// We don't check whether |right| has focus since if no focusable view is
// found, AdvanceFocus() doesn't clear focus.
EXPECT_FALSE(left->HasFocus());
}

} // namespace views
53 changes: 36 additions & 17 deletions ui/views/focus/focus_search.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,23 @@ View* FocusSearch::FindNextFocusableView(
DCHECK(Contains(root_, starting_view));
}

base::flat_set<View*> seen_views;
View* v = nullptr;
if (search_direction == SearchDirection::kForwards) {
v = FindNextFocusableViewImpl(
starting_view, check_starting_view, true,
(traversal_direction == TraversalDirection::kDown),
can_go_into_anchored_dialog, starting_view_group, focus_traversable,
focus_traversable_view);
can_go_into_anchored_dialog, starting_view_group, &seen_views,
focus_traversable, focus_traversable_view);
} else {
// If the starting view is focusable, we don't want to go down, as we are
// traversing the view hierarchy tree bottom-up.
bool can_go_down = (traversal_direction == TraversalDirection::kDown) &&
!IsFocusable(starting_view);
v = FindPreviousFocusableViewImpl(starting_view, check_starting_view, true,
can_go_down, can_go_into_anchored_dialog,
starting_view_group, focus_traversable,
focus_traversable_view);
v = FindPreviousFocusableViewImpl(
starting_view, check_starting_view, true, can_go_down,
can_go_into_anchored_dialog, starting_view_group, &seen_views,
focus_traversable, focus_traversable_view);
}

// Don't set the focus to something outside of this view hierarchy.
Expand Down Expand Up @@ -165,8 +166,17 @@ View* FocusSearch::FindNextFocusableViewImpl(
bool can_go_down,
AnchoredDialogPolicy can_go_into_anchored_dialog,
int skip_group_id,
base::flat_set<View*>* seen_views,
FocusTraversable** focus_traversable,
View** focus_traversable_view) {
// Views are not supposed to have focus cycles, but just in case, fail
// gracefully to avoid a crash.
if (seen_views->contains(starting_view)) {
LOG(ERROR) << "View focus cycle detected.";
return nullptr;
}
seen_views->insert(starting_view);

if (check_starting_view == StartingViewPolicy::kCheckStartingView) {
if (IsViewFocusableCandidate(starting_view, skip_group_id)) {
View* v = FindSelectedViewForGroup(starting_view);
Expand All @@ -188,7 +198,7 @@ View* FocusSearch::FindNextFocusableViewImpl(
if (starting_view->has_children()) {
View* v = FindNextFocusableViewImpl(
starting_view->child_at(0), StartingViewPolicy::kCheckStartingView,
false, true, can_go_into_anchored_dialog, skip_group_id,
false, true, can_go_into_anchored_dialog, skip_group_id, seen_views,
focus_traversable, focus_traversable_view);
if (v || *focus_traversable)
return v;
Expand All @@ -212,8 +222,8 @@ View* FocusSearch::FindNextFocusableViewImpl(
if (sibling) {
View* v = FindNextFocusableViewImpl(
sibling, FocusSearch::StartingViewPolicy::kCheckStartingView, false,
true, can_go_into_anchored_dialog, skip_group_id, focus_traversable,
focus_traversable_view);
true, can_go_into_anchored_dialog, skip_group_id, seen_views,
focus_traversable, focus_traversable_view);
if (v || *focus_traversable)
return v;
}
Expand All @@ -237,8 +247,8 @@ View* FocusSearch::FindNextFocusableViewImpl(
if (sibling) {
return FindNextFocusableViewImpl(
sibling, StartingViewPolicy::kCheckStartingView, true, true,
can_go_into_anchored_dialog, skip_group_id, focus_traversable,
focus_traversable_view);
can_go_into_anchored_dialog, skip_group_id, seen_views,
focus_traversable, focus_traversable_view);
}
parent = GetParent(parent);
}
Expand All @@ -261,8 +271,17 @@ View* FocusSearch::FindPreviousFocusableViewImpl(
bool can_go_down,
FocusSearch::AnchoredDialogPolicy can_go_into_anchored_dialog,
int skip_group_id,
base::flat_set<View*>* seen_views,
FocusTraversable** focus_traversable,
View** focus_traversable_view) {
// Views are not supposed to have focus cycles, but just in case, fail
// gracefully to avoid a crash.
if (seen_views->contains(starting_view)) {
LOG(ERROR) << "View focus cycle detected.";
return nullptr;
}
seen_views->insert(starting_view);

// Normally when we navigate to a FocusTraversableParent, can_go_down is
// false so we don't navigate back in. However, if we just navigated out
// of an anchored dialog, allow going down in order to navigate into
Expand Down Expand Up @@ -303,8 +322,8 @@ View* FocusSearch::FindPreviousFocusableViewImpl(
starting_view->child_at(starting_view->child_count() - 1);
View* v = FindPreviousFocusableViewImpl(
view, StartingViewPolicy::kCheckStartingView, false, true,
can_go_into_anchored_dialog, skip_group_id, focus_traversable,
focus_traversable_view);
can_go_into_anchored_dialog, skip_group_id, seen_views,
focus_traversable, focus_traversable_view);
if (v || *focus_traversable)
return v;
}
Expand All @@ -326,8 +345,8 @@ View* FocusSearch::FindPreviousFocusableViewImpl(
if (sibling) {
return FindPreviousFocusableViewImpl(
sibling, StartingViewPolicy::kCheckStartingView, can_go_up, true,
can_go_into_anchored_dialog, skip_group_id, focus_traversable,
focus_traversable_view);
can_go_into_anchored_dialog, skip_group_id, seen_views,
focus_traversable, focus_traversable_view);
}

// Then go up the parent.
Expand All @@ -336,8 +355,8 @@ View* FocusSearch::FindPreviousFocusableViewImpl(
if (parent)
return FindPreviousFocusableViewImpl(
parent, StartingViewPolicy::kCheckStartingView, true, false,
can_go_into_anchored_dialog, skip_group_id, focus_traversable,
focus_traversable_view);
can_go_into_anchored_dialog, skip_group_id, seen_views,
focus_traversable, focus_traversable_view);
}

// We found nothing.
Expand Down
3 changes: 3 additions & 0 deletions ui/views/focus/focus_search.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef UI_VIEWS_FOCUS_FOCUS_SEARCH_H_
#define UI_VIEWS_FOCUS_FOCUS_SEARCH_H_

#include "base/containers/flat_set.h"
#include "base/macros.h"
#include "ui/views/view.h"

Expand Down Expand Up @@ -129,6 +130,7 @@ class VIEWS_EXPORT FocusSearch {
bool can_go_down,
AnchoredDialogPolicy can_go_into_anchored_dialog,
int skip_group_id,
base::flat_set<View*>* seen_views,
FocusTraversable** focus_traversable,
View** focus_traversable_view);

Expand All @@ -140,6 +142,7 @@ class VIEWS_EXPORT FocusSearch {
bool can_go_down,
AnchoredDialogPolicy can_go_into_anchored_dialog,
int skip_group_id,
base::flat_set<View*>* seen_views,
FocusTraversable** focus_traversable,
View** focus_traversable_view);

Expand Down

0 comments on commit 7063804

Please sign in to comment.