From 0a74064e5d5a788994f136de899baba192bddba1 Mon Sep 17 00:00:00 2001 From: Scott Violet Date: Fri, 11 Mar 2016 15:59:43 -0800 Subject: [PATCH] [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 R=pkotwicz@chromium.org Review URL: https://codereview.chromium.org/1768533003 Cr-Commit-Position: refs/heads/master@{#379620} (cherry picked from commit 16f0d343ae7f100783af993694d5f6f2457f59a9) Review URL: https://codereview.chromium.org/1783353007 . Cr-Commit-Position: refs/branch-heads/2661@{#200} Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081} --- ui/views/BUILD.gn | 1 + ui/views/view.cc | 121 ++++++++++++++++++++------------------ ui/views/view.h | 8 ++- ui/views/view_unittest.cc | 93 +++++++++++++++++++++++++++++ ui/views/views.gyp | 1 + 5 files changed, 165 insertions(+), 59 deletions(-) diff --git a/ui/views/BUILD.gn b/ui/views/BUILD.gn index 5c471a5b1aa06..1ab70089935c0 100644 --- 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 index 6f5122faae7ca..7e4f170d79f81 100644 --- 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_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 index 86bed2a0de6f0..39c702a405157 100644 --- 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 index 76b9950f7e24a..7d7000017867a 100644 --- 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 index 8adfc68781c6e..cd81230ee2749 100644 --- 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',