Summary: | Nullptr crash in CSSValue::cssText() via DeleteSelectionCommand::calculateTypingStyleAfterDelete | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||
Component: | HTML Editing | Assignee: | Rob Buis <rbuis> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | bfulgham, cgarcia, ews-feeder, ews-watchlist, fred.wang, gpoo, koivisto, mifenton, product-security, rbuis, svillar, webkit-bug-importer, wenson_hsieh | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=230047 | ||||||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2021-08-18 21:17:40 PDT
I can reproduce this with ASAN release build of WebKitTestRunner at r281219. Created attachment 435881 [details]
Patch
Created attachment 436274 [details]
Patch
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? Created attachment 436378 [details]
Patch
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. Created attachment 436401 [details]
Patch
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. Why is the test timing out on iOS?? 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. (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... Created attachment 437398 [details]
Patch
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]. 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 (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 (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. (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. 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 @wenson_hsieh do you know how r282090 could change the execution order of executeDelete and executeForwardDelete? (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? (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() |