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
Created attachment 259486 [details] Patch
Created attachment 259505 [details] Patch It turns out that fix did not work, so here is a new patch that does work.
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.
(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.
http://trac.webkit.org/changeset/188712 Thanks Zalan!
Re-opened since this is blocked by bug 148274
Rolled out in http://trac.webkit.org/changeset/188723
Re-naming the bug to reflect my new approach for a crash fix,
Created attachment 259586 [details] Patch Here's another go.
Comment on attachment 259586 [details] Patch This seems pretty risky.
I wonder if the earlier patch broke everything because frame().view() is null normally, meaning frame().view() == this is never true?
(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 on attachment 259586 [details] Patch I think this is fine, as long as all the layout tests actually run and don't crash :)
Thanks Brady! I ran WK1 and WK2 tests locally, and everything seems good. http://trac.webkit.org/changeset/188765
I think it would be good to land a layout test for this, if we can make one.