[MERGE to 2661] Fixes bug in making layer tree match view tree

Prior to this patch when adding a view we could call into OnNativeThemeChanged() before ensuring the layer tree and view tree are in sync. This is problematic as its entirely possible for client code to trigger trying to resync the layer tree and view tree, and since we assume the two are already in sync we get crashes. As part of this I'm moving syncing the layer trees out of ViewHierarchyChangedImpl() into the call sites so we can ensure the layer tree and view tree are in sync before calling any client code. BUG=590696 TEST=covered by test [email protected] Review URL: https://codereview.chromium.org/1768533003 Cr-Commit-Position: refs/heads/master@{#379620} (cherry picked from commit 16f0d34) Review URL: https://codereview.chromium.org/1783353007 . Cr-Commit-Position: refs/branch-heads/2661@{crosswalk-project#200} Cr-Branched-From: ef6f6ae-refs/heads/master@{#378081}
huningxin · Mar 12, 2016 · 0a74064 · 0a74064
1 parent c1fd03e
commit 0a74064
Show file tree

Hide file tree

Showing 5 changed files with 165 additions and 59 deletions.
diff --git a/ui/views/BUILD.gn b/ui/views/BUILD.gn
@@ -226,6 +226,7 @@ test("views_unittests") {
     "//ui/gfx:test_support",
     "//ui/gfx/geometry",
     "//ui/gl:test_support",
+    "//ui/native_theme",
     "//ui/resources",
     "//ui/strings",
     "//url",

diff --git a/ui/views/view.cc b/ui/views/view.cc
@@ -178,11 +178,25 @@ void View::AddChildViewAt(View* view, int index) {
   // Sets the prev/next focus views.
   InitFocusSiblings(view, index);
 
-  // Let's insert the view.
   view->parent_ = this;
   children_.insert(children_.begin() + index, view);
 
-  views::Widget* widget = GetWidget();
+  // Ensure the layer tree matches the view tree before calling to any client
+  // code. This way if client code further modifies the view tree we are in a
+  // sane state.
+  const bool did_reparent_any_layers = view->UpdateParentLayers();
+  Widget* widget = GetWidget();
+  if (did_reparent_any_layers && widget)
+    widget->UpdateRootLayers();
+
+  ReorderLayers();
+
+  // Make sure the visibility of the child layers are correct.
+  // If any of the parent View is hidden, then the layers of the subtree
+  // rooted at |this| should be hidden. Otherwise, all the child layers should
+  // inherit the visibility of the owner View.
+  view->UpdateLayerVisibility();
+
   if (widget) {
     const ui::NativeTheme* new_theme = view->GetNativeTheme();
     if (new_theme != old_theme)
@@ -195,23 +209,18 @@ void View::AddChildViewAt(View* view, int index) {
     v->ViewHierarchyChangedImpl(false, details);
 
   view->PropagateAddNotifications(details);
+
   UpdateTooltip();
+
   if (widget) {
     RegisterChildrenForVisibleBoundsNotification(view);
+
     if (view->visible())
       view->SchedulePaint();
   }
 
   if (layout_manager_.get())
     layout_manager_->ViewAdded(this, view);
-
-  ReorderLayers();
-
-  // Make sure the visibility of the child layers are correct.
-  // If any of the parent View is hidden, then the layers of the subtree
-  // rooted at |this| should be hidden. Otherwise, all the child layers should
-  // inherit the visibility of the owner View.
-  UpdateLayerVisibility();
 }
 
 void View::ReorderChildView(View* view, int index) {
@@ -1806,42 +1815,47 @@ void View::DoRemoveChildView(View* view,
   DCHECK(view);
 
   const Views::iterator i(std::find(children_.begin(), children_.end(), view));
+  if (i == children_.end())
+    return;
+
   scoped_ptr<View> view_to_be_deleted;
-  if (i != children_.end()) {
-    if (update_focus_cycle) {
-      // Let's remove the view from the focus traversal.
-      View* next_focusable = view->next_focusable_view_;
-      View* prev_focusable = view->previous_focusable_view_;
-      if (prev_focusable)
-        prev_focusable->next_focusable_view_ = next_focusable;
-      if (next_focusable)
-        next_focusable->previous_focusable_view_ = prev_focusable;
-    }
+  if (update_focus_cycle) {
+    View* next_focusable = view->next_focusable_view_;
+    View* prev_focusable = view->previous_focusable_view_;
+    if (prev_focusable)
+      prev_focusable->next_focusable_view_ = next_focusable;
+    if (next_focusable)
+      next_focusable->previous_focusable_view_ = prev_focusable;
+  }
 
-    Widget* widget = GetWidget();
-    if (widget) {
-      UnregisterChildrenForVisibleBoundsNotification(view);
-      if (view->visible())
-        view->SchedulePaint();
+  Widget* widget = GetWidget();
+  if (widget) {
+    UnregisterChildrenForVisibleBoundsNotification(view);
+    if (view->visible())
+      view->SchedulePaint();
 
-      if (!new_parent || new_parent->GetWidget() != widget)
-        widget->NotifyWillRemoveView(view);
-    }
+    if (!new_parent || new_parent->GetWidget() != widget)
+      widget->NotifyWillRemoveView(view);
+  }
+
+  // Make sure the layers belonging to the subtree rooted at |view| get
+  // removed.
+  view->OrphanLayers();
+  if (widget)
+    widget->UpdateRootLayers();
 
-    view->PropagateRemoveNotifications(this, new_parent);
-    view->parent_ = NULL;
-    view->UpdateLayerVisibility();
+  view->PropagateRemoveNotifications(this, new_parent);
+  view->parent_ = nullptr;
 
-    if (delete_removed_view && !view->owned_by_client_)
-      view_to_be_deleted.reset(view);
+  if (delete_removed_view && !view->owned_by_client_)
+    view_to_be_deleted.reset(view);
 
-    children_.erase(i);
-  }
+  children_.erase(i);
 
   if (update_tool_tip)
     UpdateTooltip();
 
-  if (layout_manager_.get())
+  if (layout_manager_)
     layout_manager_->ViewRemoved(this, view);
 }
 
@@ -1882,20 +1896,6 @@ void View::ViewHierarchyChangedImpl(
     }
   }
 
-  if (details.is_add && layer() && !layer()->parent()) {
-    UpdateParentLayer();
-    Widget* widget = GetWidget();
-    if (widget)
-      widget->UpdateRootLayers();
-  } else if (!details.is_add && details.child == this) {
-    // Make sure the layers belonging to the subtree rooted at |child| get
-    // removed from layers that do not belong in the same subtree.
-    OrphanLayers();
-    Widget* widget = GetWidget();
-    if (widget)
-      widget->UpdateRootLayers();
-  }
-
   ViewHierarchyChanged(details);
   details.parent->needs_layout_ = true;
 }
@@ -2120,14 +2120,23 @@ void View::CreateLayer() {
   SchedulePaintOnParent();
 }
 
-void View::UpdateParentLayers() {
+bool View::UpdateParentLayers() {
   // Attach all top-level un-parented layers.
-  if (layer() && !layer()->parent()) {
-    UpdateParentLayer();
-  } else {
-    for (int i = 0, count = child_count(); i < count; ++i)
-      child_at(i)->UpdateParentLayers();
+  if (layer()) {
+    if (!layer()->parent()) {
+      UpdateParentLayer();
+      return true;
+    }
+    // The layers of any child views are already in place, so we can stop
+    // iterating here.
+    return false;
   }
+  bool result = false;
+  for (int i = 0, count = child_count(); i < count; ++i) {
+    if (child_at(i)->UpdateParentLayers())
+      result = true;
+  }
+  return result;
 }
 
 void View::OrphanLayers() {

diff --git a/ui/views/view.h b/ui/views/view.h
@@ -1348,9 +1348,11 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   // Creates the layer and related fields for this view.
   void CreateLayer();
 
-  // Parents all un-parented layers within this view's hierarchy to this view's
-  // layer.
-  void UpdateParentLayers();
+  // Recursively calls UpdateParentLayers() on all descendants, stopping at any
+  // Views that have layers. Calls UpdateParentLayer() for any Views that have
+  // a layer with no parent. If at least one descendant had an unparented layer
+  // true is returned.
+  bool UpdateParentLayers();
 
   // Parents this view's layer to |parent_layer|, and sets its bounds and other
   // properties in accordance to |offset|, the view's offset from the

diff --git a/ui/views/view_unittest.cc b/ui/views/view_unittest.cc
@@ -31,6 +31,7 @@
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/path.h"
 #include "ui/gfx/transform.h"
+#include "ui/native_theme/native_theme.h"
 #include "ui/strings/grit/ui_strings.h"
 #include "ui/views/background.h"
 #include "ui/views/controls/native/native_view_host.h"
@@ -4473,4 +4474,96 @@ TEST_F(ViewTest, ScopedTargetHandlerReceivesEvents) {
   EXPECT_EQ(ui::ET_MOUSE_RELEASED, v->last_mouse_event_type_);
 }
 
+// See comment above test for details.
+class WidgetWithCustomTheme : public Widget {
+ public:
+  explicit WidgetWithCustomTheme(ui::NativeTheme* theme) : theme_(theme) {}
+  ~WidgetWithCustomTheme() override {}
+
+  // Widget:
+  const ui::NativeTheme* GetNativeTheme() const override { return theme_; }
+
+ private:
+  ui::NativeTheme* theme_;
+
+  DISALLOW_COPY_AND_ASSIGN(WidgetWithCustomTheme);
+};
+
+// See comment above test for details.
+class ViewThatAddsViewInOnNativeThemeChanged : public View {
+ public:
+  ViewThatAddsViewInOnNativeThemeChanged() { SetPaintToLayer(true); }
+  ~ViewThatAddsViewInOnNativeThemeChanged() override {}
+
+  bool on_native_theme_changed_called() const {
+    return on_native_theme_changed_called_;
+  }
+
+  // View:
+  void OnNativeThemeChanged(const ui::NativeTheme* theme) override {
+    on_native_theme_changed_called_ = true;
+    GetWidget()->GetRootView()->AddChildView(new View);
+  }
+
+ private:
+  bool on_native_theme_changed_called_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(ViewThatAddsViewInOnNativeThemeChanged);
+};
+
+// See comment above test for details.
+class TestNativeTheme : public ui::NativeTheme {
+ public:
+  TestNativeTheme() {}
+  ~TestNativeTheme() override {}
+
+  // ui::NativeTheme:
+  SkColor GetSystemColor(ColorId color_id) const override {
+    return SK_ColorRED;
+  }
+  gfx::Size GetPartSize(Part part,
+                        State state,
+                        const ExtraParams& extra) const override {
+    return gfx::Size();
+  }
+  void Paint(SkCanvas* canvas,
+             Part part,
+             State state,
+             const gfx::Rect& rect,
+             const ExtraParams& extra) const override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestNativeTheme);
+};
+
+// Creates and adds a new child view to |parent| that has a layer.
+void AddViewWithChildLayer(View* parent) {
+  View* child = new View;
+  child->SetPaintToLayer(true);
+  parent->AddChildView(child);
+}
+
+// This test does the following:
+// . creates a couple of views with layers added to the root.
+// . Add a view that overrides OnNativeThemeChanged(). In
+//   OnNativeThemeChanged() another view is added.
+// This sequence triggered DCHECKs or crashes previously. This tests verifies
+// that doesn't happen. Reason for crash was OnNativeThemeChanged() was called
+// before the layer hierarchy was updated. OnNativeThemeChanged() should be
+// called after the layer hierarchy matches the view hierarchy.
+TEST_F(ViewTest, CrashOnAddFromFromOnNativeThemeChanged) {
+  TestNativeTheme theme;
+  WidgetWithCustomTheme widget(&theme);
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
+  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.bounds = gfx::Rect(50, 50, 350, 350);
+  widget.Init(params);
+
+  AddViewWithChildLayer(widget.GetRootView());
+  ViewThatAddsViewInOnNativeThemeChanged* v =
+      new ViewThatAddsViewInOnNativeThemeChanged;
+  widget.GetRootView()->AddChildView(v);
+  EXPECT_TRUE(v->on_native_theme_changed_called());
+}
+
 }  // namespace views
diff --git a/ui/views/views.gyp b/ui/views/views.gyp
@@ -864,6 +864,7 @@
         '../events/events.gyp:events_test_support',
         '../gfx/gfx.gyp:gfx',
         '../gfx/gfx.gyp:gfx_geometry',
+        '../native_theme/native_theme.gyp:native_theme',
         '../resources/ui_resources.gyp:ui_resources',
         '../resources/ui_resources.gyp:ui_test_pak',
         '../strings/ui_strings.gyp:ui_strings',