Bug 148237 - HistoryItems with null CachedPages should never be left in the list of items; causes crash
Summary: HistoryItems with null CachedPages should never be left in the list of items;...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-08-20 12:34 PDT by Beth Dakin
Modified: 2015-08-21 17:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.21 KB, patch)
2015-08-20 12:38 PDT, Beth Dakin
zalan: review+
Details | Formatted Diff | Diff
Patch (2.07 KB, patch)
2015-08-20 15:19 PDT, Beth Dakin
zalan: review+
Details | Formatted Diff | Diff
Patch (2.27 KB, patch)
2015-08-20 23:15 PDT, Beth Dakin
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2015-08-20 12:34:27 PDT
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
Comment 1 Beth Dakin 2015-08-20 12:38:23 PDT
Created attachment 259486 [details]
Patch
Comment 2 Beth Dakin 2015-08-20 15:19:44 PDT
Created attachment 259505 [details]
Patch

It turns out that fix did not work, so here is a new patch that does work.
Comment 3 zalan 2015-08-20 15:24:08 PDT
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.
Comment 4 Beth Dakin 2015-08-20 15:36:08 PDT
(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.
Comment 5 Beth Dakin 2015-08-20 15:37:50 PDT
http://trac.webkit.org/changeset/188712

Thanks Zalan!
Comment 6 WebKit Commit Bot 2015-08-20 17:51:53 PDT
Re-opened since this is blocked by bug 148274
Comment 7 Tim Horton 2015-08-20 18:03:59 PDT
Rolled out in http://trac.webkit.org/changeset/188723
Comment 8 Beth Dakin 2015-08-20 22:27:18 PDT
Re-naming the bug to reflect my new approach for a crash fix,
Comment 9 Beth Dakin 2015-08-20 23:15:29 PDT
Created attachment 259586 [details]
Patch

Here's another go.
Comment 10 Simon Fraser (smfr) 2015-08-20 23:24:07 PDT
Comment on attachment 259586 [details]
Patch

This seems pretty risky.
Comment 11 Simon Fraser (smfr) 2015-08-20 23:25:58 PDT
I wonder if the earlier patch broke everything because frame().view() is null normally, meaning frame().view() == this is never true?
Comment 12 Beth Dakin 2015-08-21 10:36:38 PDT
(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.
Comment 13 Brady Eidson 2015-08-21 10:46:31 PDT
Comment on attachment 259586 [details]
Patch

I think this is fine, as long as all the layout tests actually run and don't crash :)
Comment 14 Beth Dakin 2015-08-21 12:24:02 PDT
Thanks Brady! I ran WK1 and WK2 tests locally, and everything seems good.

http://trac.webkit.org/changeset/188765
Comment 15 Simon Fraser (smfr) 2015-08-21 17:45:32 PDT
I think it would be good to land a layout test for this, if we can make one.