186393 2018-06-07 06:47:46 -0700 Crash under Page::scrollingCoordinator() 2018-06-07 08:29:43 -0700 1 1 1 Unclassified WebKit Layout and Rendering Safari 11 Unspecified macOS 10.13 NEW InRadar P2 Normal --- 1 graouts graouts bfulgham dino simon.fraser webkit-bug-importer zalan oldest_to_newest 1430805 0 graouts 2018-06-07 06:47:46 -0700 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 1430806 1 graouts 2018-06-07 06:48:16 -0700 <rdar://problem/38424306> 1430807 2 graouts 2018-06-07 06:49:20 -0700 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. 1430808 3 342156 graouts 2018-06-07 06:51:33 -0700 Created attachment 342156 Patch 1430811 4 342156 zalan 2018-06-07 07:19:58 -0700 Comment on attachment 342156 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. 1430831 5 graouts 2018-06-07 08:22:29 -0700 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. 1430836 6 zalan 2018-06-07 08:25:57 -0700 (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. 1430837 7 zalan 2018-06-07 08:29:43 -0700 (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) 342156 2018-06-07 06:51:33 -0700 2018-06-07 07:19:58 -0700 Patch bug-186393-20180607155132.patch text/plain 1789 graouts U3VidmVyc2lvbiBSZXZpc2lvbjogMjMyNTczCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYzI3ZTZiNDVkNjdkN2U0 NDBmNTMxMTg0ZjVmNjJmYWI3NDkyZDE3ZC4uMWIyYWQwMTVmMmQyZmNlODg1MDUwNjU0ZWMwMzIz YmYxMDlmODVjZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDE4LTA2LTA3ICBBbnRv aW5lIFF1aW50ICA8Z3Jhb3V0c0BhcHBsZS5jb20+CisKKyAgICAgICAgQ3Jhc2ggdW5kZXIgUGFn ZTo6c2Nyb2xsaW5nQ29vcmRpbmF0b3IoKQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MTg2MzkzCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zODQyNDMw Nj4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBCbGlu ZCBhdHRlbXB0IGF0IGZpeGluZyBhIGNyYXNoIGluIFBhZ2U6OnNjcm9sbGluZ0Nvb3JkaW5hdG9y KCkuIExvb2tpbmcgYXQgdGhlIGNvZGUsIHdlIGNvdWxkIGNyYXNoIGlmIGVpdGhlcgorICAgICAg ICBtX3NldHRpbmdzIGlzIG51bGxwdHIgb3IgaWYgbV9zY3JvbGxpbmdDb29yZGluYXRvciBpcyBu dWxscHRyIGFuZCBtX3NldHRpbmdzLT5zY3JvbGxpbmdDb29yZGluYXRvckVuYWJsZWQoKSBpcwor ICAgICAgICBmYWxzZS4gU28gd2UgZ3VhcmQgYWdhaW5zdCBib3RoIHNjZW5hcmlvcyBhbmQgcmV0 dXJuIG51bGxwdHIgaW4gdGhlc2Ugc2l0dWF0aW9ucy4KKworICAgICAgICAqIHBhZ2UvUGFnZS5j cHA6CisgICAgICAgIChXZWJDb3JlOjpQYWdlOjpzY3JvbGxpbmdDb29yZGluYXRvcik6CisKIDIw MTgtMDYtMDYgIFlvdWVubiBGYWJsZXQgIDx5b3Vlbm5AYXBwbGUuY29tPgogCiAgICAgICAgIEhU VFAgSGVhZGVyIHZhbHVlcyB2YWxpZGF0aW9uIGlzIHRvbyBzdHJpY3QKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL3BhZ2UvUGFnZS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL1BhZ2UuY3Bw CmluZGV4IDllOWRlZmM4ZjRiMmQ3NDQzNGJmYTZhM2U0YTk3NGY4NzI0NGI1MWYuLjJjZDIzZmFk ZWQwOTJjMGQ1YjI1OGRiZGRlNDg2OWUyOGI4MmQ0YjYgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJD b3JlL3BhZ2UvUGFnZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGFnZS9QYWdlLmNwcApAQCAt MzczLDcgKzM3MywxMCBAQCBWaWV3cG9ydEFyZ3VtZW50cyBQYWdlOjp2aWV3cG9ydEFyZ3VtZW50 cygpIGNvbnN0CiAKIFNjcm9sbGluZ0Nvb3JkaW5hdG9yKiBQYWdlOjpzY3JvbGxpbmdDb29yZGlu YXRvcigpCiB7Ci0gICAgaWYgKCFtX3Njcm9sbGluZ0Nvb3JkaW5hdG9yICYmIG1fc2V0dGluZ3Mt PnNjcm9sbGluZ0Nvb3JkaW5hdG9yRW5hYmxlZCgpKSB7CisgICAgaWYgKCFtX3NldHRpbmdzIHx8 ICFtX3NldHRpbmdzLT5zY3JvbGxpbmdDb29yZGluYXRvckVuYWJsZWQoKSkKKyAgICAgICAgcmV0 dXJuIG51bGxwdHI7CisKKyAgICBpZiAoIW1fc2Nyb2xsaW5nQ29vcmRpbmF0b3IpIHsKICAgICAg ICAgbV9zY3JvbGxpbmdDb29yZGluYXRvciA9IGNocm9tZSgpLmNsaWVudCgpLmNyZWF0ZVNjcm9s bGluZ0Nvb3JkaW5hdG9yKCp0aGlzKTsKICAgICAgICAgaWYgKCFtX3Njcm9sbGluZ0Nvb3JkaW5h dG9yKQogICAgICAgICAgICAgbV9zY3JvbGxpbmdDb29yZGluYXRvciA9IFNjcm9sbGluZ0Nvb3Jk aW5hdG9yOjpjcmVhdGUodGhpcyk7Cg==