Detach PaintLayerScrollableArea from RootFrameViewport when disposed. (issue 2365173002 by bokan@chromium.org)

1 view
Skip to first unread message

bo...@chromium.org

unread,
Sep 25, 2016, 11:56:45 AM9/25/16
to dtap...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org
Reviewers: dtapuska
CL: https://codereview.chromium.org/2365173002/


https://codereview.chromium.org/2365173002/diff/40001/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
(right):

https://codereview.chromium.org/2365173002/diff/40001/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h#newcode50
third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:50:
#include "core/page/scrolling/StickyPositionScrollingConstraints.h"
Unrelated but I noticed this is needed to include this file in new
places. It's current uses just happen to have it via other includes.

Description:
Detach PaintLayerScrollableArea from RootFrameViewport when disposed.

With document.rootScroller, a PLSA can become the layoutViewport in
RootFrameViewport. This crash was happening because the associated Node being
removed from the DOM causes deletion of the associated LayoutObject and
PaintLayer but the RootFrameViewport still has a pointer to the PLSA. The
RootScrollerController will realize the rootScroller's LayoutObject is gone
during the next layout but we can call into the dead PLSA (which could have
been garbage collected in the mean time).

This fix checks during PLSA disposal whether it's registered as the layout
viewport, and if so, resets the layout viewport to the FrameView.

BUG=649340
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Affected files (+62, -1 lines):
M third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h
M third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
M third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
M third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
M third_party/WebKit/Source/web/tests/RootScrollerTest.cpp


Index: third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
diff --git a/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp b/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
index 6dfa436619f353c1f553182fe0ce08030e4a2ecb..0ad1b19f5136105c706ca6c7ba37c934ca1a02ac 100644
--- a/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
+++ b/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.cpp
@@ -88,6 +88,16 @@ void RootScrollerController::didUpdateLayout()
recomputeEffectiveRootScroller();
}

+void RootScrollerController::didDisposePaintLayerScrollableArea(
+ PaintLayerScrollableArea& area)
+{
+ if (!m_effectiveRootScroller)
+ return;
+
+ if (&area.box() == m_effectiveRootScroller->layoutObject())
+ recomputeEffectiveRootScroller();
+}
+
void RootScrollerController::recomputeEffectiveRootScroller()
{
bool rootScrollerValid =
@@ -150,8 +160,11 @@ PaintLayer* RootScrollerController::rootScrollerPaintLayer() const
// PaintLayer (i.e. the PaintLayerCompositor's root layer). The reason the root
// scroller is the <html> layer and not #document is because the latter is a Node
// but not an Element.
- if (m_effectiveRootScroller->isSameNode(m_document->documentElement()))
+ if (m_effectiveRootScroller->isSameNode(m_document->documentElement())) {
+ if (!layer || !layer->compositor())
+ return nullptr;
return layer->compositor()->rootLayer();
+ }

return layer;
}
Index: third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h
diff --git a/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h b/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h
index b6bd17714142d1441bfac04689cc94c65831eca1..a293942104a35b6a0819ffe9ea77b1457220a747 100644
--- a/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h
+++ b/third_party/WebKit/Source/core/page/scrolling/RootScrollerController.h
@@ -14,6 +14,7 @@ class Document;
class Element;
class GraphicsLayer;
class PaintLayer;
+class PaintLayerScrollableArea;
class ScrollableArea;
class ScrollStateCallback;

@@ -70,6 +71,10 @@ public:
// replaced by the default root scroller.
void didUpdateLayout();

+ // PaintLayerScrollableAreas need to notify this class when they're being
+ // disposed so that we can remove them as the root scroller.
+ void didDisposePaintLayerScrollableArea(PaintLayerScrollableArea&);
+
// Returns the PaintLayer associated with the currently effective root
// scroller.
PaintLayer* rootScrollerPaintLayer() const;
Index: third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
index ab70822b20994b38efc5be6a20d2a373e360672f..f572ff2bcfa033518902f60fc776a7b44a1cd743 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp
@@ -52,6 +52,7 @@
#include "core/frame/FrameHost.h"
#include "core/frame/FrameView.h"
#include "core/frame/LocalFrame.h"
+#include "core/frame/RootFrameViewport.h"
#include "core/frame/Settings.h"
#include "core/html/HTMLFrameOwnerElement.h"
#include "core/input/EventHandler.h"
@@ -67,6 +68,7 @@
#include "core/page/ChromeClient.h"
#include "core/page/FocusController.h"
#include "core/page/Page.h"
+#include "core/page/scrolling/RootScrollerController.h"
#include "core/page/scrolling/ScrollingCoordinator.h"
#include "core/paint/PaintLayerFragment.h"
#include "platform/PlatformGestureEvent.h"
@@ -156,6 +158,9 @@ void PaintLayerScrollableArea::dispose()
frameView->removeResizerArea(box());
}

+ if (RootScrollerController* controller = box().document().rootScrollerController())
+ controller->didDisposePaintLayerScrollableArea(*this);
+
m_scrollbarManager.dispose();

if (m_scrollCorner)
Index: third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
diff --git a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
index aba96399d77ea7e0b1b02e469e6a6f0eea11467d..611f278f02a8f9ee5a75b00fed1c73d22ea1209b 100644
--- a/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
+++ b/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
@@ -47,6 +47,7 @@
#include "core/CoreExport.h"
#include "core/layout/ScrollAnchor.h"
#include "core/layout/ScrollEnums.h"
+#include "core/page/scrolling/StickyPositionScrollingConstraints.h"
#include "core/paint/PaintInvalidationCapableScrollableArea.h"
#include "core/paint/PaintLayerFragment.h"
#include "platform/heap/Handle.h"
Index: third_party/WebKit/Source/web/tests/RootScrollerTest.cpp
diff --git a/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp b/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp
index 101e7151c738d80d0dbeacfd613849624cfc397c..0ae0336731114da462a5a57ceaed6946c7d5a759 100644
--- a/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp
+++ b/third_party/WebKit/Source/web/tests/RootScrollerTest.cpp
@@ -4,6 +4,7 @@

#include "core/frame/FrameHost.h"
#include "core/frame/FrameView.h"
+#include "core/frame/RootFrameViewport.h"
#include "core/frame/TopControls.h"
#include "core/html/HTMLFrameOwnerElement.h"
#include "core/layout/LayoutBox.h"
@@ -950,6 +951,42 @@ TEST_F(RootScrollerTest, RemoveClippingOnCompositorLayers)
}
}

+// Tests that removing the root scroller element from the DOM resets the
+// effective root scroller without waiting for any lifecycle events.
+TEST_F(RootScrollerTest, RemoveRootScrollerFromDom)
+{
+ initialize("root-scroller-iframe.html");
+
+ {
+ HTMLFrameOwnerElement* iframe = toHTMLFrameOwnerElement(
+ mainFrame()->document()->getElementById("iframe"));
+ Element* innerContainer =
+ iframe->contentDocument()->getElementById("container");
+
+ NonThrowableExceptionState exceptionState;
+ mainFrame()->document()->setRootScroller(iframe, exceptionState);
+ iframe->contentDocument()->setRootScroller(
+ innerContainer, exceptionState);
+ mainFrameView()->updateAllLifecyclePhases();
+
+ ASSERT_EQ(iframe, mainFrame()->document()->rootScroller());
+ ASSERT_EQ(iframe, effectiveRootScroller(mainFrame()->document()));
+ ASSERT_EQ(innerContainer, iframe->contentDocument()->rootScroller());
+ ASSERT_EQ(innerContainer,
+ effectiveRootScroller(iframe->contentDocument()));
+
+ iframe->contentDocument()->body()->setInnerHTML("", exceptionState);
+
+ // If the root scroller wasn't updated by the DOM removal above, this
+ // will touch the disposed root scroller's ScrollableArea.
+ mainFrameView()->getRootFrameViewport()->serviceScrollAnimations(0);
+
+ EXPECT_EQ(iframe->contentDocument()->documentElement(),
+ effectiveRootScroller(iframe->contentDocument()));
+ }
+}
+
+
} // namespace

} // namespace blink


dtap...@chromium.org

unread,
Sep 26, 2016, 9:51:55 AM9/26/16
to bo...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org
On 2016/09/25 15:56:44, bokan (OOO Sept 26-28) wrote:
>
https://codereview.chromium.org/2365173002/diff/40001/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h
> File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right):
>
>
https://codereview.chromium.org/2365173002/diff/40001/third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h#newcode50
> third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:50: #include
> "core/page/scrolling/StickyPositionScrollingConstraints.h"
> Unrelated but I noticed this is needed to include this file in new places.
It's
> current uses just happen to have it via other includes.

seems reasonable. But there seems to be a use after free on the asan bot with a
layout object.

https://codereview.chromium.org/2365173002/

bo...@chromium.org

unread,
Sep 26, 2016, 11:05:06 AM9/26/16
to dtap...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org

bo...@chromium.org

unread,
Sep 29, 2016, 11:17:03 AM9/29/16
to dtap...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org
The issue was that we don't do all the normal tear-down in the case that the
document is being destroyed. Specifically, this meant that we didn't do the
ScrollAnchor removal so the ScrollAnchor kept it's pointer to a deleted
LayoutObject. Recomputing the root scroller means we reassign ScrollableAreas to
the ScrollAnchor which causes it to touch its dead pointer. I've avoided
recomputing the root scroller if we're destroying the document. ptal.

https://codereview.chromium.org/2365173002/

bo...@chromium.org

unread,
Sep 30, 2016, 12:26:11 PM9/30/16
to dtap...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org

dtap...@chromium.org

unread,
Sep 30, 2016, 12:29:21 PM9/30/16
to bo...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 12:30:10 PM9/30/16
to bo...@chromium.org, dtap...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 3:05:45 PM9/30/16
to bo...@chromium.org, dtap...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org
Try jobs failed on following builders:
win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/302928)

https://codereview.chromium.org/2365173002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 3:06:46 PM9/30/16
to bo...@chromium.org, dtap...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 4:35:35 PM9/30/16
to bo...@chromium.org, dtap...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org
Committed patchset #4 (id:60001)

https://codereview.chromium.org/2365173002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Sep 30, 2016, 4:40:42 PM9/30/16
to bo...@chromium.org, dtap...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dongseo...@intel.com, slimming-pa...@chromium.org, blink-rev...@chromium.org
Patchset 4 (id:??) landed as
https://crrev.com/10a29c5ef01177b72535c855713df141a9ef9ddc
Cr-Commit-Position: refs/heads/master@{#422209}

https://codereview.chromium.org/2365173002/
Reply all
Reply to author
Forward
0 new messages