Bug 229281 – Nullptr crash in CSSValue::cssText() via DeleteSelectionCommand::calculateTypingStyleAfterDelete

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 435841 [details]
Test

e.g.

#0 0x37d31b422 in WebCore::CSSValue::classType() const+0x22 (WebCore.framework/Versions/A/WebCore:x86_64+0x329d422)
#1 0x37d388674 in WebCore::CSSValue::cssText() const+0x14 (WebCore.framework/Versions/A/WebCore:x86_64+0x330a674)
#2 0x37da8f929 in WebCore::EditingStyle::init(WebCore::Node*, WebCore::EditingStyle::PropertiesToInclude)+0x599 (WebCore.framework/Versions/A/WebCore:x86_64+0x3a11929)
#3 0x37da8fbed in WebCore::EditingStyle::EditingStyle(WebCore::Position const&, WebCore::EditingStyle::PropertiesToInclude)+0x2d (WebCore.framework/Versions/A/WebCore:x86_64+0x3a11bed)
#4 0x37da6201e in WebCore::EditingStyle::create(WebCore::Position const&, WebCore::EditingStyle::PropertiesToInclude)+0x2e (WebCore.framework/Versions/A/WebCore:x86_64+0x39e401e)
#5 0x37da81d45 in WebCore::EditingStyle::prepareToApplyAt(WebCore::Position const&, WebCore::EditingStyle::ShouldPreserveWritingDirection)+0x125 (WebCore.framework/Versions/A/WebCore:x86_64+0x3a03d45)
#6 0x37da81aeb in WebCore::DeleteSelectionCommand::calculateTypingStyleAfterDelete()+0x14b (WebCore.framework/Versions/A/WebCore:x86_64+0x3a03aeb)
#7 0x37da83f41 in WebCore::DeleteSelectionCommand::doApply()+0xbd1 (WebCore.framework/Versions/A/WebCore:x86_64+0x3a05f41)
#8 0x37da575fa in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&)+0x6a (WebCore.framework/Versions/A/WebCore:x86_64+0x39d95fa)
#9 0x37da5ac6a in WebCore::CompositeEditCommand::deleteSelection(WebCore::VisibleSelection const&, bool, bool, bool, bool, bool)+0x12a (WebCore.framework/Versions/A/WebCore:x86_64+0x39dcc6a)
#10 0x37db8ec88 in WebCore::TypingCommand::forwardDeleteKeyPressed(WebCore::TextGranularity, bool)+0xfd8 (WebCore.framework/Versions/A/WebCore:x86_64+0x3b10c88)
#11 0x37db90ec2 in WebCore::TypingCommand::doApply()+0x132 (WebCore.framework/Versions/A/WebCore:x86_64+0x3b12ec2)
#12 0x37da353c6 in WebCore::CompositeEditCommand::apply()+0x216 (WebCore.framework/Versions/A/WebCore:x86_64+0x39b73c6)
#13 0x37db8dbeb in WebCore::TypingCommand::forwardDeleteKeyPressed(WebCore::Document&, unsigned int, WebCore::TextGranularity)+0x29b (WebCore.framework/Versions/A/WebCore:x86_64+0x3b0fbeb)
#14 0x37db06e7c in WebCore::executeForwardDelete(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)+0x1c (WebCore.framework/Versions/A/WebCore:x86_64+0x3a88e7c)
#15 0x37dac808b in WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const+0xdb (WebCore.framework/Versions/A/WebCore:x86_64+0x3a4a08b)
#16 0x37d72dc7e in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)+0x14e (WebCore.framework/Versions/A/WebCore:x86_64+0x36afc7e)
#17 0x37ab2d8c8 in WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)+0x4f8 (WebCore.framework/Versions/A/WebCore:x86_64+0xaaf8c8)

<rdar://80077558>

[Ryosuke Niwa]

I can reproduce this with ASAN release build of WebKitTestRunner at r281219.

[Rob Buis]

Created attachment 435881 [details]
Patch

[Rob Buis]

Created attachment 436274 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 436274 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Null check the CSSValue in EditingStyle::init

We should match the actual bug title now that we know this isn't a security bug.

> LayoutTests/ChangeLog:3
> +        Null check the CSSValue in EditingStyle::init

Ditto.

> LayoutTests/editing/deleting/forward-delete-crash.html:17
> +    document.designMode = 'on';

Can we simplify this code more? Do we really need all these elements?

[Rob Buis]

Created attachment 436378 [details]
Patch

[Rob Buis]

Comment on attachment 436274 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Null check the CSSValue in EditingStyle::init
> 
> We should match the actual bug title now that we know this isn't a security bug.

Done.

>> LayoutTests/ChangeLog:3
>> +        Null check the CSSValue in EditingStyle::init
> 
> Ditto.

Done.

>> LayoutTests/editing/deleting/forward-delete-crash.html:17
>> +    document.designMode = 'on';
> 
> Can we simplify this code more? Do we really need all these elements?

It is needed, except the document.body.offsetTop; line, which I removed.

[Rob Buis]

Created attachment 436401 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 436401 [details]
Patch

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

> LayoutTests/ChangeLog:9
> +        * editing/deleting/forward-delete-crash-expected.txt: Added.
> +        * editing/deleting/forward-delete-crash.html: Added.

This is a very generic name. We should probably call it something like forward-delete-calculate-typing-style-crash.html or something.

> LayoutTests/editing/deleting/forward-delete-crash.html:10
> +<style>
> +  :last-of-type {
> +    height: 1px;
> +    display: block;
> +  }
> +@font-face {
> +    font-family: "Ahem";
> +    src: url("../../resources/Ahem.ttf");
> +}
> +</style>

Please indent consistency.

> LayoutTests/editing/deleting/forward-delete-crash.html:12
> +<script>
> +  if (window.testRunner) {

Ditto for the rest of the file.

> LayoutTests/editing/deleting/forward-delete-crash.html:32
> +    if (window.caches)
> +      await caches.has('a');

What is this about?

> LayoutTests/editing/deleting/forward-delete-crash.html:38
> +    if (window.caches)
> +      await caches.has('a');

Ditto.

[Ryosuke Niwa]

Why is the test timing out on iOS??

[Rob Buis]

Comment on attachment 436401 [details]
Patch

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

>> LayoutTests/editing/deleting/forward-delete-crash.html:32
>> +      await caches.has('a');
> 
> What is this about?

This bug seems timing dependant. For WK1 I think this API is not even available, so I use document.fonts workaround instead, but sadly that one does not work for WK2, so we end up with the test case as uploaded.

[Rob Buis]

(In reply to Ryosuke Niwa from comment #9)
> Why is the test timing out on iOS??

I am not sure! I do not even know if it is something special about editing on iOS or something to do with Cache API...

[Rob Buis]

Created attachment 437398 [details]
Patch

[EWS]

Committed r282074 (241374@main): <https://commits.webkit.org/241374@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437398 [details].

[Ryosuke Niwa]

Looks like test aded in this patch has been failing on the bots:
e.g.
https://build.webkit.org/results/Apple-BigSur-Debug-WK1-Tests/r282140%20(4040)/results.html

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #14)
> Looks like test aded in this patch has been failing on the bots:
> e.g.
> https://build.webkit.org/results/Apple-BigSur-Debug-WK1-Tests/
> r282140%20(4040)/results.html

Tracked by https://bugs.webkit.org/show_bug.cgi?id=230047

[Rob Buis]

(In reply to Ryosuke Niwa from comment #14)
> Looks like test aded in this patch has been failing on the bots:
> e.g.
> https://build.webkit.org/results/Apple-BigSur-Debug-WK1-Tests/
> r282140%20(4040)/results.html

Should we make the test Mac-wk2 only? It seems highly timing dependent, i.e. it seems slower Debug-wk1 can even cause different timing to not trigger the right code paths, unlike Release-wk1.

[Ryosuke Niwa]

(In reply to Rob Buis from comment #16)
> (In reply to Ryosuke Niwa from comment #14)
> > Looks like test aded in this patch has been failing on the bots:
> > e.g.
> > https://build.webkit.org/results/Apple-BigSur-Debug-WK1-Tests/
> > r282140%20(4040)/results.html
> 
> Should we make the test Mac-wk2 only? It seems highly timing dependent, i.e.
> it seems slower Debug-wk1 can even cause different timing to not trigger the
> right code paths, unlike Release-wk1.

It looks like it's also timing out on Mac-wk2. We need to make the test less time dependent. Please make further followups on https://bugs.webkit.org/show_bug.cgi?id=230047 instead.

[Rob Buis]

Behaviour is different with r282090:
scheduleFullEditorStateUpdate
executeDelete
executeDelete done
executeForwardDelete
scheduleFullEditorStateUpdate
executeForwardDelete done

Old behaviour with crash before r282090:
scheduleFullEditorStateUpdate
executeForwardDelete
executeDelete
scheduleFullEditorStateUpdate
executeDelete done
AddressSanitizer:DEADLYSIGNAL

[Rob Buis]

@wenson_hsieh do you know how r282090 could change the execution order of executeDelete and executeForwardDelete?

[Wenson Hsieh]

(In reply to Rob Buis from comment #19)
> @wenson_hsieh do you know how r282090 could change the execution order of
> executeDelete and executeForwardDelete?

Hm..it's not obvious to me why that would cause a difference here.

Out of curiosity, what are the call stacks to executing `executeDelete` and `executeForwardDelete` after r282090?

[Rob Buis]

(In reply to Wenson Hsieh from comment #20)
> (In reply to Rob Buis from comment #19)
> > @wenson_hsieh do you know how r282090 could change the execution order of
> > executeDelete and executeForwardDelete?
> 
> Hm..it's not obvious to me why that would cause a difference here.
> 
> Out of curiosity, what are the call stacks to executing `executeDelete` and
> `executeForwardDelete` after r282090?

Sorry for the small number of frames, with ToT (so after r282090) executeDelete is called first:
1   0x7a40cf784 WebCore::executeDelete(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
2   0x7a405f6e0 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
3   0x7a3889751 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
4   0x79d342fca WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)
5   0x79d34210b long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*)
6   0x79d30a064 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*)
7   0x5c6326c011d8
8   0x7d97917c1 llint_entry
9   0x7d9791711 llint_entry
10  0x7d9791711 llint_entry
11  0x7d97917c1 llint_entry
12  0x7d9791711 llint_entry
13  0x7d976dfb2 vmEntryToJavaScript
14  0x7dc353e07 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
15  0x7dc3550f6 JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
16  0x7dcdf93b0 JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
17  0x7dcdfa070 JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
18  0x7dd4e71bd JSC::JSMicrotask::run(JSC::JSGlobalObject*)
19  0x7a275a042 WebCore::JSExecState::runTask(JSC::JSGlobalObject*, JSC::Microtask&)
20  0x7a276a7c2 WebCore::JSMicrotaskCallback::call()
21  0x7a276a412 WebCore::JSDOMWindowBase::queueMicrotaskToEventLoop(JSC::JSGlobalObject&, WTF::Ref<JSC::Microtask, WTF::RawPtrTraits<JSC::Microtask> >&&)::$_40::operator()()
22  0x7a276a1ee WTF::Detail::CallableWrapper<WebCore::JSDOMWindowBase::queueMicrotaskToEventLoop(JSC::JSGlobalObject&, WTF::Ref<JSC::Microtask, WTF::RawPtrTraits<JSC::Microtask> >&&)::$_40, void>::call()
23  0x79b86c0a5 WTF::Function<void ()>::operator()() const
24  0x7a3b1442e WebCore::EventLoopFunctionDispatchTask::execute()
25  0x7a3bbbf54 WebCore::MicrotaskQueue::performMicrotaskCheckpoint()
26  0x7a3aff849 WebCore::EventLoop::run()
27  0x7a3edcb82 WebCore::WindowEventLoop::didReachTimeToRun()
28  0x7a3ee55f2 decltype(*(std::__1::forward<WebCore::WindowEventLoop*&>(fp0)).*fp()) std::__1::__invoke<void (WebCore::WindowEventLoop::*&)(), WebCore::WindowEventLoop*&, void>(void (WebCore::WindowEventLoop::*&)(), WebCore::WindowEventLoop*&)
29  0x7a3ee54a0 std::__1::__bind_return<void (WebCore::WindowEventLoop::*)(), std::__1::tuple<WebCore::WindowEventLoop*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::WindowEventLoop::*)(), std::__1::tuple<WebCore::WindowEventLoop*>, std::__1::tuple<> >::value>::type std::__1::__apply_functor<void (WebCore::WindowEventLoop::*)(), std::__1::tuple<WebCore::WindowEventLoop*>, 0ul, std::__1::tuple<> >(void (WebCore::WindowEventLoop::*&)(), std::__1::tuple<WebCore::WindowEventLoop*>&, std::__1::__tuple_indices<0ul>, std::__1::tuple<>&&)
30  0x7a3ee53e3 std::__1::__bind_return<void (WebCore::WindowEventLoop::*)(), std::__1::tuple<WebCore::WindowEventLoop*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::WindowEventLoop::*)(), std::__1::tuple<WebCore::WindowEventLoop*>, std::__1::tuple<> >::value>::type std::__1::__bind<void (WebCore::WindowEventLoop::*&)(), WebCore::WindowEventLoop*>::operator()<>()
31  0x7a3ee529e WTF::Detail::CallableWrapper<std::__1::__bind<void (WebCore::WindowEventLoop::*&)(), WebCore::WindowEventLoop*>, void>::call()

And executeForwardDelete later:
1   0x7a40d11dc WebCore::executeForwardDelete(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
2   0x7a405f6e0 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
3   0x7a3889751 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
4   0x79d342fca WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)
5   0x79d34210b long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*)
6   0x79d30a064 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*)
7   0x5c6326c011d8
8   0x7d97917c1 llint_entry
9   0x7d976dfb2 vmEntryToJavaScript
10  0x7dc353e07 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
11  0x7dc3550f6 JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
12  0x7dcdf93b0 JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
13  0x7dcdf98c2 JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
14  0x7dcdfa56a JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
15  0x7a261f97e WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
16  0x7a2671fe0 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)
17  0x7a3b0ce53 WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::RawPtrTraits<WebCore::RegisteredEventListener>, WTF::DefaultRefDerefTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase)
18  0x7a3b0c144 WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase)
19  0x7a3aa1f69 WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const
20  0x7a3af93f5 WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&)
21  0x7a3af8665 WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&)
22  0x7a3c6524d WebCore::Node::dispatchEvent(WebCore::Event&)
23  0x7a45fba14 WebCore::HTMLImageLoader::dispatchLoadEvent()
24  0x7a59dfefd WebCore::ImageLoader::dispatchPendingLoadEvent()
25  0x7a59dfd0d WebCore::ImageLoader::dispatchPendingEvent(WebCore::EventSender<WebCore::ImageLoader>*)
26  0x7a59e04eb WebCore::EventSender<WebCore::ImageLoader>::dispatchPendingEvents(WebCore::Page*)
27  0x7a59efc09 WebCore::EventSender<WebCore::ImageLoader>::timerFired()
28  0x7a59f0dc2 decltype(*(std::__1::forward<WebCore::EventSender<WebCore::ImageLoader>*&>(fp0)).*fp()) std::__1::__invoke<void (WebCore::EventSender<WebCore::ImageLoader>::*&)(), WebCore::EventSender<WebCore::ImageLoader>*&, void>(void (WebCore::EventSender<WebCore::ImageLoader>::*&)(), WebCore::EventSender<WebCore::ImageLoader>*&)
29  0x7a59f0c70 std::__1::__bind_return<void (WebCore::EventSender<WebCore::ImageLoader>::*)(), std::__1::tuple<WebCore::EventSender<WebCore::ImageLoader>*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::EventSender<WebCore::ImageLoader>::*)(), std::__1::tuple<WebCore::EventSender<WebCore::ImageLoader>*>, std::__1::tuple<> >::value>::type std::__1::__apply_functor<void (WebCore::EventSender<WebCore::ImageLoader>::*)(), std::__1::tuple<WebCore::EventSender<WebCore::ImageLoader>*>, 0ul, std::__1::tuple<> >(void (WebCore::EventSender<WebCore::ImageLoader>::*&)(), std::__1::tuple<WebCore::EventSender<WebCore::ImageLoader>*>&, std::__1::__tuple_indices<0ul>, std::__1::tuple<>&&)
30  0x7a59f0bb3 std::__1::__bind_return<void (WebCore::EventSender<WebCore::ImageLoader>::*)(), std::__1::tuple<WebCore::EventSender<WebCore::ImageLoader>*>, std::__1::tuple<>, __is_valid_bind_return<void (WebCore::EventSender<WebCore::ImageLoader>::*)(), std::__1::tuple<WebCore::EventSender<WebCore::ImageLoader>*>, std::__1::tuple<> >::value>::type std::__1::__bind<void (WebCore::EventSender<WebCore::ImageLoader>::*&)(), WebCore::EventSender<WebCore::ImageLoader>*>::operator()<>()
31  0x7a59f0a6e WTF::Detail::CallableWrapper<std::__1::__bind<void (WebCore::EventSender<WebCore::ImageLoader>::*&)(), WebCore::EventSender<WebCore::ImageLoader>*>, void>::call()