Bug 96863

Summary:

REGRESSION(r124168): Null crash in RenderLayer::createScrollbar

Product:

WebKit

Reporter:

Abhishek Arya <inferno>

Component:

Layout and Rendering

Assignee:

Julien Chaffraix <jchaffraix>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

enne, eric, jchaffraix, simon.fraser, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Test patch for an early EWS run.	none
Proposed fix: avoid triggering a style change if we only need a temporary style.	none
Patch for landing	none

Abhishek Arya

Reported 2012-09-15 09:23:11 PDT

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=90515299 http://code.google.com/p/chromium/issues/detail?id=149813 Fuzzer: Bj_doc_fuzzer Crash Type: UNKNOWN Crash Address: 0x000000000031 Crash State: - crash stack - WebCore::RenderLayer::createScrollbar WebCore::RenderLayer::setHasHorizontalScrollbar WebCore::RenderLayer::updateScrollbarsAfterStyleChange Regressed: https://cluster-fuzz.appspot.com/revisions?range=149138:149142 Minimized Testcase (0.09 Kb): Download: https://cluster-fuzz.appspot.com/download/AMIfv97GA52fsm5JFjaslhsLLqH4yCIc287Po1v-VilVZ4WcPbajFwvqHvTUMB01SXmdbmEu1AmtBr7uEwFH6zIJtxeePU44CfHt-iFF4HQYe7KxaF9ALsXuyxC-x0JeXbd-m554J7TUyWjNsUh_dpztGT5Fr-nkV8iDZA0gIy4m8SiGviynBBs <html class="class3"> <style> .class3 { overflow:scroll; content:url(data:text/plain,aaa); AddressSanitizer can not provide additional info. ABORTING #0 0x7fbe255cba4e in WebCore::RenderObject::RenderObjectBitfields::isBox() const third_party/WebKit/Source/WebCore/rendering/RenderObject.h:1021 #1 0x7fbe255cb91d in WebCore::RenderObject::isBox() const third_party/WebKit/Source/WebCore/rendering/RenderObject.h:513 #2 0x7fbe31c5a851 in WebCore::RenderLayer::createScrollbar(WebCore::ScrollbarOrientation) third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:2261 #3 0x7fbe31c5b613 in WebCore::RenderLayer::setHasHorizontalScrollbar(bool) third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:2312 #4 0x7fbe31c99f13 in WebCore::RenderLayer::updateScrollbarsAfterStyleChange(WebCore::RenderStyle const*) third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4857 #5 0x7fbe31c9b32e in WebCore::RenderLayer::styleChanged(WebCore::StyleDifference, WebCore::RenderStyle const*) third_party/WebKit/Source/WebCore/rendering/RenderLayer.cpp:4900 #6 0x7fbe319a3206 in WebCore::RenderBoxModelObject::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) third_party/WebKit/Source/WebCore/rendering/RenderBoxModelObject.cpp:445 #7 0x7fbe31902337 in WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) third_party/WebKit/Source/WebCore/rendering/RenderBox.cpp:235 #8 0x7fbe31f1e378 in WebCore::RenderReplaced::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) third_party/WebKit/Source/WebCore/rendering/RenderReplaced.cpp:74 #9 0x7fbe31bc979b in WebCore::RenderImage::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) third_party/WebKit/Source/WebCore/rendering/RenderImage.cpp:135 #10 0x7fbe31ea2900 in WebCore::RenderObject::setStyle(WTF::PassRefPtr<WebCore::RenderStyle>) third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:1759 #11 0x7fbe31e63423 in WebCore::RenderObject::createObject(WebCore::Node*, WebCore::RenderStyle*) third_party/WebKit/Source/WebCore/rendering/RenderObject.cpp:135 #12 0x7fbe2c3af93a in WebCore::HTMLElement::createRenderer(WebCore::RenderArena*, WebCore::RenderStyle*) third_party/WebKit/Source/WebCore/html/HTMLElement.cpp:783 #13 0x7fbe2afc800c in WebCore::NodeRendererFactory::createRenderer() third_party/WebKit/Source/WebCore/dom/NodeRenderingContext.cpp:281 #14 0x7fbe2afc8dba in WebCore::NodeRendererFactory::createRendererIfNeeded() third_party/WebKit/Source/WebCore/dom/NodeRenderingContext.cpp:324 #15 0x7fbe2af20099 in WebCore::Node::createRendererIfNeeded() third_party/WebKit/Source/WebCore/dom/Node.cpp:1384 #16 0x7fbe2ad0e331 in WebCore::Element::attach() third_party/WebKit/Source/WebCore/dom/Element.cpp:954 #17 0x7fbe2a904710 in WebCore::Node::reattach() third_party/WebKit/Source/WebCore/dom/Node.h:868 #18 0x7fbe2ad11132 in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/Element.cpp:1086 #19 0x7fbe2aa72a2a in WebCore::Document::recalcStyle(WebCore::Node::StyleChange) third_party/WebKit/Source/WebCore/dom/Document.cpp:1848 #20 0x7fbe2aa64393 in WebCore::Document::styleResolverChanged(WebCore::StyleResolverUpdateFlag) third_party/WebKit/Source/WebCore/dom/Document.cpp:3383 #21 0x7fbe2aa8ec4c in WebCore::Document::removePendingSheet() third_party/WebKit/Source/WebCore/dom/Document.cpp:3335 #22 0x7fbe2b14be3d in WebCore::StyleElement::sheetLoaded(WebCore::Document*) third_party/WebKit/Source/WebCore/dom/StyleElement.cpp:201 #23 0x7fbe2c61f2c2 in WebCore::HTMLStyleElement::sheetLoaded() third_party/WebKit/Source/WebCore/html/HTMLStyleElement.h:70 #24 0x7fbe2fb583b2 in WebCore::StyleSheetContents::checkLoaded() third_party/WebKit/Source/WebCore/css/StyleSheetContents.cpp:343 #25 0x7fbe2b14b2ae in WebCore::StyleElement::createSheet(WebCore::Element*, WTF::OrdinalNumber, WTF::String const&) third_party/WebKit/Source/WebCore/dom/StyleElement.cpp:185 #26 0x7fbe2b148b55 in WebCore::StyleElement::process(WebCore::Element*) third_party/WebKit/Source/WebCore/dom/StyleElement.cpp:138 #27 0x7fbe2b14a00c in WebCore::StyleElement::finishParsingChildren(WebCore::Element*) third_party/WebKit/Source/WebCore/dom/StyleElement.cpp:109 #28 0x7fbe2c61b648 in WebCore::HTMLStyleElement::finishParsingChildren() third_party/WebKit/Source/WebCore/html/HTMLStyleElement.cpp:122 #29 0x7fbe2c9cd847 in WebCore::HTMLElementStack::popCommon() third_party/WebKit/Source/WebCore/html/parser/HTMLElementStack.cpp:578 #30 0x7fbe2c9ce0b6 in WebCore::HTMLElementStack::pop() third_party/WebKit/Source/WebCore/html/parser/HTMLElementStack.cpp:215 #31 0x7fbe2ca7b698 in WebCore::HTMLTreeBuilder::processEndOfFile(WebCore::AtomicHTMLToken*) third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:2497 #32 0x7fbe2ca717f2 in WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken*) third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:515 #33 0x7fbe2ca6f108 in WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken(WebCore::AtomicHTMLToken*) third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:473 #34 0x7fbe2ca6ec94 in WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken&) third_party/WebKit/Source/WebCore/html/parser/HTMLTreeBuilder.cpp:458 #35 0x7fbe2c9ae7a3 in WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:270 #36 0x7fbe2c9ad846 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:174 #37 0x7fbe2c9ad379 in WebCore::HTMLDocumentParser::prepareToStopParsing() third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:140 #38 0x7fbe2c9b0ecf in WebCore::HTMLDocumentParser::attemptToEnd() third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:394 #39 0x7fbe2c9b104a in WebCore::HTMLDocumentParser::finish() third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp:421 #40 0x7fbe308356bd in WebCore::DocumentWriter::end() third_party/WebKit/Source/WebCore/loader/DocumentWriter.cpp:242 #41 0x7fbe307bc62e in WebCore::DocumentLoader::finishedLoading() third_party/WebKit/Source/WebCore/loader/DocumentLoader.cpp:301 #42 0x7fbe3094ded3 in WebCore::MainResourceLoader::didFinishLoading(double) third_party/WebKit/Source/WebCore/loader/MainResourceLoader.cpp:521 #43 0x7fbe309dca58 in WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle*, double) third_party/WebKit/Source/WebCore/loader/ResourceLoader.cpp:437 #44 0x7fbe2d7d38e9 in WebCore::ResourceHandleInternal::didFinishLoading(WebKit::WebURLLoader*, double) third_party/WebKit/Source/WebCore/platform/network/chromium/ResourceHandle.cpp:157 #45 0x7fbe43464d2d in webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest(net::URLRequestStatus const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, base::TimeTicks const&) webkit/glue/weburlloader_impl.cc:667 #46 0x7fbe48731d7a in content::ResourceDispatcher::OnRequestComplete(int, net::URLRequestStatus const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, base::TimeTicks const&) content/common/resource_dispatcher.cc:473 #47 0x7fbe4873da86 in void DispatchToMethod<content::ResourceDispatcher, void (content::ResourceDispatcher::*)(int, net::URLRequestStatus const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, base::TimeTicks const&), int, net::URLRequestStatus, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, base::TimeTicks>(content::ResourceDispatcher*, void (content::ResourceDispatcher::*)(int, net::URLRequestStatus const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, base::TimeTicks const&), Tuple4<int, net::URLRequestStatus, std::basic_string<char, std::char_traits<char>, std::allocator<char> >, base::TimeTicks> const&) ./base/tuple.h:566 #48 0x7fbe4873ac70 in bool ResourceMsg_RequestComplete::Dispatch<content::ResourceDispatcher, content::ResourceDispatcher, void (content::ResourceDispatcher::*)(int, net::URLRequestStatus const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, base::TimeTicks const&)>(IPC::Message const*, content::ResourceDispatcher*, content::ResourceDispatcher*, void (content::ResourceDispatcher::*)(int, net::URLRequestStatus const&, std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, base::TimeTicks const&)) ./content/common/resource_messages.h:172 #49 0x7fbe4872a2bd in content::ResourceDispatcher::DispatchMessage(IPC::Message const&) content/common/resource_dispatcher.cc:543 #50 0x7fbe48727681 in content::ResourceDispatcher::OnMessageReceived(IPC::Message const&) content/common/resource_dispatcher.cc:311 #51 0x7fbe479444be in ChildThread::OnMessageReceived(IPC::Message const&) content/common/child_thread.cc:223 #52 0x7fbe455db7f3 in IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message const&) ipc/ipc_channel_proxy.cc:263 #53 0x7fbe456026b8 in base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(IPC::Message const&)>::Run(IPC::ChannelProxy::Context*, IPC::Message const&) ./base/bind_internal.h:190 #54 0x7fbe45602262 in base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(IPC::Message const&)>, void ()(IPC::ChannelProxy::Context* const&, IPC::Message const&)>::MakeItSo(base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(IPC::Message const&)>, IPC::ChannelProxy::Context* const&, IPC::Message const&) ./base/bind_internal.h:899 #55 0x7fbe45601d8d in base::internal::Invoker<2, base::internal::BindState<base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(IPC::Message const&)>, void ()(IPC::ChannelProxy::Context*, IPC::Message const&), void ()(IPC::ChannelProxy::Context*, IPC::Message)>, void ()(IPC::ChannelProxy::Context*, IPC::Message const&)>::Run(base::internal::BindStateBase*) ./base/bind_internal.h:1256 #56 0x7fbe56c13cf5 in base::Callback<void ()()>::Run() const ./base/callback.h:388 #57 0x7fbe56e32b4f in MessageLoop::RunTask(base::PendingTask const&) base/message_loop.cc:461 #58 0x7fbe56e34523 in MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) base/message_loop.cc:475 #59 0x7fbe56e34d48 in MessageLoop::DoWork() base/message_loop.cc:648 #60 0x7fbe56e8638e in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) base/message_pump_default.cc:28 #61 0x7fbe56e3139d in MessageLoop::RunInternal() base/message_loop.cc:420 #62 0x7fbe56e30eb3 in MessageLoop::RunHandler() base/message_loop.cc:393 #63 0x7fbe56ff36d4 in base::RunLoop::Run() base/run_loop.cc:46 Stats: 44M malloced (78M for red zones) by 271319 calls Stats: 1M realloced by 5069 calls Stats: 34M freed by 196789 calls Stats: 0M really freed by 0 calls Stats: 152M (38925 full pages) mmaped in 38 calls mmaps by size class: 8:262128; 9:16382; 10:20475; 11:4094; 12:2048; 13:512; 14:512; 15:128; 16:256; 17:32; 18:16; 20:4; mallocs by size class: 8:237153; 9:12051; 10:16534; 11:2671; 12:1828; 13:393; 14:383; 15:64; 16:212; 17:16; 18:13; 20:1; frees by size class: 8:168893; 9:7737; 10:15659; 11:2231; 12:1432; 13:272; 14:342; 15:26; 16:182; 17:6; 18:8; 20:1; rfrees by size class: Stats: malloc large: 30 small slow: 1012 This null crash is driving one of our fuzzers insane for a long time. Will be awesome to see it go away.

Attachments
Test patch for an early EWS run. (6.50 KB, patch) 2012-09-24 19:21 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Proposed fix: avoid triggering a style change if we only need a temporary style. (4.46 KB, patch) 2012-09-25 15:30 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Patch for landing (4.62 KB, patch) 2012-09-28 15:11 PDT, Julien Chaffraix	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Abhishek Arya

Comment 1 2012-09-15 09:25:09 PDT

From regression range, this looks to regress in https://trac.webkit.org/changeset/124168/. Julien, is this the same null scrollbar crash you were mentioning a few days back.

Julien Chaffraix

Comment 2 2012-09-15 19:04:37 PDT

(In reply to comment #1) > From regression range, this looks to regress in https://trac.webkit.org/changeset/124168/. It's definitely a regression from this change. > Julien, is this the same null scrollbar crash you were mentioning a few days back. Nope, the other crasher was bug 93903. This is another regression.

Julien Chaffraix

Comment 3 2012-09-24 19:21:38 PDT

Created attachment 165499 [details] Test patch for an early EWS run.

Julien Chaffraix

Comment 4 2012-09-25 15:30:31 PDT

Created attachment 165688 [details] Proposed fix: avoid triggering a style change if we only need a temporary style.

Abhishek Arya

Comment 5 2012-09-26 19:48:18 PDT

Comment on attachment 165688 [details] Proposed fix: avoid triggering a style change if we only need a temporary style. View in context: https://bugs.webkit.org/attachment.cgi?id=165688&action=review > Source/WebCore/rendering/RenderObject.cpp:135 > + // RenderImageResource requires a style being present on the image but we don't want to typo s/RenderImageResource/RenderImageResourceStyleImage. i actually debugged this to understand what is going on and can see the addClient call invoked through RenderImageResourceStyleImage::initialize. Also please add a comment on why this code with the "if (contentData && !contentData->next() && contentData->isImage() && doc != node)" not be moved to RenderImage::styleDidChange ? > LayoutTests/scrollbars/scrollbar-content-crash.html:12 > +function passed() { 'passed' sounds weird since the test might fail. How about something like runTest.

Abhishek Arya

Comment 6 2012-09-26 20:01:38 PDT

Comment on attachment 165688 [details] Proposed fix: avoid triggering a style change if we only need a temporary style. Actually I found a more cleaner approach and i think the bug is in RenderImage::imageChanged called by CachedImage::didAddClient. We should just bail out if we don't have a parent. See similar [http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/rendering/RenderBox.cpp&exact_package=chromium&q=RenderBox::imageChanged%20file:webcore&l=989] So, the fix should be to get rid of setStyle which is in the wrong place here. And bail out on no parent in imageChanged, since it is later called when the real setAnimatableStyle is called. RenderObject* NodeRendererFactory::createRenderer() { Node* node = m_context.node(); RenderObject* newRenderer = node->createRenderer(node->document()->renderArena(), m_context.style()); if (!newRenderer) ..... node->setRenderer(newRenderer); newRenderer->setAnimatableStyle(m_context.releaseStyle());

Julien Chaffraix

Comment 7 2012-09-27 13:11:12 PDT

> So, the fix should be to get rid of setStyle which is in the wrong place here. And bail out on no parent in imageChanged, since it is later called when the real setAnimatableStyle is called. It's OK for most RenderObject to bail out of imageChanged if you don't have a parent as they only trigger a repaint (which would be a no-op anyway). RenderImage is not among them as it uses the signal to recompute its intrinsic size. AFAICT there is no guarantee imageChanged will be called asynchronously after you have been inserted into the tree so this approach makes at least 2 tests flaky (fast/css/contentDiv.html and fast/css/contentImage.html). RenderObjectChildList has a similar code to handle image content and also calls setStyle before calling setImageResource which makes me believe it's needed. What's not needed is triggering a style change notification in RenderObject::createObject.

Julien Chaffraix

Comment 8 2012-09-28 15:11:54 PDT

Created attachment 166321 [details] Patch for landing

WebKit Review Bot

Comment 9 2012-09-28 15:42:42 PDT

Comment on attachment 166321 [details] Patch for landing Clearing flags on attachment: 166321 Committed r129955: <http://trac.webkit.org/changeset/129955>

WebKit Review Bot

Comment 10 2012-09-28 15:42:45 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.