Changeset 180063 in webkit

Timestamp:

Feb 13, 2015, 11:04:12 AM (10 years ago)

Author:

Simon Fraser

Message:

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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/FrameView.cpp (modified) (2 diffs)
• page/FrameView.h (modified) (1 diff)
• rendering/RenderWidget.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog ¶

@@ +1 @@
+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>
 

trunk/Source/WebCore/page/FrameView.cpp ¶

@@ -2562 +2562 @@
 }
 
+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
 {
@@ -3981 +4008 @@
         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());
 }
 

trunk/Source/WebCore/page/FrameView.h ¶

@@ -123 +123 @@
     void setViewportConstrainedObjectsNeedLayout();
 
+    bool needsStyleRecalcOrLayout(bool includeSubframes = true) const;
+
     bool needsFullRepaint() const { return m_needsFullRepaint; }
 

trunk/Source/WebCore/rendering/RenderWidget.cpp ¶

@@ -100 +100 @@
     }
 
-    setWidget(0);
+    setWidget(nullptr);
 
     RenderReplaced::willBeDestroyed();