HistoryItems with null CachedPages should never be left in the list of items; causes crash

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

WK1 can re-enter layout during FrameView destruction and crash

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff91ab948a WebCore::PageCache::markPagesForContentsSizeChanged(WebCore::Page&) + 42
1   com.apple.WebCore             	0x00007fff90fda0bd WebCore::FrameView::setContentsSize(WebCore::IntSize const&) + 221
2   com.apple.WebCore             	0x00007fff9100de56 WebCore::FrameView::adjustViewSize() + 150
3   com.apple.WebCore             	0x00007fff91003e90 WebCore::FrameView::layout(bool) + 3312
4   com.apple.WebKitLegacy        	0x00007fff9e2508fc -[WebHTMLView layoutToMinimumPageWidth:height:originalPageWidth:originalPageHeight:maximumShrinkRatio:adjustingViewSize:] + 316
5   com.apple.WebKitLegacy        	0x00007fff9e249f3e -[WebDynamicScrollBarsView(WebInternal) updateScrollers] + 158
6   com.apple.WebCore             	0x00007fff91c6fb4d WebCore::ScrollView::platformSetScrollbarModes() + 45
7   com.apple.WebCore             	0x00007fff90fcdf21 WebCore::ScrollView::setScrollbarModes(WebCore::ScrollbarMode, WebCore::ScrollbarMode, bool, bool) + 241
8   com.apple.WebCore             	0x00007fff91003906 WebCore::FrameView::layout(bool) + 1894
9   com.apple.WebKitLegacy        	0x00007fff9e2508fc -[WebHTMLView layoutToMinimumPageWidth:height:originalPageWidth:originalPageHeight:maximumShrinkRatio:adjustingViewSize:] + 316
10  com.apple.WebKitLegacy        	0x00007fff9e24a4f9 -[WebDynamicScrollBarsView(WebInternal) updateScrollers] + 1625
11  com.apple.WebCore             	0x00007fff91c6fb4d WebCore::ScrollView::platformSetScrollbarModes() + 45
12  com.apple.WebCore             	0x00007fff90fcdf21 WebCore::ScrollView::setScrollbarModes(WebCore::ScrollbarMode, WebCore::ScrollbarMode, bool, bool) + 241
13  com.apple.WebCore             	0x00007fff91034f50 WebCore::FrameView::~FrameView() + 224
14  com.apple.WebCore             	0x00007fff91034e5e WebCore::FrameView::~FrameView() + 14

rdar://problem/22356782

[Beth Dakin]

Created attachment 259486 [details]
Patch

[Beth Dakin]

Created attachment 259505 [details]
Patch

It turns out that fix did not work, so here is a new patch that does work.

[zalan]

Comment on attachment 259505 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:277
>  

I hope that none of the setHas*Scrollbar() calls (few lines below) trigger similar layout.

[Beth Dakin]

(In reply to comment #3)
> Comment on attachment 259505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=259505&action=review
> 
> > Source/WebCore/page/FrameView.cpp:277
> >  
> 
> I hope that none of the setHas*Scrollbar() calls (few lines below) trigger
> similar layout.

From what I can tell, they do not, but this is a great example of why this fix is hack-ish.

[Beth Dakin]

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

Thanks Zalan!

[WebKit Commit Bot]

Re-opened since this is blocked by bug 148274

[Tim Horton]

Rolled out in http://trac.webkit.org/changeset/188723

[Beth Dakin]

Re-naming the bug to reflect my new approach for a crash fix,

[Beth Dakin]

Created attachment 259586 [details]
Patch

Here's another go.

[Simon Fraser (smfr)]

Comment on attachment 259586 [details]
Patch

This seems pretty risky.

[Simon Fraser (smfr)]

I wonder if the earlier patch broke everything because frame().view() is null normally, meaning frame().view() == this is never true?

[Beth Dakin]

(In reply to comment #10)
> Comment on attachment 259586 [details]
> Patch
> 
> This seems pretty risky.

Can you elaborate? I think it seems extremely safe. Safer than limiting calls to resetScrollbars() from the FrameView destructor now that I have a better understanding of those implications.

[Brady Eidson]

Comment on attachment 259586 [details]
Patch

I think this is fine, as long as all the layout tests actually run and don't crash :)

[Beth Dakin]

Thanks Brady! I ran WK1 and WK2 tests locally, and everything seems good.

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

[Simon Fraser (smfr)]

I think it would be good to land a layout test for this, if we can make one.