Bug 186393 – Crash under Page::scrollingCoordinator()

• Component: Layout and Rendering
• Status: NEW

[Antoine Quint]

We've been getting reports of crashes in Page::scrollingCoordinator() with the following trace:

0   com.apple.WebCore             	0x00007fff55293549 WebCore::Page::scrollingCoordinator() + 9
1   com.apple.WebCore             	0x00007fff552d6028 WebCore::RenderLayer::~RenderLayer() + 408
2   com.apple.WebCore             	0x00007fff552d5e7e WebCore::RenderLayer::~RenderLayer() + 14
3   com.apple.WebCore             	0x00007fff552d5a01 WebCore::RenderLayerModelObject::willBeDestroyed() + 145
4   com.apple.WebCore             	0x00007fff552d5964 WebCore::RenderBoxModelObject::willBeDestroyed() + 452
5   com.apple.WebCore             	0x00007fff552d578c WebCore::RenderBox::willBeDestroyed() + 476
6   com.apple.WebCore             	0x00007fff552d5522 WebCore::RenderObject::destroy() + 82
7   com.apple.WebCore             	0x00007fff564aa838 WebCore::RenderElement::removeAndDestroyChild(WebCore::RenderObject&) + 56
8   com.apple.WebCore             	0x00007fff565f07f1 WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType)::$_8::operator()(unsigned int) const + 161
9   com.apple.WebCore             	0x00007fff565eff5c WebCore::RenderTreeUpdater::tearDownRenderers(WebCore::Element&, WebCore::RenderTreeUpdater::TeardownType) + 1100
10  com.apple.WebCore             	0x00007fff55ef32d2 WebCore::Document::destroyRenderTree() + 210
11  com.apple.WebCore             	0x00007fff552d4dce WebCore::Document::prepareForDestruction() + 654
12  com.apple.WebCore             	0x00007fff56238841 WebCore::Frame::setView(WTF::RefPtr<WebCore::FrameView, WTF::DumbPtrTraits<WebCore::FrameView> >&&) + 177
13  com.apple.WebCore             	0x00007fff553224c9 WebCore::FrameLoader::detachFromParent() + 537
14  com.apple.WebCore             	0x00007fff55361a36 WebCore::FrameLoader::frameDetached() + 70
15  com.apple.WebCore             	0x00007fff55361994 WebCore::HTMLFrameOwnerElement::disconnectContentFrame() + 36
16  com.apple.WebCore             	0x00007fff55ede94b WebCore::disconnectSubframes(WebCore::ContainerNode&, WebCore::SubframeDisconnectPolicy) + 299
17  com.apple.WebCore             	0x00007fff552d4d84 WebCore::Document::prepareForDestruction() + 580
18  com.apple.WebCore             	0x00007fff553ce48d WebCore::CachedFrame::destroy() + 253
19  com.apple.WebCore             	0x00007fff56010b74 WebCore::PageCache::prune(WebCore::PruningReason) + 100
20  com.apple.WebCore             	0x00007fff56010af8 WebCore::PageCache::pruneToSizeNow(unsigned int, WebCore::PruningReason) + 24
21  com.apple.WebKit              	0x00007fff56bd5fc5 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 119
22  com.apple.WebKit              	0x00007fff56bd8b1c IPC::Connection::dispatchOneMessage() + 176
23  com.apple.JavaScriptCore      	0x00007fff4bbddf6c WTF::RunLoop::performWork() + 236
24  com.apple.JavaScriptCore      	0x00007fff4bbde202 WTF::RunLoop::performWork(void*) + 34
25  com.apple.CoreFoundation      	0x00007fff47fc6a61 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
26  com.apple.CoreFoundation      	0x00007fff4808047c __CFRunLoopDoSource0 + 108
27  com.apple.CoreFoundation      	0x00007fff47fa94c0 __CFRunLoopDoSources0 + 208
28  com.apple.CoreFoundation      	0x00007fff47fa893d __CFRunLoopRun + 1293
29  com.apple.CoreFoundation      	0x00007fff47fa81a3 CFRunLoopRunSpecific + 483
30  com.apple.HIToolbox           	0x00007fff47290d96 RunCurrentEventLoopInMode + 286
31  com.apple.HIToolbox           	0x00007fff47290b06 ReceiveNextEventCommon + 613
32  com.apple.HIToolbox           	0x00007fff47290884 _BlockUntilNextEventMatchingListInModeWithFilter + 64
33  com.apple.AppKit              	0x00007fff45543b53 _DPSNextEvent + 2085
34  com.apple.AppKit              	0x00007fff45cd9eb0 -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 3044
35  com.apple.AppKit              	0x00007fff45538965 -[NSApplication run] + 764
36  com.apple.AppKit              	0x00007fff45507b3e NSApplicationMain + 804
37  libxpc.dylib                  	0x00007fff70618f57 _xpc_objc_main + 580
38  libxpc.dylib                  	0x00007fff70617baa xpc_main + 417
39  com.apple.WebKit.WebContent   	0x1048c46a1 main + 490 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/WebKit2/WebKit2-7605.1.33.1.2/Shared/EntryPointUtilities/mac/XPCService/XPCServiceMain.mm:148)
40  libdyld.dylib                 	0x00007fff702be015 start + 1

[Antoine Quint]

<rdar://problem/38424306>

[Antoine Quint]

ScrollingCoordinator* Page::scrollingCoordinator()
{
    if (!m_scrollingCoordinator && m_settings->scrollingCoordinatorEnabled()) {
        m_scrollingCoordinator = chrome().client().createScrollingCoordinator(*this);
        if (!m_scrollingCoordinator)
            m_scrollingCoordinator = ScrollingCoordinator::create(this);
    }

    return m_scrollingCoordinator.get();
}

I suppose we could crash if either m_settings is nullptr or if m_scrollingCoordinator is nullptr and m_settings->scrollingCoordinatorEnabled() is false.

[Antoine Quint]

Created attachment 342156 [details]
Patch

[zalan]

Comment on attachment 342156 [details]
Patch

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

> Source/WebCore/page/Page.cpp:377
> +    if (!m_settings || !m_settings->scrollingCoordinatorEnabled())
> +        return nullptr;

m_settings can't really be nullptr and as long as the Page::settings() returns Settings& we should not add random nullptr checks (since the contract is that as long as the page is valid, the settings is valid too )
Not sure way we would crash with m_scrollingCoordinator nullptr. If we end up not constructing a scrollingCoordinator object, m_scrollingCoordinator.get() would just return nullptr.
Also I think this stacktrace is about accessing an invalid Page object.

[Antoine Quint]

But the call site in ~RenderLayer() accesses the Page through renderer().page().scrollingCoordinator() and this should return a Page&, so this ought not be null either.

[zalan]

(In reply to Antoine Quint from comment #5)
> But the call site in ~RenderLayer() accesses the Page through
> renderer().page().scrollingCoordinator() and this should return a Page&, so
> this ought not be null either.
Sure and if it turns out to be null, then we need to find out why and assess whether it's a valid case and either change the contract (Page& -> Page*) or fix the broken case.

[zalan]

(In reply to zalan from comment #6)
> (In reply to Antoine Quint from comment #5)
> > But the call site in ~RenderLayer() accesses the Page through
> > renderer().page().scrollingCoordinator() and this should return a Page&, so
> > this ought not be null either.
> Sure and if it turns out to be null, then we need to find out why and assess
> whether it's a valid case and either change the contract (Page& -> Page*) or
> fix the broken case.
My point is that the combination of null checks and returning the reference is confusing and should be avoided. it's either guaranteed to be not null -> &, or null -> * (there are obviously some exceptions but I don't think this falls into that category)