Bug 205290 – Nullptr crash in WebCore::findPlaceForCounter with display: contents parent

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jack]

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

[Jack]

Created attachment 385790 [details]
Test HTML

[Jack]

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]

Created attachment 385819 [details]
Patch

[Jack]

Created attachment 385924 [details]
Patch

[Ryosuke Niwa]

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]

Created attachment 386017 [details]
Patch

[Ryosuke Niwa]

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 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
> +<p> Bug <a href="https://bugs.webkit.org/show_bug.cgi?id=205290">205290</a>: Crash in WebCore::findPlaceForCounter</p>
> +<p> This test PASSES if it does not CRASH or ASSERT.</p>

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]

Created attachment 386075 [details]
Patch

[Ryosuke Niwa]

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
> +<p> 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.</p>

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]

Created attachment 386077 [details]
Patch

[Ryosuke Niwa]

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
> +<p> This tests an element with CSS counter having a parent with display: contents.</p>

And get rid of this leading whitespace.

[Ryosuke Niwa]

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