Lock down access to LayoutSuperclass<>() and verify protections.

It's not possible to enforce at compile-time that LayoutSuperclass<>() is only called while a layout is being processed, so enforce that at runtime instead, and verify that enforcement with a death test. This also adds nocompile tests that verify the PassKey protections on Layout(). As a result, it should be impossible to improperly Layout() directly from tests, so the presubmits blocking that are removed. Bug: 1521108 Change-Id: I8f7653c7bdcbbebe5e042310028b3f7de691614f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5259737 Reviewed-by: Dominic Battre <[email protected]> Reviewed-by: Allen Bauer <[email protected]> Commit-Queue: Peter Kasting <[email protected]> Cr-Commit-Position: refs/heads/main@{#1255197}
sunlin-link · Feb 1, 2024 · 134e9e5 · 134e9e5
1 parent ae89e68
commit 134e9e5
Show file tree

Hide file tree

Showing 6 changed files with 116 additions and 107 deletions.
diff --git a/PRESUBMIT.py b/PRESUBMIT.py
@@ -2572,35 +2572,6 @@ def CheckForMatch(affected_file, line_num: int, line: str,
                                       '\n'.join(errors)))
     return result
 
-def CheckNoLayoutCallsInTests(input_api, output_api):
-    """Make sure there are no explicit calls to View::Layout() in tests"""
-    warnings = []
-    ban_rule = BanRule(
-        r'/(\.|->)Layout\(\);',
-        (
-        'Direct calls to View::Layout() are not allowed in tests. '
-        'If the view must be laid out here, use RunScheduledLayout(view). It '
-        'is found in //ui/views/test/views_test_utils.h. '
-        'See http://crbug.com/1350521 for more details.',
-        ),
-        False,
-    )
-    file_filter = lambda f: input_api.re.search(
-        r'_(unittest|browsertest|ui_test).*\.(cc|mm)$', f.LocalPath())
-    for f in input_api.AffectedFiles(file_filter = file_filter):
-        for line_num, line in f.ChangedContents():
-            problems = _GetMessageForMatchingType(input_api, f,
-                                                  line_num, line,
-                                                  ban_rule)
-            if problems:
-                warnings.extend(problems)
-    result = []
-    if (warnings):
-        result.append(
-            output_api.PresubmitPromptWarning(
-                'Banned call to View::Layout() in tests.\n\n'.join(warnings)))
-    return result
-
 def _CheckAndroidNoBannedImports(input_api, output_api):
     """Make sure that banned java imports are not used."""
     errors = []

diff --git a/PRESUBMIT_test.py b/PRESUBMIT_test.py
@@ -4987,31 +4987,6 @@ def testTrueNegatives(self):
         self.assertEqual(0, len(errors))
 
 
-class LayoutInTestsTest(unittest.TestCase):
-  def testLayoutInTest(self):
-    mock_input = MockInputApi()
-    mock_input.files = [
-        MockFile('path/to/foo_unittest.cc',
-                 ['  foo->Layout();', '  bar.Layout();']),
-    ]
-    errors = PRESUBMIT.CheckNoLayoutCallsInTests(mock_input, MockOutputApi())
-    self.assertNotEqual(0, len(errors))
-
-  def testNoTriggerOnLayoutOverride(self):
-    mock_input = MockInputApi();
-    mock_input.files = [
-        MockFile('path/to/foo_unittest.cc',
-                 ['class TestView: public views::View {',
-                  ' public:',
-                  '  void Layout(); override {',
-                  '    views::View::Layout();',
-                  '    // perform bespoke layout',
-                  '  }',
-                  '};'])
-    ]
-    errors = PRESUBMIT.CheckNoLayoutCallsInTests(mock_input, MockOutputApi())
-    self.assertEqual(0, len(errors))
-
 class AssertNoJsInIosTest(unittest.TestCase):
     def testErrorJs(self):
         input_api = MockInputApi()

diff --git a/ui/views/view.cc b/ui/views/view.cc
@@ -8,6 +8,7 @@
 #include <memory>
 #include <utility>
 
+#include "base/auto_reset.h"
 #include "base/check_op.h"
 #include "base/command_line.h"
 #include "base/containers/adapters.h"
@@ -3419,6 +3420,7 @@ void View::LayoutImmediately(bool collect_trace) {
   if (collect_trace) {
     TRACE_EVENT1("ui", "View::LayoutImmediately", "view class", GetClassName());
   }
+  base::AutoReset allow_layout(&layout_allowed_, true);
   Layout(PassKey());
 }
 

diff --git a/ui/views/view.h b/ui/views/view.h
@@ -15,6 +15,7 @@
 #include <vector>
 
 #include "base/callback_list.h"
+#include "base/check.h"
 #include "base/containers/contains.h"
 #include "base/functional/callback.h"
 #include "base/gtest_prod_util.h"
@@ -1849,6 +1850,7 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
     requires std::derived_from<Super, View> && std::derived_from<This, Super> &&
              (!std::same_as<Super, This>)
   void LayoutSuperclass(This* ptr) {
+    CHECK(layout_allowed_);
     static_cast<Super*>(ptr)->Super::Layout(PassKey());
   }
 
@@ -2329,6 +2331,10 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   // Whether the view needs to be laid out.
   bool needs_layout_ = true;
 
+  // Whether Layout() access is currently legal. This is used to prevent calls
+  // to LayoutSuperclass() outside the implementation of Layout().
+  bool layout_allowed_ = false;
+
   // The View's LayoutManager defines the sizing heuristics applied to child
   // Views. The default is absolute positioning according to bounds_.
   std::unique_ptr<LayoutManager> layout_manager_;

diff --git a/ui/views/view_nocompile.nc b/ui/views/view_nocompile.nc
@@ -7,18 +7,31 @@
 
 #include "ui/views/view.h"
 
+#include <memory>
+
 namespace views {
 
+// `DeprecatedLayoutImmediately()` should be the only way to trigger layout
+// synchronously.
 struct SyncLayout : public View {
+  SyncLayout() : child_(AddChildView(std::make_unique<View>())) {
+    child_->Layout();  // expected-error {{too few arguments to function call}}
+  }
+
   void DoSomething() {
+    Layout({});           // expected-error {{calling a private constructor}}
     LayoutImmediately();  // expected-error {{'LayoutImmediately' is a private member}}
   }
+
+ private:
+  View* child_;
 };
 
 // `LayoutSuperclass<SuperT>(this)` should be the only way to trigger superclass
 // layout.
 struct SuperclassLayout : public View {
-  void Layout(PassKey) override {
+  void Layout(PassKey key) override {
+    View::Layout(key);                         // expected-error {{call to deleted constructor}}
     LayoutSuperclass<SuperclassLayout>(this);  // expected-error {{no matching member function}}
     LayoutSuperclass<SyncLayout>(this);        // expected-error {{no matching member function}}
   }

diff --git a/ui/views/view_unittest.cc b/ui/views/view_unittest.cc
@@ -22,6 +22,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/gtest_util.h"
 #include "base/test/icu_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/time/time.h"
@@ -63,6 +64,7 @@
 #include "ui/views/test/ax_event_counter.h"
 #include "ui/views/test/view_metadata_test_utils.h"
 #include "ui/views/test/views_test_base.h"
+#include "ui/views/test/views_test_utils.h"
 #include "ui/views/test/widget_test.h"
 #include "ui/views/view_class_properties.h"
 #include "ui/views/view_observer.h"
@@ -306,6 +308,62 @@ class TestView : public View {
   base::OnceClosure destruction_callback_;
 };
 
+void TestView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
+  did_change_bounds_ = true;
+  new_bounds_ = bounds();
+}
+
+bool TestView::OnMousePressed(const ui::MouseEvent& event) {
+  last_mouse_event_type_ = event.type();
+  location_.SetPoint(event.x(), event.y());
+  if (delete_on_pressed_) {
+    delete this;
+  }
+  return true;
+}
+
+bool TestView::OnMouseDragged(const ui::MouseEvent& event) {
+  last_mouse_event_type_ = event.type();
+  location_.SetPoint(event.x(), event.y());
+  return true;
+}
+
+void TestView::OnMouseReleased(const ui::MouseEvent& event) {
+  last_mouse_event_type_ = event.type();
+  location_.SetPoint(event.x(), event.y());
+}
+
+void TestView::OnMouseEntered(const ui::MouseEvent& event) {
+  received_mouse_enter_ = true;
+}
+
+void TestView::OnMouseExited(const ui::MouseEvent& event) {
+  received_mouse_exit_ = true;
+}
+
+void TestView::OnPaint(gfx::Canvas* canvas) {
+  did_paint_ = true;
+}
+
+void TestView::OnDidSchedulePaint(const gfx::Rect& rect) {
+  scheduled_paint_rects_.push_back(rect);
+  View::OnDidSchedulePaint(rect);
+}
+
+bool TestView::AcceleratorPressed(const ui::Accelerator& accelerator) {
+  accelerator_count_map_[accelerator]++;
+  return true;
+}
+
+void TestView::OnThemeChanged() {
+  View::OnThemeChanged();
+  native_theme_ = GetNativeTheme();
+}
+
+void TestView::OnAccessibilityEvent(ax::mojom::Event event_type) {
+  last_a11y_event_ = event_type;
+}
+
 BEGIN_METADATA(TestView)
 END_METADATA
 
@@ -413,8 +471,42 @@ TEST_F(ViewTest, SizeToPreferredSizeInducesLayout) {
   EXPECT_TRUE(example_view.did_layout_);
 }
 
-void TestView::OnAccessibilityEvent(ax::mojom::Event event_type) {
-  last_a11y_event_ = event_type;
+namespace {
+
+// A view that provides direct and indirect ways to trigger
+// `LayoutSuperclass<>()`.
+class SuperclassLayoutTestView : public TestView {
+ public:
+  int layout_count() const { return layout_count_; }
+
+  void AttemptSuperclassLayout() {
+    ++layout_count_;
+    LayoutSuperclass<TestView>(this);
+  }
+
+  void Layout(PassKey) override { AttemptSuperclassLayout(); }
+
+ private:
+  int layout_count_ = 0;
+};
+
+}  // namespace
+
+// Verifies that LayoutSuperclass<>() can only be invoked while layout is
+// occurring.
+TEST_F(ViewTest, CannotLayoutSuperclassOutsideLayout) {
+  // Construction should not automatically attempt layout.
+  SuperclassLayoutTestView view;
+  EXPECT_EQ(0, view.layout_count());
+
+  // Triggering layout through the standard method should attempt superclass
+  // layout, which should succeed.
+  view.InvalidateLayout();
+  test::RunScheduledLayout(&view);
+  EXPECT_EQ(1, view.layout_count());
+
+  // Attempting superclass layout outside that flow should checkfail.
+  EXPECT_CHECK_DEATH(view.AttemptSuperclassLayout());
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -943,11 +1035,6 @@ TEST_F(ViewTest, OnBoundsChangedFiresA11yEvent) {
   EXPECT_EQ(v.last_a11y_event_, ax::mojom::Event::kLocationChanged);
 }
 
-void TestView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
-  did_change_bounds_ = true;
-  new_bounds_ = bounds();
-}
-
 TEST_F(ViewTest, OnBoundsChanged) {
   TestView v;
 
@@ -999,33 +1086,6 @@ TEST_F(ViewTest, OnStateChangedFiresA11yEvent) {
 // MouseEvent
 ////////////////////////////////////////////////////////////////////////////////
 
-bool TestView::OnMousePressed(const ui::MouseEvent& event) {
-  last_mouse_event_type_ = event.type();
-  location_.SetPoint(event.x(), event.y());
-  if (delete_on_pressed_)
-    delete this;
-  return true;
-}
-
-bool TestView::OnMouseDragged(const ui::MouseEvent& event) {
-  last_mouse_event_type_ = event.type();
-  location_.SetPoint(event.x(), event.y());
-  return true;
-}
-
-void TestView::OnMouseReleased(const ui::MouseEvent& event) {
-  last_mouse_event_type_ = event.type();
-  location_.SetPoint(event.x(), event.y());
-}
-
-void TestView::OnMouseEntered(const ui::MouseEvent& event) {
-  received_mouse_enter_ = true;
-}
-
-void TestView::OnMouseExited(const ui::MouseEvent& event) {
-  received_mouse_exit_ = true;
-}
-
 TEST_F(ViewTest, MouseEvent) {
   auto view1 = std::make_unique<TestView>();
   view1->SetBoundsRect(gfx::Rect(0, 0, 300, 300));
@@ -1152,10 +1212,6 @@ TEST_F(ViewTest, DetectReturnFormDrag) {
 // Painting
 ////////////////////////////////////////////////////////////////////////////////
 
-void TestView::OnPaint(gfx::Canvas* canvas) {
-  did_paint_ = true;
-}
-
 namespace {
 
 // Helper class to create a Widget with standard parameters that is closed when
@@ -1960,11 +2016,6 @@ TEST_F(ViewTest, PaintLocalBounds) {
   EXPECT_TRUE(v1->canvas_bounds().Contains(v1->GetVisibleBounds()));
 }
 
-void TestView::OnDidSchedulePaint(const gfx::Rect& rect) {
-  scheduled_paint_rects_.push_back(rect);
-  View::OnDidSchedulePaint(rect);
-}
-
 namespace {
 
 gfx::Transform RotationCounterclockwise() {
@@ -2789,10 +2840,6 @@ TEST_F(ViewPaintOptimizationTest, PaintDirtyViewsOnly) {
 ////////////////////////////////////////////////////////////////////////////////
 // Accelerators
 ////////////////////////////////////////////////////////////////////////////////
-bool TestView::AcceleratorPressed(const ui::Accelerator& accelerator) {
-  accelerator_count_map_[accelerator]++;
-  return true;
-}
 
 namespace {
 
@@ -5823,11 +5870,6 @@ TEST_F(ViewTest, FocusableAssertions) {
 // NativeTheme
 ////////////////////////////////////////////////////////////////////////////////
 
-void TestView::OnThemeChanged() {
-  View::OnThemeChanged();
-  native_theme_ = GetNativeTheme();
-}
-
 TEST_F(ViewTest, OnThemeChanged) {
   auto test_view = std::make_unique<TestView>();
   EXPECT_FALSE(test_view->native_theme_);