Bug 205290

Summary:

Nullptr crash in WebCore::findPlaceForCounter with display: contents parent

Product:

WebKit

Reporter:

Jack <shihchieh_lee>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, esprehn+autocc, ews-watchlist, glenn, kondapallykalyan, pdr, rniwa, simon.fraser, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Test HTML	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	rniwa: review+

Jack

Reported 2019-12-16 11:26:34 PST

rdar://problem/56730730 : Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00007fff42f1e7dc WebCore::findPlaceForCounter(WebCore::RenderElement&, WTF::AtomString const&, bool) + 1164 1 com.apple.WebCore 0x00007fff42f00e7b WebCore::makeCounterNode(WebCore::RenderElement&, WTF::AtomString const&, bool) + 1435 2 com.apple.WebCore 0x00007fff42f1e42f WebCore::findPlaceForCounter(WebCore::RenderElement&, WTF::AtomString const&, bool) + 223 3 com.apple.WebCore 0x00007fff42f00e7b WebCore::makeCounterNode(WebCore::RenderElement&, WTF::AtomString const&, bool) + 1435 4 com.apple.WebCore 0x00007fff42f1e42f WebCore::findPlaceForCounter(WebCore::RenderElement&, WTF::AtomString const&, bool) + 223 5 com.apple.WebCore 0x00007fff42f00e7b WebCore::makeCounterNode(WebCore::RenderElement&, WTF::AtomString const&, bool) + 1435 6 com.apple.WebCore 0x00007fff42f1e42f WebCore::findPlaceForCounter(WebCore::RenderElement&, WTF::AtomString const&, bool) + 223 7 com.apple.WebCore 0x00007fff42f00e7b WebCore::makeCounterNode(WebCore::RenderElement&, WTF::AtomString const&, bool) + 1435 8 com.apple.WebCore 0x00007fff42f08945 WebCore::RenderElement::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) + 2053 9 com.apple.WebCore 0x00007fff414aac2f WebCore::RenderLayerModelObject::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) + 31 10 com.apple.WebCore 0x00007fff414a9c7a WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) + 42 11 com.apple.WebCore 0x00007fff414a972e WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) + 94 12 com.apple.WebCore 0x00007fff42ec171c WebCore::RenderBlockFlow::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) + 28 13 com.apple.WebCore 0x00007fff42f06a61 WebCore::RenderElement::setStyle(WebCore::RenderStyle&&, WebCore::StyleDifference) + 225 14 com.apple.WebCore 0x00007fff43093129 WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdate const&) + 1609 15 com.apple.WebCore 0x00007fff43091523 WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) + 2403 16 com.apple.WebCore 0x00007fff427044a1 WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 1441 17 com.apple.WebCore 0x00007fff414bdf5a WebCore::Document::updateStyleIfNeeded() + 362 18 com.apple.WebCore 0x00007fff4153f1a8 WebCore::Document::updateLayout() + 216 19 com.apple.WebCore 0x00007fff42972365 WebCore::HTMLObjectElement::renderWidgetLoadingPlugin() const + 101 20 com.apple.WebCore 0x00007fff415c78cf WebCore::pluginScriptObject(JSC::ExecState*, WebCore::JSHTMLElement*) + 255 21 com.apple.WebCore 0x00007fff4162c839 WebCore::JSHTMLObjectElement::put(JSC::JSCell*, JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) + 41 22 com.apple.JavaScriptCore 0x00007fff36347df9 llint_slow_path_put_by_val + 1417 23 com.apple.JavaScriptCore 0x00007fff364f4838 llint_entry + 45951 24 com.apple.JavaScriptCore 0x00007fff365000e3 llint_entry + 93226 25 com.apple.JavaScriptCore 0x00007fff364e930f vmEntryToJavaScript + 200 26 com.apple.JavaScriptCore 0x00007fff361d6050 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 416 27 com.apple.JavaScriptCore 0x00007fff36c7fc9b JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 187 28 com.apple.WebCore 0x00007fff42492fc4 WebCore::JSExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 100 29 com.apple.WebCore 0x00007fff424ab29b WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) + 1403 30 com.apple.WebCore 0x00007fff42757699 WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul>, WebCore::EventTarget::EventInvokePhase) + 1113 31 com.apple.WebCore 0x00007fff427551e3 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) + 611 32 com.apple.WebCore 0x00007fff42753d1e WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&) (.llvm.1484162973191472938) + 206 33 com.apple.WebCore 0x00007fff42753812 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) + 1090 34 com.apple.WebCore 0x00007fff41525572 WebCore::HTMLStyleElement::dispatchPendingEvent(WebCore::EventSender<WebCore::HTMLStyleElement>*) + 178 35 com.apple.WebCore 0x00007fff414facb6 WebCore::EventSender<WebCore::HTMLStyleElement>::dispatchPendingEvents() + 134 36 com.apple.WebCore 0x00007fff42ceb0f8 WTF::Detail::CallableWrapper<WebCore::ThreadTimers::setSharedTimer(WebCore::SharedTimer*)::$_0, void>::call() + 184 37 com.apple.WebCore 0x00007fff41493b9f WebCore::timerFired(__CFRunLoopTimer*, void*) + 31 38 com.apple.CoreFoundation 0x00007fff328da326 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20 39 com.apple.CoreFoundation 0x00007fff328d9ee0 __CFRunLoopDoTimer + 872 40 com.apple.CoreFoundation 0x00007fff328d98f3 __CFRunLoopDoTimers + 317 41 com.apple.CoreFoundation 0x00007fff328baf40 __CFRunLoopRun + 2227 42 com.apple.CoreFoundation 0x00007fff328ba418 CFRunLoopRunSpecific + 503 43 com.apple.Foundation 0x00007fff34ff7e0d -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212 44 parseWebKit 0x000000010bad6988 main + 4104 45 0x00007fff69c312f5 start + 1

Attachments
Test HTML (209 bytes, text/html) 2019-12-16 11:28 PST, Jack	no flags	Details
Patch (3.63 KB, patch) 2019-12-16 15:33 PST, Jack	no flags	Details Formatted Diff Diff
Patch (3.56 KB, patch) 2019-12-17 15:44 PST, Jack	no flags	Details Formatted Diff Diff
Patch (3.65 KB, patch) 2019-12-18 15:18 PST, Jack	no flags	Details Formatted Diff Diff
Patch (3.82 KB, patch) 2019-12-18 23:13 PST, Jack	no flags	Details Formatted Diff Diff
Patch (3.74 KB, patch) 2019-12-18 23:57 PST, Jack	rniwa: review+	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Jack

Comment 1 2019-12-16 11:28:13 PST

Created attachment 385790 [details] Test HTML

Jack

Comment 2 2019-12-16 15:20:50 PST

Cause of the crash: Function parentOrPseudoHostElement() finds an element that does not contain renderer, causing the caller (findPlaceForCounter) to use nullptr as current render and try to dereference it. Fix: In function parentOrPseudoHostElement(), search the whole renderer tree for an element that has a parent with "hasDisplayContents" set to false.

Jack

Comment 3 2019-12-16 15:33:20 PST

Created attachment 385819 [details] Patch

Jack

Comment 4 2019-12-17 15:44:59 PST

Created attachment 385924 [details] Patch

Ryosuke Niwa

Comment 5 2019-12-17 17:52:27 PST

Comment on attachment 385924 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385924&action=review > LayoutTests/ChangeLog:9 > + * fast/css/counters/findPlaceForCounter-crash-expected.txt: Added. > + * fast/css/counters/findPlaceForCounter-crash.html: Added. We don't typically use camel case for the test name. Also, we shouldn't name a function after C++ function. How about counter-with-display-contents-parent. > LayoutTests/fast/css/counters/findPlaceForCounter-crash.html:5 > +onload = function eventhandler1() { Please call testRunner.dumpAsText.

Jack

Comment 6 2019-12-18 15:18:55 PST

Created attachment 386017 [details] Patch

Ryosuke Niwa

Comment 7 2019-12-18 20:54:33 PST

Comment on attachment 386017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386017&action=review > Source/WebCore/rendering/RenderCounter.cpp:77 > + Element* parent = renderer.element()->parentElement(); Let's use RefPtr here.

Ryosuke Niwa

Comment 8 2019-12-18 20:57:11 PST

Comment on attachment 386017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386017&action=review > Source/WebCore/ChangeLog:3 > + Null Ptr Deref in WebCore::findPlaceForCounter Please update this line to match the bug title. > LayoutTests/ChangeLog:3 > + Null Ptr Deref in WebCore::findPlaceForCounter Ditto. > LayoutTests/fast/css/counters/findPlaceForCounter-crash.html:13 > + Bug <a href="https://bugs.webkit.org/show_bug.cgi?id=205290">205290</a>: Crash in WebCore::findPlaceForCounter > + This test PASSES if it does not CRASH or ASSERT. I don't think where the crash used to happen would be a useful information. We should just say: This tests an element with CSS counter having a parent with display: contents. The test passes if WebKit doesn't crash or hit an assertion.

Jack

Comment 9 2019-12-18 23:13:04 PST

Created attachment 386075 [details] Patch

Ryosuke Niwa

Comment 10 2019-12-18 23:21:18 PST

Comment on attachment 386075 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386075&action=review > LayoutTests/fast/css/counters/findPlaceForCounter-crash.html:12 > + Bug <a href="https://bugs.webkit.org/show_bug.cgi?id=205290">205290</a>: This tests an element with CSS counter having a parent with display: contents. I don't think we need a bug link. We know where we came from based on the svn version history / trac. At least use a shorter URL form: https://webkit.org/b/205290.

Jack

Comment 11 2019-12-18 23:57:10 PST

Created attachment 386077 [details] Patch

Ryosuke Niwa

Comment 12 2019-12-19 15:18:48 PST

Comment on attachment 386077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386077&action=review Landing this manually to fix the following issues. > LayoutTests/fast/css/counters/findPlaceForCounter-crash.html:5 > + if (window.testRunner) We should dedent this. > LayoutTests/fast/css/counters/findPlaceForCounter-crash.html:12 > + This tests an element with CSS counter having a parent with display: contents. And get rid of this leading whitespace.

Ryosuke Niwa

Comment 13 2019-12-19 18:00:58 PST

Committed r253805: <https://trac.webkit.org/changeset/253805>

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