Crashes under RenderLayer::hitTestLayer under determinePrimarySnapshottedPlugIn()
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Feb 2015 19:04:12 +0000 (19:04 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 13 Feb 2015 19:04:12 +0000 (19:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=141551

Reviewed by Zalan Bujtas.

It's possible for a layout to dirty the parent frame's state, via the calls to
ownerElement()->scheduleSetNeedsStyleRecalc() that RenderLayerCompositor does when
iframes toggle their compositing mode.

That could cause FrameView::updateLayoutAndStyleIfNeededRecursive() to fail to
leave all the frames in a clean state. Later on, we could enter hit testing,
which calls document().updateLayout() on each frame's document. Document::updateLayout()
does layout on all ancestor documents, so in the middle of hit testing, we could
layout a subframe (dirtying an ancestor frame), then layout another frame, which
would forcing that ancestor to be laid out while we're hit testing it, thus
corrupting the RenderLayer tree while it's being iterated over.

Fix by having FrameView::updateLayoutAndStyleIfNeededRecursive() do a second
layout after laying out subframes, which most of the time will be a no-op.

Also add a stronger assertion, that this frame and all subframes are clean
at the end of FrameView::updateLayoutAndStyleIfNeededRecursive() for the
main frame.

Various existing frames tests hit the new assertion if the code change is removed,
so this is covered by existing tests.

* page/FrameView.cpp:
(WebCore::FrameView::needsStyleRecalcOrLayout):
(WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):
* page/FrameView.h:
* rendering/RenderWidget.cpp:
(WebCore::RenderWidget::willBeDestroyed):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@180063 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/rendering/RenderWidget.cpp

index 03d9ece..ff9e205 100644 (file)
@@ -1,3 +1,39 @@
+2015-02-13  Simon Fraser  <simon.fraser@apple.com>
+
+        Crashes under RenderLayer::hitTestLayer under determinePrimarySnapshottedPlugIn()
+        https://bugs.webkit.org/show_bug.cgi?id=141551
+
+        Reviewed by Zalan Bujtas.
+        
+        It's possible for a layout to dirty the parent frame's state, via the calls to
+        ownerElement()->scheduleSetNeedsStyleRecalc() that RenderLayerCompositor does when
+        iframes toggle their compositing mode.
+        
+        That could cause FrameView::updateLayoutAndStyleIfNeededRecursive() to fail to 
+        leave all the frames in a clean state. Later on, we could enter hit testing,
+        which calls document().updateLayout() on each frame's document. Document::updateLayout()
+        does layout on all ancestor documents, so in the middle of hit testing, we could
+        layout a subframe (dirtying an ancestor frame), then layout another frame, which
+        would forcing that ancestor to be laid out while we're hit testing it, thus
+        corrupting the RenderLayer tree while it's being iterated over.
+        
+        Fix by having FrameView::updateLayoutAndStyleIfNeededRecursive() do a second
+        layout after laying out subframes, which most of the time will be a no-op.
+        
+        Also add a stronger assertion, that this frame and all subframes are clean
+        at the end of FrameView::updateLayoutAndStyleIfNeededRecursive() for the
+        main frame.
+
+        Various existing frames tests hit the new assertion if the code change is removed,
+        so this is covered by existing tests.
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::needsStyleRecalcOrLayout):
+        (WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):
+        * page/FrameView.h:
+        * rendering/RenderWidget.cpp:
+        (WebCore::RenderWidget::willBeDestroyed):
+
 2015-02-12  Simon Fraser  <simon.fraser@apple.com>
 
         determinePrimarySnapshottedPlugIn() should only traverse visible Frames
index 7a7c092..7643fb1 100644 (file)
@@ -2561,6 +2561,33 @@ bool FrameView::layoutPending() const
     return m_layoutTimer.isActive();
 }
 
+bool FrameView::needsStyleRecalcOrLayout(bool includeSubframes) const
+{
+    if (frame().document() && frame().document()->childNeedsStyleRecalc())
+        return true;
+    
+    if (needsLayout())
+        return true;
+
+    if (!includeSubframes)
+        return false;
+
+    // Find child frames via the Widget tree, as updateLayoutAndStyleIfNeededRecursive() does.
+    Vector<Ref<FrameView>, 16> childViews;
+    childViews.reserveInitialCapacity(children().size());
+    for (auto& widget : children()) {
+        if (is<FrameView>(*widget))
+            childViews.uncheckedAppend(downcast<FrameView>(*widget));
+    }
+
+    for (unsigned i = 0; i < childViews.size(); ++i) {
+        if (childViews[i]->needsStyleRecalcOrLayout())
+            return true;
+    }
+
+    return false;
+}
+
 bool FrameView::needsLayout() const
 {
     // This can return true in cases where the document does not have a body yet.
@@ -3980,10 +4007,12 @@ void FrameView::updateLayoutAndStyleIfNeededRecursive()
     for (unsigned i = 0; i < childViews.size(); ++i)
         childViews[i]->updateLayoutAndStyleIfNeededRecursive();
 
-    // When frame flattening is on, child frame can mark parent frame dirty. In such case, child frame
-    // needs to call layout on parent frame recursively.
-    // This assert ensures that parent frames are clean, when child frames finished updating layout and style.
-    ASSERT(!needsLayout());
+    // A child frame may have dirtied us during its layout.
+    frame().document()->updateStyleIfNeeded();
+    if (needsLayout())
+        layout();
+
+    ASSERT(!frame().isMainFrame() || !needsStyleRecalcOrLayout());
 }
 
 bool FrameView::qualifiesAsVisuallyNonEmpty() const
index 27bf40c..0a29c38 100644 (file)
@@ -122,6 +122,8 @@ public:
     WEBCORE_EXPORT void setNeedsLayout();
     void setViewportConstrainedObjectsNeedLayout();
 
+    bool needsStyleRecalcOrLayout(bool includeSubframes = true) const;
+
     bool needsFullRepaint() const { return m_needsFullRepaint; }
 
     WEBCORE_EXPORT bool renderedCharactersExceed(unsigned threshold);
index 2e62877..c476512 100644 (file)
@@ -99,7 +99,7 @@ void RenderWidget::willBeDestroyed()
         cache->remove(this);
     }
 
-    setWidget(0);
+    setWidget(nullptr);
 
     RenderReplaced::willBeDestroyed();
 }