<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[210777] trunk/Source/WebCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/210777">210777</a></dd>
<dt>Author</dt> <dd>akling@apple.com</dd>
<dt>Date</dt> <dd>2017-01-15 02:48:04 -0800 (Sun, 15 Jan 2017)</dd>
</dl>
<h3>Log Message</h3>
<pre>FrameView shouldn't keep dangling pointers into dead render trees.
<https://webkit.org/b/167011>
Reviewed by Antti Koivisto.
Added some pretty paranoid assertions to FrameView that verify all of its raw pointers
into the render tree are gone after the render tree has been destroyed.
They immediately caught two bugs, also fixed in this patch.
* page/FrameView.h:
* page/FrameView.cpp:
(WebCore::FrameView::willDestroyRenderTree):
(WebCore::FrameView::didDestroyRenderTree): Added these two callbacks for before/after
Document tears down its render tree. The former clears the layout root, and detaches
custom scrollbars. The latter contains a bunch of sanity assertions that pointers into
the now-destroyed render tree are gone.
* dom/Document.cpp:
(WebCore::Document::destroyRenderTree): Notify FrameView before/after teardown.
* page/animation/AnimationController.h:
* page/animation/AnimationController.cpp:
(WebCore::AnimationController::hasAnimations): Added a helper to check if there are
any composite animations around, as these contain raw pointers to renderers.
* rendering/RenderElement.cpp:
(WebCore::RenderElement::willBeRemovedFromTree):
(WebCore::RenderElement::willBeDestroyed): Moved slow repaint object unregistration
from willBeRemovedFromTree() to willBeDestroyed(). The willBeRemovedFromTree() callback
is skipped as an optimization during full tree teardown, but willBeDestroyed() always
gets called. This fixes a bug where we'd fail to remove dangling pointers.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoredomDocumentcpp">trunk/Source/WebCore/dom/Document.cpp</a></li>
<li><a href="#trunkSourceWebCorepageFrameViewcpp">trunk/Source/WebCore/page/FrameView.cpp</a></li>
<li><a href="#trunkSourceWebCorepageFrameViewh">trunk/Source/WebCore/page/FrameView.h</a></li>
<li><a href="#trunkSourceWebCorepageanimationAnimationControllercpp">trunk/Source/WebCore/page/animation/AnimationController.cpp</a></li>
<li><a href="#trunkSourceWebCorepageanimationAnimationControllerh">trunk/Source/WebCore/page/animation/AnimationController.h</a></li>
<li><a href="#trunkSourceWebCorerenderingRenderElementcpp">trunk/Source/WebCore/rendering/RenderElement.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (210776 => 210777)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog 2017-01-15 08:23:31 UTC (rev 210776)
+++ trunk/Source/WebCore/ChangeLog 2017-01-15 10:48:04 UTC (rev 210777)
</span><span class="lines">@@ -1,3 +1,37 @@
</span><ins>+2017-01-15 Andreas Kling <akling@apple.com>
+
+ FrameView shouldn't keep dangling pointers into dead render trees.
+ <https://webkit.org/b/167011>
+
+ Reviewed by Antti Koivisto.
+
+ Added some pretty paranoid assertions to FrameView that verify all of its raw pointers
+ into the render tree are gone after the render tree has been destroyed.
+ They immediately caught two bugs, also fixed in this patch.
+
+ * page/FrameView.h:
+ * page/FrameView.cpp:
+ (WebCore::FrameView::willDestroyRenderTree):
+ (WebCore::FrameView::didDestroyRenderTree): Added these two callbacks for before/after
+ Document tears down its render tree. The former clears the layout root, and detaches
+ custom scrollbars. The latter contains a bunch of sanity assertions that pointers into
+ the now-destroyed render tree are gone.
+
+ * dom/Document.cpp:
+ (WebCore::Document::destroyRenderTree): Notify FrameView before/after teardown.
+
+ * page/animation/AnimationController.h:
+ * page/animation/AnimationController.cpp:
+ (WebCore::AnimationController::hasAnimations): Added a helper to check if there are
+ any composite animations around, as these contain raw pointers to renderers.
+
+ * rendering/RenderElement.cpp:
+ (WebCore::RenderElement::willBeRemovedFromTree):
+ (WebCore::RenderElement::willBeDestroyed): Moved slow repaint object unregistration
+ from willBeRemovedFromTree() to willBeDestroyed(). The willBeRemovedFromTree() callback
+ is skipped as an optimization during full tree teardown, but willBeDestroyed() always
+ gets called. This fixes a bug where we'd fail to remove dangling pointers.
+
</ins><span class="cx"> 2017-01-15 Myles C. Maxfield <mmaxfield@apple.com>
</span><span class="cx">
</span><span class="cx"> [Cocoa] Unify font fallback between macOS and iOS for when the font-family list is exhausted
</span></span></pre></div>
<a id="trunkSourceWebCoredomDocumentcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/dom/Document.cpp (210776 => 210777)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/dom/Document.cpp 2017-01-15 08:23:31 UTC (rev 210776)
+++ trunk/Source/WebCore/dom/Document.cpp 2017-01-15 10:48:04 UTC (rev 210777)
</span><span class="lines">@@ -2199,8 +2199,11 @@
</span><span class="cx"> {
</span><span class="cx"> ASSERT(hasLivingRenderTree());
</span><span class="cx"> ASSERT(frame());
</span><ins>+ ASSERT(frame()->view());
</ins><span class="cx"> ASSERT(page());
</span><span class="cx">
</span><ins>+ FrameView& frameView = *frame()->view();
+
</ins><span class="cx"> // Prevent Widget tree changes from committing until the RenderView is dead and gone.
</span><span class="cx"> WidgetHierarchyUpdatesSuspensionScope suspendWidgetHierarchyUpdates;
</span><span class="cx">
</span><span class="lines">@@ -2211,8 +2214,7 @@
</span><span class="cx">
</span><span class="cx"> documentWillBecomeInactive();
</span><span class="cx">
</span><del>- if (FrameView* frameView = view())
- frameView->detachCustomScrollbars();
</del><ins>+ frameView.willDestroyRenderTree();
</ins><span class="cx">
</span><span class="cx"> #if ENABLE(FULLSCREEN_API)
</span><span class="cx"> if (m_fullScreenRenderer)
</span><span class="lines">@@ -2238,6 +2240,8 @@
</span><span class="cx"> // Do this before the arena is cleared, which is needed to deref the RenderStyle on TextAutoSizingKey.
</span><span class="cx"> m_textAutoSizedNodes.clear();
</span><span class="cx"> #endif
</span><ins>+
+ frameView.didDestroyRenderTree();
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void Document::prepareForDestruction()
</span></span></pre></div>
<a id="trunkSourceWebCorepageFrameViewcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/FrameView.cpp (210776 => 210777)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/FrameView.cpp 2017-01-15 08:23:31 UTC (rev 210776)
+++ trunk/Source/WebCore/page/FrameView.cpp 2017-01-15 10:48:04 UTC (rev 210777)
</span><span class="lines">@@ -644,6 +644,27 @@
</span><span class="cx"> updateScrollableAreaSet();
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+void FrameView::willDestroyRenderTree()
+{
+ detachCustomScrollbars();
+ m_layoutRoot = nullptr;
+}
+
+void FrameView::didDestroyRenderTree()
+{
+ ASSERT(!m_layoutRoot);
+ ASSERT(m_widgetsInRenderTree.isEmpty());
+
+ // If the render tree is destroyed below FrameView::updateEmbeddedObjects(), there will still be a null sentinel in the set.
+ // Everything else should have removed itself as the tree was felled.
+ ASSERT(!m_embeddedObjectsToUpdate || m_embeddedObjectsToUpdate->isEmpty() || (m_embeddedObjectsToUpdate->size() == 1 && m_embeddedObjectsToUpdate->first() == nullptr));
+
+ ASSERT(!m_viewportConstrainedObjects || m_viewportConstrainedObjects->isEmpty());
+ ASSERT(!m_slowRepaintObjects || m_slowRepaintObjects->isEmpty());
+
+ ASSERT(!frame().animation().hasAnimations());
+}
+
</ins><span class="cx"> void FrameView::setContentsSize(const IntSize& size)
</span><span class="cx"> {
</span><span class="cx"> if (size == contentsSize())
</span></span></pre></div>
<a id="trunkSourceWebCorepageFrameViewh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/FrameView.h (210776 => 210777)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/FrameView.h 2017-01-15 08:23:31 UTC (rev 210776)
+++ trunk/Source/WebCore/page/FrameView.h 2017-01-15 10:48:04 UTC (rev 210777)
</span><span class="lines">@@ -589,6 +589,9 @@
</span><span class="cx">
</span><span class="cx"> void didRestoreFromPageCache();
</span><span class="cx">
</span><ins>+ void willDestroyRenderTree();
+ void didDestroyRenderTree();
+
</ins><span class="cx"> protected:
</span><span class="cx"> bool scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect) final;
</span><span class="cx"> void scrollContentsSlowPath(const IntRect& updateRect) final;
</span></span></pre></div>
<a id="trunkSourceWebCorepageanimationAnimationControllercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/animation/AnimationController.cpp (210776 => 210777)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/animation/AnimationController.cpp 2017-01-15 08:23:31 UTC (rev 210776)
+++ trunk/Source/WebCore/page/animation/AnimationController.cpp 2017-01-15 10:48:04 UTC (rev 210777)
</span><span class="lines">@@ -761,4 +761,9 @@
</span><span class="cx"> }
</span><span class="cx"> #endif
</span><span class="cx">
</span><ins>+bool AnimationController::hasAnimations() const
+{
+ return m_data->hasAnimations();
+}
+
</ins><span class="cx"> } // namespace WebCore
</span></span></pre></div>
<a id="trunkSourceWebCorepageanimationAnimationControllerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/animation/AnimationController.h (210776 => 210777)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/animation/AnimationController.h 2017-01-15 08:23:31 UTC (rev 210776)
+++ trunk/Source/WebCore/page/animation/AnimationController.h 2017-01-15 10:48:04 UTC (rev 210777)
</span><span class="lines">@@ -91,6 +91,8 @@
</span><span class="cx"> void scrollWasUpdated();
</span><span class="cx"> #endif
</span><span class="cx">
</span><ins>+ bool hasAnimations() const;
+
</ins><span class="cx"> private:
</span><span class="cx"> const std::unique_ptr<AnimationControllerPrivate> m_data;
</span><span class="cx"> };
</span></span></pre></div>
<a id="trunkSourceWebCorerenderingRenderElementcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/rendering/RenderElement.cpp (210776 => 210777)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/rendering/RenderElement.cpp 2017-01-15 08:23:31 UTC (rev 210776)
+++ trunk/Source/WebCore/rendering/RenderElement.cpp 2017-01-15 10:48:04 UTC (rev 210777)
</span><span class="lines">@@ -1087,9 +1087,6 @@
</span><span class="cx"> removeLayers(layer);
</span><span class="cx"> }
</span><span class="cx">
</span><del>- if (m_style.hasFixedBackgroundImage() && !settings().fixedBackgroundsPaintRelativeToDocument())
- view().frameView().removeSlowRepaintObject(this);
-
</del><span class="cx"> if (isOutOfFlowPositioned() && parent()->childrenInline())
</span><span class="cx"> parent()->dirtyLinesFromChangedChild(*this);
</span><span class="cx">
</span><span class="lines">@@ -1116,6 +1113,9 @@
</span><span class="cx">
</span><span class="cx"> void RenderElement::willBeDestroyed()
</span><span class="cx"> {
</span><ins>+ if (m_style.hasFixedBackgroundImage() && !settings().fixedBackgroundsPaintRelativeToDocument())
+ view().frameView().removeSlowRepaintObject(this);
+
</ins><span class="cx"> animation().cancelAnimations(*this);
</span><span class="cx">
</span><span class="cx"> destroyLeftoverChildren();
</span><span class="lines">@@ -1133,6 +1133,7 @@
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx"> #endif
</span><ins>+
</ins><span class="cx"> clearLayoutRootIfNeeded();
</span><span class="cx"> }
</span><span class="cx">
</span></span></pre>
</div>
</div>
</body>
</html>