Fix crash in FrameView::updateLayersAndCompositingAfterScrollIfNeeded

third_party/WebKit/Source/core/frame/FrameViewTest.cpp

Index: third_party/WebKit/Source/core/frame/FrameViewTest.cpp
diff --git a/third_party/WebKit/Source/core/frame/FrameViewTest.cpp b/third_party/WebKit/Source/core/frame/FrameViewTest.cpp
index a182ea5f845aad0dc09f447fab842d1f2f14a124..62a9abd12ca4b088c37deb706bd0a5639b10e854 100644
--- a/third_party/WebKit/Source/core/frame/FrameViewTest.cpp
+++ b/third_party/WebKit/Source/core/frame/FrameViewTest.cpp
@@ -13,6 +13,7 @@
 #include "core/layout/LayoutObject.h"
 #include "core/loader/EmptyClients.h"
 #include "core/page/Page.h"
+#include "core/paint/PaintLayer.h"
 #include "core/testing/DummyPageHolder.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/geometry/IntSize.h"
@@ -139,6 +140,34 @@ TEST_P(FrameViewTest, NoOverflowInIncrementVisuallyNonEmptyPixelCount) {
   EXPECT_TRUE(document().view()->isVisuallyNonEmpty());
 }
 
+// This test addresses http://crbug.com/696173, in which a call to
+// FrameView::updateLayersAndCompositingAfterScrollIfNeeded during layout caused
+// a crash as the code was incorrectly assuming that the ancestor overflow layer
+// would always be valid.
+TEST_P(FrameViewTest, ViewportConstrainedObjectsHandledCorrectlyDuringLayout) {
+  // When using root layer scrolling there is no concept of viewport constrained
+  // objects, so skip this test.
+  if (RuntimeEnabledFeatures::rootLayerScrollingEnabled())

[flackr, 2017/02/27 16:42:09]

Don't we run into the same problem with updateLayerPositionsAfterOverflowScroll
on non-root layers?

[flackr, 2017/02/27 16:43:14]

Nevermind, on that we update top-down so you didn't have the additional ancestor
overflow checks.

[bokan, 2017/02/28 13:07:44]

I'm not sure I understand why we can't run this with RLS enabled. Sticky will
still have to work in that world (though I'd believe it doesn't yet) so we
should test there too. If RLS doesn't support sticky yet and so this test
*can't* work, note that in the comment so that we know to remove the skip once
RLS is far along enough.

[smcgruer, 2017/02/28 13:13:47]

I guess its black-box testing.
FrameView::updateLayersAndCompositingAfterScrollIfNeeded early-exits
(immediately) if there are no viewport constrained objects, and as far as I
understand it (which may be wrong), RLS implies no viewport constrained objects.

However its still fair to say that the call shouldn't crash, so I have removed
the special-case.

+    return;
+
+  document().body()->setInnerHTML(
+      "<style>.container { height: 200%; }"
+      "#sticky { position: sticky; top: 0; height: 50px; }</style>"
+      "<div class='container'><div id='sticky'></div></div>");
+  document().view()->updateAllLifecyclePhases();
+
+  LayoutBoxModelObject* sticky = toLayoutBoxModelObject(
+      document().getElementById("sticky")->layoutObject());
+
+  // Deliberately invalidate the ancestor overflow layer. This approximates
+  // http://crbug.com/696173, in which the ancestor overflow layer can be null
+  // during layout.
+  sticky->layer()->updateAncestorOverflowLayer(nullptr);
+
+  // This call should not crash.
+  document().view()->setScrollOffset(ScrollOffset(0, 100), ProgrammaticScroll);
+}
+
 TEST_P(FrameViewTest, StyleChangeUpdatesViewportConstrainedObjects) {
   // When using root layer scrolling there is no concept of viewport constrained
   // objects, so skip this test.