Fix a crash when a transient child is added to WindowPreviewView

When initializing a new widget such that its native window is
added as a transient child of a window tracked by a WindowPreviewView,
WindowPreviewView gets notified with this addition *before* the window
is parented to a container. If we try to get its WindowState at this
point we would crash at this line: https://cs.chromium.org/chromium/src/ash/wm/window_state.cc?rcl=110515d4d38fd48cb9c6a1d0d3e4f9abf7c415f4&l=820

This CL fixes this issue and adds a test that fails without the fix.

BUG=1003544
TEST=Added a new test

Change-Id: I794d312601eb5b80f1928e86ba0fa42911b8c327
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1826031
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702112}

ash/wm/window_preview_view.cc

@@ -26,12 +26,8 @@ WindowPreviewView::WindowPreviewView(aura::Window* window,
   DCHECK(window);
   aura::client::GetTransientWindowClient()->AddObserver(this);
 
-  for (auto* window : GetTransientTreeIterator(window_)) {
-    if (window->type() == aura::client::WINDOW_TYPE_POPUP)
-      continue;
-
+  for (auto* window : GetTransientTreeIterator(window_))
     AddWindow(window);
-  }
 }
 
 WindowPreviewView::~WindowPreviewView() {
@@ -93,6 +89,12 @@ void WindowPreviewView::OnTransientChildWindowAdded(
   if (!::wm::HasTransientAncestor(parent, root) && parent != root)
     return;
 
+  if (!transient_child->parent()) {
+    transient_child->AddObserver(this);
+    unparented_transient_children_.emplace(transient_child);
+    return;
+  }
+
   AddWindow(transient_child);
 }
 
@@ -110,10 +112,24 @@ void WindowPreviewView::OnWindowDestroying(aura::Window* window) {
   RemoveWindow(window);
 }
 
+void WindowPreviewView::OnWindowParentChanged(aura::Window* window,
+                                              aura::Window* parent) {
+  if (!unparented_transient_children_.contains(window))
+    return;
+
+  DCHECK(parent);
+  unparented_transient_children_.erase(window);
+  AddWindow(window);
+}
+
 void WindowPreviewView::AddWindow(aura::Window* window) {
+  if (window->type() == aura::client::WINDOW_TYPE_POPUP)
+    return;
+
   DCHECK(!mirror_views_.contains(window));
 
-  window->AddObserver(this);
+  if (!window->HasObserver(this))
+    window->AddObserver(this);
 
   auto* mirror_view =
       window_util::IsArcPipWindow(window)

ash/wm/window_preview_view.h

@@ -8,6 +8,7 @@
 #include "ash/ash_export.h"
 #include "ash/wm/window_mirror_view.h"
 #include "base/containers/flat_map.h"
+#include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "ui/aura/client/transient_window_client_observer.h"
 #include "ui/aura/window.h"
@@ -42,6 +43,8 @@ class ASH_EXPORT WindowPreviewView
 
   // aura::WindowObserver:
   void OnWindowDestroying(aura::Window* window) override;
+  void OnWindowParentChanged(aura::Window* window,
+                             aura::Window* parent) override;
 
   aura::Window* window() { return window_; }
 
@@ -60,6 +63,12 @@ class ASH_EXPORT WindowPreviewView
 
   base::flat_map<aura::Window*, WindowMirrorView*> mirror_views_;
 
+  // Transient children of |window_| may be added as transients before they're
+  // actually parented; i.e. `OnTransientChildWindowAdded()` is called before
+  // `transient_child->parent()` is set. We track those here so that we can add
+  // them to the view once they're parented.
+  base::flat_set<aura::Window*> unparented_transient_children_;
+
   DISALLOW_COPY_AND_ASSIGN(WindowPreviewView);
 };
 

ash/wm/window_preview_view_unittest.cc

@@ -4,8 +4,10 @@
 
 #include "ash/wm/window_preview_view.h"
 
+#include "ash/public/cpp/app_types.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/wm/window_preview_view_test_api.h"
+#include "ui/aura/client/aura_constants.h"
 #include "ui/views/widget/widget.h"
 #include "ui/wm/core/window_util.h"
 
@@ -54,6 +56,64 @@ TEST_F(WindowPreviewViewTest, TransientChildAddedAndRemoved) {
   EXPECT_EQ(2u, test_api.GetMirrorViews().size());
 }
 
+// Tests that init'ing a Widget with a native window as a transient child before
+// it is parented to a parent window doesn't cause a crash while the
+// WindowPreviewView is observing transient windows additions.
+// https://crbug.com/1003544.
+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(
+      widget1.get(), views::Widget::InitParams::TYPE_WINDOW);
+
+  EXPECT_EQ(widget1->GetNativeWindow(),
+            wm::GetTransientParent(transient_child1->GetNativeWindow()));
+
+  auto preview_view = std::make_unique<WindowPreviewView>(
+      widget1->GetNativeWindow(), /*trilinear_filtering_on_init=*/false);
+  WindowPreviewViewTestApi test_api(preview_view.get());
+  ASSERT_EQ(2u, test_api.GetMirrorViews().size());
+
+  // The popup and bubble transient child should be ignored (they result in a
+  // 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(
+      widget1.get(), views::Widget::InitParams::TYPE_POPUP);
+  auto transient_child3 = create_transient_child(
+      widget1.get(), views::Widget::InitParams::TYPE_BUBBLE);
+  auto transient_child4 = create_transient_child(
+      widget1.get(), views::Widget::InitParams::TYPE_WINDOW);
+
+  EXPECT_EQ(widget1->GetNativeWindow(),
+            wm::GetTransientParent(transient_child2->GetNativeWindow()));
+  EXPECT_EQ(widget1->GetNativeWindow(),
+            wm::GetTransientParent(transient_child3->GetNativeWindow()));
+  EXPECT_EQ(widget1->GetNativeWindow(),
+            wm::GetTransientParent(transient_child4->GetNativeWindow()));
+  EXPECT_EQ(3u, test_api.GetMirrorViews().size());
+
+  transient_child3.reset();
+  EXPECT_EQ(3u, test_api.GetMirrorViews().size());
+  transient_child4.reset();
+  EXPECT_EQ(2u, test_api.GetMirrorViews().size());
+}
+
 // Test that WindowPreviewView layouts the transient tree correctly when each
 // transient child is within the bounds of its transient parent.
 TEST_F(WindowPreviewViewTest, LayoutChildWithinParentBounds) {

ash/wm/window_util.cc

@@ -306,7 +306,7 @@ bool IsArcWindow(const aura::Window* window) {
 }
 
 bool IsArcPipWindow(const aura::Window* window) {
-  return WindowState::Get(window)->IsPip() && window_util::IsArcWindow(window);
+  return IsArcWindow(window) && WindowState::Get(window)->IsPip();
 }
 
 }  // namespace window_util