Skip to content
Permalink
Browse files

[Merge to M-78] Actual fix for the crash in ~WindowPreviewView()

The crash used to happen when a transient popup child is
added to a window while alt-tab window cycling is active,
and cycling stops before that transient is removed.

This CL fixes the crash and adds a test for it.

TBR=jamescook@chromium.org
BUG=1014543
TEST=Added a new test that crashes without the fix.

(cherry picked from commit 97d1068)

Change-Id: Ib3abf9aee9332859ea00e6a54d3f8fc93d00449f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1885496
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#710188}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1887651
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/branch-heads/3904@{#824}
Cr-Branched-From: 675968a-refs/heads/master@{#693954}
  • Loading branch information
Ahmed Fakhry Ahmed Fakhry
Ahmed Fakhry authored and Ahmed Fakhry committed Oct 29, 2019
1 parent b83df29 commit 0cfec22195bc12fb99a5cd47bdadc1206a142df9
Showing with 45 additions and 21 deletions.
  1. +5 −2 ash/wm/window_preview_view.cc
  2. +40 −19 ash/wm/window_preview_view_unittest.cc
@@ -121,15 +121,18 @@ void WindowPreviewView::OnWindowParentChanged(aura::Window* window,

DCHECK(parent);
unparented_transient_children_.erase(window);
window->RemoveObserver(this);
AddWindow(window);
}

void WindowPreviewView::AddWindow(aura::Window* window) {
DCHECK(!mirror_views_.contains(window));
DCHECK(!unparented_transient_children_.contains(window));
DCHECK(!window->HasObserver(this));

if (window->type() == aura::client::WINDOW_TYPE_POPUP)
return;

DCHECK(!mirror_views_.contains(window));

if (!window->HasObserver(this))
window->AddObserver(this);

@@ -16,6 +16,23 @@ namespace {

using WindowPreviewViewTest = AshTestBase;

// Creates and returns a widget whose type is the given |type|, which is added
// as a transient child of the given |parent_widget|.
std::unique_ptr<views::Widget> CreateTransientChild(
views::Widget* parent_widget,
views::Widget::InitParams::Type type) {
auto widget = std::make_unique<views::Widget>();
views::Widget::InitParams params{type};
params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
params.bounds = gfx::Rect{40, 50};
params.context = params.parent = parent_widget->GetNativeWindow();
params.init_properties_container.SetProperty(
aura::client::kAppType, static_cast<int>(ash::AppType::ARC_APP));
widget->Init(std::move(params));
widget->Show();
return widget;
}

// Test that if we have two widgets whos windows are linked together by
// transience, WindowPreviewView's internal collection will contain both those
// two windows.
@@ -63,22 +80,7 @@ TEST_F(WindowPreviewViewTest, TransientChildAddedAndRemoved) {
TEST_F(WindowPreviewViewTest, NoCrashWithTransientChildWithNoWindowState) {
auto widget1 = CreateTestWidget();

auto create_transient_child = [](views::Widget* parent_widget,
views::Widget::InitParams::Type type)
-> std::unique_ptr<views::Widget> {
auto widget = std::make_unique<views::Widget>();
views::Widget::InitParams params{type};
params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
params.bounds = gfx::Rect{40, 50};
params.context = params.parent = parent_widget->GetNativeWindow();
params.init_properties_container.SetProperty(
aura::client::kAppType, static_cast<int>(ash::AppType::ARC_APP));
widget->Init(std::move(params));
widget->Show();
return widget;
};

auto transient_child1 = create_transient_child(
auto transient_child1 = CreateTransientChild(
widget1.get(), views::Widget::InitParams::TYPE_WINDOW);

EXPECT_EQ(widget1->GetNativeWindow(),
@@ -93,11 +95,11 @@ TEST_F(WindowPreviewViewTest, NoCrashWithTransientChildWithNoWindowState) {
// native window type WINDOW_TYPE_POPUP), while the TYPE_WINDOW one will be
// added as a transient child before it is parented to a container. This
// should not cause a crash.
auto transient_child2 = create_transient_child(
auto transient_child2 = CreateTransientChild(
widget1.get(), views::Widget::InitParams::TYPE_POPUP);
auto transient_child3 = create_transient_child(
auto transient_child3 = CreateTransientChild(
widget1.get(), views::Widget::InitParams::TYPE_BUBBLE);
auto transient_child4 = create_transient_child(
auto transient_child4 = CreateTransientChild(
widget1.get(), views::Widget::InitParams::TYPE_WINDOW);

EXPECT_EQ(widget1->GetNativeWindow(),
@@ -114,6 +116,25 @@ TEST_F(WindowPreviewViewTest, NoCrashWithTransientChildWithNoWindowState) {
EXPECT_EQ(2u, test_api.GetMirrorViews().size());
}

// Tests that if cycling stops before a transient popup child is destroyed
// doesn't introduce a crash. https://crbug.com/1014543.
TEST_F(WindowPreviewViewTest,
NoCrashWhenWindowCyclingIsCanceledWithATransientPopup) {
auto widget1 = CreateTestWidget();

auto preview_view = std::make_unique<WindowPreviewView>(
widget1->GetNativeWindow(), /*trilinear_filtering_on_init=*/false);
WindowPreviewViewTestApi test_api(preview_view.get());
ASSERT_EQ(1u, test_api.GetMirrorViews().size());

auto transient_popup = CreateTransientChild(
widget1.get(), views::Widget::InitParams::TYPE_POPUP);
ASSERT_EQ(1u, test_api.GetMirrorViews().size());

// Simulate canceling window cycling now, there should be no crashes.
preview_view.reset();
}

// Test that WindowPreviewView layouts the transient tree correctly when each
// transient child is within the bounds of its transient parent.
TEST_F(WindowPreviewViewTest, LayoutChildWithinParentBounds) {

0 comments on commit 0cfec22

Please sign in to comment.
You can’t perform that action at this time.