Bug 97841 – Crash re-entering Document layout with frame flattening enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Simon Fraser (smfr)]

Navigation with framesets and plugins and the page cache can cause layout to be re-entered, causing crashes. I have a layout test that reproduces this.

In debug, DRT will assert about layout being called on a FrameView whose frame's view is not it:


Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010ca49076 WebCore::RenderView::layout() + 886 (RenderView.cpp:158)
1   com.apple.WebCore             	0x000000010bce8192 WebCore::FrameView::layout(bool) + 3426 (FrameView.cpp:1190)
2   com.apple.WebCore             	0x000000010bce8a3c WebCore::FrameView::doLayoutWithFrameFlattening(bool) + 380 (FrameView.cpp:3128)
3   com.apple.WebCore             	0x000000010bce761d WebCore::FrameView::layout(bool) + 493 (FrameView.cpp:1011)
4   com.apple.WebCore             	0x000000010bcf1765 WebCore::FrameView::forceLayout(bool) + 37 (FrameView.cpp:3412)
5   com.apple.WebKit              	0x000000010af85565 -[WebHTMLView layoutToMinimumPageWidth:height:originalPageWidth:originalPageHeight:maximumShrinkRatio:adjustingViewSize:] + 469 (WebHTMLView.mm:3065)
6   com.apple.WebKit              	0x000000010af855cc -[WebHTMLView layout] + 76 (WebHTMLView.mm:3079)
7   com.apple.WebKit              	0x000000010af226f8 -[WebDynamicScrollBarsView(WebInternal) updateScrollers] + 264 (WebDynamicScrollBarsView.mm:266)
8   com.apple.WebKit              	0x000000010af23364 -[WebDynamicScrollBarsView(WebInternal) reflectScrolledClipView:] + 228 (WebDynamicScrollBarsView.mm:408)
9   com.apple.AppKit              	0x00007fff8d94b59a -[NSClipView _selfBoundsChanged] + 689
10  com.apple.AppKit              	0x00007fff8d94ad70 -[NSClipView setFrameSize:] + 410
11  com.apple.AppKit              	0x00007fff8d8f503e -[NSView setFrame:] + 299
12  com.apple.AppKit              	0x00007fff8d9b8abf -[NSScrollView _setContentViewFrame:] + 596
13  com.apple.AppKit              	0x00007fff8d9b84f7 -[NSScrollView _applyContentAreaLayout:] + 129
14  com.apple.AppKit              	0x00007fff8d9b75ae -[NSScrollView tile] + 2091
15  com.apple.WebKit              	0x000000010af222a9 -[WebDynamicScrollBarsView(WebInternal) tile] + 57 (WebDynamicScrollBarsView.mm:212)
16  com.apple.AppKit              	0x00007fff8d9b6ce6 -[NSScrollView _tileWithoutRecursing] + 49
17  com.apple.AppKit              	0x00007fff8d9b6c6a -[NSScrollView _update] + 30
18  com.apple.AppKit              	0x00007fff8d8f57f8 -[NSView setFrameSize:] + 1101
19  com.apple.AppKit              	0x00007fff8d9bbf0c -[NSScrollView setFrameSize:] + 1131
20  com.apple.AppKit              	0x00007fff8d8f503e -[NSView setFrame:] + 299
21  com.apple.AppKit              	0x00007fff8d94a2a2 -[NSView resizeWithOldSuperviewSize:] + 1502
22  com.apple.AppKit              	0x00007fff8d9493e7 -[NSView resizeSubviewsWithOldSize:] + 318
23  com.apple.AppKit              	0x00007fff8d8f57f8 -[NSView setFrameSize:] + 1101
24  com.apple.WebKit              	0x000000010af51deb -[WebFrameView setFrameSize:] + 267 (WebFrameView.mm:514)
25  com.apple.AppKit              	0x00007fff8d8f503e -[NSView setFrame:] + 299
26  com.apple.WebCore             	0x000000010cf07b1f WebCore::Widget::setFrameRect(WebCore::IntRect const&) + 607 (WidgetMac.mm:178)
27  com.apple.WebCore             	0x000000010cb14df7 WebCore::ScrollView::setFrameRect(WebCore::IntRect const&) + 103 (ScrollView.cpp:872)
28  com.apple.WebCore             	0x000000010bce53cf WebCore::FrameView::setFrameRect(WebCore::IntRect const&) + 95 (FrameView.cpp:442)
29  com.apple.WebCore             	0x000000010ca57425 WebCore::RenderWidget::setWidgetGeometry(WebCore::FractionalLayoutRect const&) + 437 (RenderWidget.cpp:159)
30  com.apple.WebCore             	0x000000010ca576e7 WebCore::RenderWidget::updateWidgetGeometry() + 423 (RenderWidget.cpp:175)
31  com.apple.WebCore             	0x000000010ca587d3 WebCore::RenderWidget::updateWidgetPosition() + 83 (RenderWidget.cpp:331)
32  com.apple.WebCore             	0x000000010c896846 WebCore::RenderFrameBase::layoutWithFlattening(bool, bool) + 374 (RenderFrameBase.cpp:70)
33  com.apple.WebCore             	0x000000010c899ead WebCore::RenderFrameSet::positionFramesWithFlattening() + 893 (RenderFrameSet.cpp:592)
34  com.apple.WebCore             	0x000000010c899951 WebCore::RenderFrameSet::layout() + 961 (RenderFrameSet.cpp:487)
35  com.apple.WebCore             	0x000000010c7bdd6c WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) + 1324 (RenderBlock.cpp:2487)
36  com.apple.WebCore             	0x000000010c7b48d4 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&) + 1316 (RenderBlock.cpp:2421)
37  com.apple.WebCore             	0x000000010c7b1dc6 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) + 1590 (RenderBlock.cpp:1556)
38  com.apple.WebCore             	0x000000010c7b0da5 WebCore::RenderBlock::layout() + 117 (RenderBlock.cpp:1378)
39  com.apple.WebCore             	0x000000010c7bdd6c WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, WebCore::FractionalLayoutUnit&, WebCore::FractionalLayoutUnit&) + 1324 (RenderBlock.cpp:2487)
40  com.apple.WebCore             	0x000000010c7b48d4 WebCore::RenderBlock::layoutBlockChildren(bool, WebCore::FractionalLayoutUnit&) + 1316 (RenderBlock.cpp:2421)
41  com.apple.WebCore             	0x000000010c7b1dc6 WebCore::RenderBlock::layoutBlock(bool, WebCore::FractionalLayoutUnit) + 1590 (RenderBlock.cpp:1556)
42  com.apple.WebCore             	0x000000010c7b0da5 WebCore::RenderBlock::layout() + 117 (RenderBlock.cpp:1378)
43  com.apple.WebCore             	0x000000010ca4912e WebCore::RenderView::layout() + 1070 (RenderView.cpp:170)
44  com.apple.WebCore             	0x000000010bce8192 WebCore::FrameView::layout(bool) + 3426 (FrameView.cpp:1190)
45  com.apple.WebCore             	0x000000010bce8a3c WebCore::FrameView::doLayoutWithFrameFlattening(bool) + 380 (FrameView.cpp:3128)
46  com.apple.WebCore             	0x000000010bce761d WebCore::FrameView::layout(bool) + 493 (FrameView.cpp:1011)
47  com.apple.WebCore             	0x000000010bce3ee8 WebCore::FrameView::layoutTimerFired(WebCore::Timer<WebCore::FrameView>*) + 72 (FrameView.cpp:2125)
48  com.apple.WebCore             	0x000000010bcff713 WebCore::Timer<WebCore::FrameView>::fired() + 115 (Timer.h:100)
49  com.apple.WebCore             	0x000000010ce34bfd WebCore::ThreadTimers::sharedTimerFiredInternal() + 285 (ThreadTimers.cpp:118)
50  com.apple.WebCore             	0x000000010ce34999 WebCore::ThreadTimers::sharedTimerFired() + 25 (ThreadTimers.cpp:94)
51  com.apple.WebCore             	0x000000010cb599a3 WebCore::timerFired(__CFRunLoopTimer*, void*) + 67 (SharedTimerMac.mm:167)

[Radar WebKit Bug Importer]

<rdar://problem/12392198>

[Simon Fraser (smfr)]

Created attachment 166113 [details]
Patch adding a test that shows the crash

[Simon Fraser (smfr)]

The reason we hit the LayoutState crash is that we're re-entering layout on the live Document. This happens as follows:

Cached subframe FrameView does layout from a timer.
Via frame flattening, main cached FrameView does a layout. It gets the document via m_frame->document(), which is the LIVE document, and does layout on it. The live document is a frameset, so does layout on its child frames. The child frame trys to lay outs its parent (beccause of frame flattening), and because the FrameView is different, misses the if (parentView->m_nestedLayoutCount) check, and starts laying out on the Document that is already in the middle of layout.

[Simon Fraser (smfr)]

This bug reflects a number of issue about how the page cache and frame flattening work that I plan to clean up as follows:

1. Make FrameView::parentFrameView() go via the Frame tree, rather than Widgets (it's how flattened frames find their parent frame)
2. Move the frame->clearTimers() in CachedFrame::CachedFrame until after m_document->documentWillSuspendForPageCache(); this will fix the crash.
3. Clear the Frame on a FrameView that's in the page cache, and add some assertions in FrameView to catch anything happening on cached FramesViews
4. Maybe clear the parent pointer on cached subframe Frames in the page cache (if this is not done already).

[Brady Eidson]

(In reply to comment #4)
> This bug reflects a number of issue about how the page cache and frame flattening work that I plan to clean up as follows:
> 
> 1. Make FrameView::parentFrameView() go via the Frame tree, rather than Widgets (it's how flattened frames find their parent frame)
> 2. Move the frame->clearTimers() in CachedFrame::CachedFrame until after m_document->documentWillSuspendForPageCache(); this will fix the crash.
> 3. Clear the Frame on a FrameView that's in the page cache, and add some assertions in FrameView to catch anything happening on cached FramesViews

These all seem reasonable!

(In reply to comment #4)
> 4. Maybe clear the parent pointer on cached subframe Frames in the page cache (if this is not done already).

We do this already.

[Simon Fraser (smfr)]

Created attachment 166303 [details]
Parent FrameView patch

[Kenneth Rohde Christiansen]

Comment on attachment 166303 [details]
Parent FrameView patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166303&action=review

> Source/WebCore/ChangeLog:8
> +        Walking up to parent FrameViews when doing a fram-flattening

frame*

[Simon Fraser (smfr)]

Comment on attachment 166303 [details]
Parent FrameView patch

http://trac.webkit.org/changeset/129944

[Simon Fraser (smfr)]

Not done yet.

[Simon Fraser (smfr)]

Created attachment 166319 [details]
clearTimers() patch

[Simon Fraser (smfr)]

Comment on attachment 166319 [details]
clearTimers() patch

http://trac.webkit.org/changeset/129966

Keeping bug open to clear out the Frame on cached FrameViews.

[Simon Fraser (smfr)]

I filed bug 98057 to do the longer-term improvement of nulling out the Frame pointer on FrameViews in the page cache. I tried, and had problems because of Frame lifetime changes. So I'm calling this bug complete.