Bug 208311

Summary: Crash in RenderStyle::hasExplicitlySetDirection
Product: WebKit Reporter: Ali Juma <ajuma>
Component: DOMAssignee: Eugene But <eugenebut>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, darin, esprehn+autocc, eugenebut, ews-feeder, ews-watchlist, kangil.han, product-security, rniwa, rohitrao, sdefresne, shihchieh_lee, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Minimal test case
none
Patch
none
Minimized test case
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Ali Juma
Reported 2020-02-27 07:52:09 PST
Created attachment 391868 [details] Minimal test case Filing this as a security bug since it was found using a fuzzer; there's no disclosure deadline for this bug. Crash stack: ================================================================= ==46072==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000070 (pc 0x0004b836e7a1 bp 0x7ffee4169930 sp 0x7ffee4169930 T0) ==46072==The signal is caused by a READ memory access. ==46072==Hint: address points to the zero page. ==46072==WARNING: invalid path to external symbolizer! ==46072==WARNING: Failed to use and restart external symbolizer! #0 0x4b836e7a0 in WebCore::RenderStyle::hasExplicitlySetDirection() const (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x47f47a0) #1 0x4b8305cc4 in WebCore::RenderBox::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x478bcc4) #2 0x4b8305849 in WebCore::RenderBlock::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x478b849) #3 0x4b83598b6 in WebCore::RenderBlockFlow::styleDidChange(WebCore::StyleDifference, WebCore::RenderStyle const*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x47df8b6) #4 0x4b88de674 in WebCore::RenderTreeUpdater::createRenderer(WebCore::Element&, WebCore::RenderStyle&&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x4d64674) #5 0x4b88dc72a in WebCore::RenderTreeUpdater::updateElementRenderer(WebCore::Element&, WebCore::Style::ElementUpdate const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x4d6272a) #6 0x4b88dbdd1 in WebCore::RenderTreeUpdater::updateRenderTree(WebCore::ContainerNode&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x4d61dd1) #7 0x4b88db40b in WebCore::RenderTreeUpdater::commit(std::__1::unique_ptr<WebCore::Style::Update const, std::__1::default_delete<WebCore::Style::Update const> >) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x4d6140b) #8 0x4b6aad4bd in WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2f334bd) #9 0x4b6aae280 in WebCore::Document::updateStyleIfNeeded() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2f34280) #10 0x4b6aa72c3 in WebCore::Document::updateLayout() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2f2d2c3) #11 0x4b6aa9472 in WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2f2f472) #12 0x4b6ecfe0a in WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3355e0a) #13 0x4b6ecfb90 in WebCore::VisiblePosition::init(WebCore::Position const&, WebCore::EAffinity) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3355b90) #14 0x4b6ed70a5 in WebCore::VisibleSelection::setBaseAndExtentToDeepEquivalents() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x335d0a5) #15 0x4b6ed5972 in WebCore::VisibleSelection::validate(WebCore::TextGranularity) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x335b972) #16 0x4b6e122c5 in WebCore::FrameSelection::moveTo(WebCore::Position const&, WebCore::EAffinity, WebCore::EUserTriggered) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32982c5) #17 0x4b79549ee in WebCore::DOMSelection::collapseToEnd() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3dda9ee) #18 0x4b44b6435 in WebCore::jsDOMSelectionPrototypeFunctionCollapseToEndBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMSelection*, JSC::ThrowScope&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x93c435) #19 0x4b4338330 in long long WebCore::IDLOperation<WebCore::JSDOMSelection>::call<&(WebCore::jsDOMSelectionPrototypeFunctionCollapseToEndBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMSelection*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x7be330) #20 0x49e53c601177 (<unknown module>) #21 0x49e57c600427 (<unknown module>) #22 0x4ce6c43d8 in vmEntryToJavaScript (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa753d8) #23 0x4cfcec937 in JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x209d937) #24 0x4d0318140 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c9140) #25 0x4d0318242 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c9242) #26 0x4d031861f in JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c961f) #27 0x4b641401b in WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x289a01b) #28 0x4b643d071 in WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x28c3071) #29 0x4b6bdf35b in WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x306535b) #30 0x4b6bda6f2 in WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x30606f2) #31 0x4b6bc765d in WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x304d65d) #32 0x4b6bc8529 in WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x304e529) #33 0x4b6bc7fad in WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x304dfad) #34 0x4b6bc7a6d in WebCore::EventDispatcher::dispatchScopedEvent(WebCore::Node&, WebCore::Event&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x304da6d) #35 0x4b6c61297 in WebCore::Node::dispatchSubtreeModifiedEvent() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x30e7297) #36 0x4b6a4d018 in WebCore::ContainerNode::replaceChild(WebCore::Node&, WebCore::Node&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2ed3018) #37 0x4b6c5455e in WebCore::Node::replaceChild(WebCore::Node&, WebCore::Node&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x30da55e) #38 0x4b4c8c2e4 in WebCore::jsNodePrototypeFunctionReplaceChildBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSNode*, JSC::ThrowScope&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x11122e4) #39 0x4b4c09c85 in long long WebCore::IDLOperation<WebCore::JSNode>::call<&(WebCore::jsNodePrototypeFunctionReplaceChildBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSNode*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x108fc85) #40 0x49e53c601177 (<unknown module>) #41 0x49e57c6005c5 (<unknown module>) #42 0x4ce6c43d8 in vmEntryToJavaScript (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa753d8) #43 0x4cfcec937 in JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x209d937) #44 0x4d0318140 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c9140) #45 0x4d0318242 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c9242) #46 0x4d031861f in JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c961f) #47 0x4b641401b in WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x289a01b) #48 0x4b643d071 in WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x28c3071) #49 0x4b6bdf35b in WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x306535b) #50 0x4b6bda6f2 in WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x30606f2) #51 0x4b6bc765d in WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x304d65d) #52 0x4b6bc8529 in WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x304e529) #53 0x4b6bc7fad in WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x304dfad) #54 0x4b6bc7a6d in WebCore::EventDispatcher::dispatchScopedEvent(WebCore::Node&, WebCore::Event&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x304da6d) #55 0x4b6c61297 in WebCore::Node::dispatchSubtreeModifiedEvent() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x30e7297) #56 0x4b6a4b537 in WebCore::ContainerNode::appendChildWithoutPreInsertionValidityCheck(WebCore::Node&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2ed1537) #57 0x4b6a4f874 in WebCore::ContainerNode::appendChild(WebCore::Node&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2ed5874) #58 0x4b6c548f4 in WebCore::Node::appendChild(WebCore::Node&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x30da8f4) #59 0x4b4c8bcab in WebCore::jsNodePrototypeFunctionAppendChildBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSNode*, JSC::ThrowScope&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x1111cab) #60 0x4b4c09ac5 in long long WebCore::IDLOperation<WebCore::JSNode>::call<&(WebCore::jsNodePrototypeFunctionAppendChildBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSNode*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x108fac5) #61 0x49e53c601177 (<unknown module>) #62 0x4ce6db45b in llint_entry (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa8c45b) #63 0x4ce6c43d8 in vmEntryToJavaScript (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa753d8) #64 0x4cfcec937 in JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x209d937) #65 0x4d0318140 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c9140) #66 0x4d0318242 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c9242) #67 0x4d031861f in JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c961f) #68 0x4b641401b in WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x289a01b) #69 0x4b64defa8 in WebCore::ScheduledAction::executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValue, WebCore::ScriptExecutionContext&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2964fa8) #70 0x4b64de95a in WebCore::ScheduledAction::execute(WebCore::Document&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x296495a) #71 0x4b7958aaa in WebCore::DOMTimer::fired() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3ddeaaa) #72 0x4b7cb6f06 in WebCore::ThreadTimers::sharedTimerFiredInternal() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x413cf06) #73 0x4b7d2e40e in WebCore::timerFired(__CFRunLoopTimer*, void*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x41b440e) #74 0x7fff3d7fee14 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x59e14) #75 0x7fff3d7fe9c0 in __CFRunLoopDoTimer (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x599c0) #76 0x7fff3d7fe4f9 in __CFRunLoopDoTimers (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x594f9) #77 0x7fff3d7dfb33 in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x3ab33) #78 0x7fff3d7df084 in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x3a084) #79 0x7fff3fa53a9e in -[NSRunLoop(NSRunLoop) runMode:beforeDate:] (/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation:x86_64+0x1ca9e) #80 0x7fff3fa53973 in -[NSRunLoop(NSRunLoop) run] (/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation:x86_64+0x1c973) #81 0x7fff69ecb1d6 in _xpc_objc_main (/usr/lib/system/libxpc.dylib:x86_64+0x111d6) #82 0x7fff69ecacd8 in xpc_main (/usr/lib/system/libxpc.dylib:x86_64+0x10cd8) #83 0x4a8904465 in WebKit::XPCServiceMain(int, char const**) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x904465) #84 0x7fff69c983d4 in start (/usr/lib/system/libdyld.dylib:x86_64+0x163d4) ==46072==Register values: rax = 0x000000000000000e rbx = 0x00007ffee4169a40 rcx = 0x000010000000000e rdx = 0x0000000000000000 rdi = 0x0000000000000070 rsi = 0x0000000000000000 rbp = 0x00007ffee4169930 rsp = 0x00007ffee4169930 r8 = 0x0000000000000000 r9 = 0x0000100000000000 r10 = 0x00000004ba065168 r11 = 0xfffffffffffffec0 r12 = 0x0000612000a93840 r13 = 0x0000000000c2b700 r14 = 0x0000000000000000 r15 = 0x0000000000000000
Attachments
Minimal test case (2.01 KB, text/html)
2020-02-27 07:52 PST, Ali Juma 		no flags
Details
Patch (7.40 KB, patch)
2020-03-19 22:01 PDT, Eugene But 		no flags
DetailsFormatted DiffDiff
Minimized test case (360 bytes, text/html)
2020-03-19 23:07 PDT, Jack 		no flags
Details
Patch (6.40 KB, patch)
2020-03-20 09:53 PDT, Eugene But 		no flags
DetailsFormatted DiffDiff
Patch (4.93 KB, patch)
2020-03-25 14:09 PDT, Eugene But 		no flags
DetailsFormatted DiffDiff
Patch (4.80 KB, patch)
2020-03-25 16:17 PDT, Eugene But 		no flags
DetailsFormatted DiffDiff
Patch (6.12 KB, patch)
2020-03-25 22:39 PDT, Eugene But 		no flags
DetailsFormatted DiffDiff
Patch (5.31 KB, patch)
2020-03-26 10:30 PDT, Eugene But 		no flags
DetailsFormatted DiffDiff
Patch (5.95 KB, patch)
2020-03-26 14:40 PDT, Eugene But 		no flags
DetailsFormatted DiffDiff
Patch (8.61 KB, patch)
2020-03-26 16:13 PDT, Eugene But 		no flags
DetailsFormatted DiffDiff
Patch (8.62 KB, patch)
2020-03-26 17:39 PDT, Eugene But 		no flags
DetailsFormatted DiffDiff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2020-02-27 07:52:19 PST
<rdar://problem/59847043>
Eugene But
Comment 2 2020-03-19 19:25:41 PDT
RenderBox::styleDidChange crashes when changing style for HTMLBodyElement element. Crash happens on dereferencing null document().documentElement()->renderer() pointer: if (.... || !documentElementRenderer->style().hasExplicitlySetWritingMode())) { During the crash web page reattaches <body> element to <desc> element: document.getElementsByTagName("desc")[0].appendChild(document.getElementById("body")); with JavaScript below: function DOMSubtreeModifiedHandler() { var desc = document.createElementNS('http://www.w3.org/2000/svg', 'desc'); try { document.appendChild(desc); } catch(e) { // Can cause JS stack overflow. } document.replaceChild(document.getElementById("body"), event.srcElement); } document.addEventListener("DOMSubtreeModified", DOMSubtreeModifiedHandler); setTimeout(function() { document.getElementById("head").outerHTML = ""; document.getElementsByTagName("desc")[0].appendChild(document.getElementById("body")); }); It does not feel right that document().documentElement()->renderer() is null, but RenderBox code has a few places with null pointer checks for document().documentElement()->renderer(), so for me it's hard to reason about document().documentElement()->renderer() lifetime. I will upload a patch with cleaned up test case (test case attached to this bug is quite complicated) and null pointer check. But I don't think that adding pointer check is the right fix. I'm not sure where to look further, but I can continue debugging if someone could provide me with pointers. Finally this does not look like a security bug.
Eugene But
Comment 3 2020-03-19 21:31:42 PDT
Chrome does not allow appending <body> as a child for another element and prints the following error in dev tools console: Uncaught TypeError: Failed to execute 'appendChild' on 'Node': parameter 1 is not of type 'Node'.
Ryosuke Niwa
Comment 4 2020-03-19 21:43:17 PDT
(In reply to Eugene But from comment #3) > Chrome does not allow appending <body> as a child for another element and > prints the following error in dev tools console: > > Uncaught TypeError: Failed to execute 'appendChild' on 'Node': parameter 1 > is not of type 'Node'. How about Firefox?
Eugene But
Comment 5 2020-03-19 22:01:53 PDT
Created attachment 394068 [details] Patch
Eugene But
Comment 6 2020-03-19 22:10:33 PDT
Firefox does not add <desc> element. The following code inside DOMSubtreeModified callback seems to be no-op: var desc = document.createElementNS('http://www.w3.org/2000/svg', 'desc'); try { document.appendChild(desc); } Please note that my patch is mostly to demo the problem with small test case. I know that LayoutTests/ChangeLog is no good.
Jack
Comment 7 2020-03-19 22:12:49 PDT
(In reply to Eugene But from comment #2) There is a reentrance allowing <desc> be the first and only child of the document thus in Document::childrenChanged we set documentElement to <desc>. > It does not feel right that document().documentElement()->renderer() is > null, but RenderBox code has a few places with null pointer checks for > document().documentElement()->renderer(), so for me it's hard to reason > about document().documentElement()->renderer() lifetime.
Jack
Comment 8 2020-03-19 22:18:19 PDT
Sorry the assignee wasn't changed so we overlapped. I was using a similar reduced test case to demonstrate the reentrancy: <html id=HTML> <head> <script> function appendDesc() { document.execCommand("SelectAll"); document.appendChild(document.createElementNS('http://www.w3.org/2000/svg', 'desc')); } document.addEventListener("DOMSubtreeModified", appendDesc); window.onload = () => { document.replaceChild(BODY.cloneNode(), HTML); }; </script> <body id=BODY dir="rtl"> This seems to be what happens: 1. Whole HTML subtree is removed in the middle of replacing HTML in document. Document becomes empty now. 2. DOMSubtreeModified is triggered so we reenter JSC and carry out commands of “SelectAll” and “appendChild”. In appending, we create a SVGDescElement and inserting it into empty Document. 3. Since SVGDescElement doesn’t require renderer, so its renderer is set to null. 4. Since Document is empty, the SVGDescElement becomes the “documentElement”. 5. After DOMSubtreeModified event is handled, we resume the command of replacing HTML, and insert cloned BODY into document. 6. After replace command is done, we dispatch subtree modified event again. 7. This time in JSC reentrance while carrying out “SelectAll”, we update the render tree and create renderer for BODY element. 8. In initializing render style for BODY element, because the test case specify direction to be “right to left”, we attempt to update the direction. 9. Before updating, we check if documentElement’s renderer style has explicity direction setting. However, since SVGDescElement has no renderer, the code crashes when accessing nullptr. Node tree when the crash happens: *#document 0x124a8d810 (renderer 0x124a8cf90) desc 0x124a8eac0 (renderer 0x0) BODY 0x124a8e9c0 (renderer 0x124a8d3a0)
Jack
Comment 9 2020-03-19 22:29:31 PDT
Comment on attachment 394068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394068&action=review > Source/WebCore/rendering/RenderBox.cpp:348 > + if (documentElementRenderer) { Is it possible that "isDocElementRenderer" is true so we don't need to check documentElementRenderer? Not sure if it is a valid scenario to handle.
Jack
Comment 10 2020-03-19 22:42:43 PDT
In this case, we do not seem to be able to choose documentElement, so probably it is a valid scenario to have renderer-less documentElement?
Jack
Comment 11 2020-03-19 23:07:40 PDT
Created attachment 394069 [details] Minimized test case Hi Eugene, thanks for reducing the test case. Alan further minimized the test. Would you like to use it for the fast test?
alan
Comment 12 2020-03-20 08:11:02 PDT
(In reply to Eugene But from comment #2) > RenderBox::styleDidChange crashes when changing style for HTMLBodyElement > element. > Crash happens on dereferencing null document().documentElement()->renderer() > pointer: > > if (.... || > !documentElementRenderer->style().hasExplicitlySetWritingMode())) { > > > During the crash web page reattaches <body> element to <desc> element: > document.getElementsByTagName("desc")[0].appendChild(document. > getElementById("body")); > > with JavaScript below: > > function DOMSubtreeModifiedHandler() { > var desc = document.createElementNS('http://www.w3.org/2000/svg', 'desc'); > try { > document.appendChild(desc); > } > catch(e) { > // Can cause JS stack overflow. > } > document.replaceChild(document.getElementById("body"), event.srcElement); > } > document.addEventListener("DOMSubtreeModified", DOMSubtreeModifiedHandler); > setTimeout(function() { > document.getElementById("head").outerHTML = ""; > > document.getElementsByTagName("desc")[0].appendChild(document. > getElementById("body")); > }); > > It does not feel right that document().documentElement()->renderer() is > null, but RenderBox code has a few places with null pointer checks for > document().documentElement()->renderer(), so for me it's hard to reason > about document().documentElement()->renderer() lifetime. > > I will upload a patch with cleaned up test case (test case attached to this > bug is quite complicated) and null pointer check. But I don't think that > adding pointer check is the right fix. I'm not sure where to look further, > but I can continue debugging if someone could provide me with pointers. > Finally this does not look like a security bug. I agree. It does not seem right to have a nullptr documentElement renderer at all. We should not get into this state though DOM mutation.
Eugene But
Comment 13 2020-03-20 09:53:45 PDT
Created attachment 394094 [details] Patch
Eugene But
Comment 14 2020-03-20 10:10:14 PDT
>> Hi Eugene, thanks for reducing the test case. Alan further minimized the test. Would you like to use it for the fast test? Alan's test case looks great. Updated the patch.
Eugene But
Comment 15 2020-03-20 10:23:37 PDT
>> I agree. It does not seem right to have a nullptr documentElement renderer at all. We should not get into this state though DOM mutation. Per comment #8 it looks like body is detached from the document, which might be a reasonable cause for null renderer. Alan, do you think WebKit should prevent replacing body with another element? This is what Chrome does: Uncaught DOMException: Failed to execute 'replaceChild' on 'Node': Only one element on document allowed. Or maybe WebKit should prevent adding <desc> child element? This is what Firefox does: HierarchyRequestError: Node cannot be inserted at the specified point in the hierarchy There might be a few different ways to get into the state when body is detached and renderer is null (original test case produced by the Fuzzer is slightly different from test case in comment #11). Do you prefer fixing all use cases which prevent WebKit from getting into crashing state or do you think it is more practical to not crash, when WebKit gets into this bad state with detached body and null renderer?
Ryosuke Niwa
Comment 16 2020-03-20 13:07:29 PDT
(In reply to Eugene But from comment #15) > >> I agree. It does not seem right to have a nullptr documentElement renderer at all. We should not get into this state though DOM mutation. > > Per comment #8 it looks like body is detached from the document, which might > be a reasonable cause for null renderer. > > Alan, do you think WebKit should prevent replacing body with another > element? This is what Chrome does: > Uncaught DOMException: Failed to execute 'replaceChild' on 'Node': Only one > element on document allowed. > > Or maybe WebKit should prevent adding <desc> child element? This is what > Firefox does: > HierarchyRequestError: Node cannot be inserted at the specified point in the > hierarchy We should probably do something about this. What does HTML / DOM spec say? > There might be a few different ways to get into the state when body is > detached and renderer is null (original test case produced by the Fuzzer is > slightly different from test case in comment #11). Do you prefer fixing all > use cases which prevent WebKit from getting into crashing state or do you > think it is more practical to not crash, when WebKit gets into this bad > state with detached body and null renderer? Can't we have <!DOCTYPE html><html style="display: none">? That'd force the document element to not have a renderer?
Eugene But
Comment 17 2020-03-20 16:15:00 PDT
> We should probably do something about this. What does HTML / DOM spec say? Chatted with Ali, and he said this: <quote> Chrome throws an exception here: https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/dom/document.cc;drc=ee4cff87f02e46e1fbbdaef0aa123e05761b35e8;l=5018?originalUrl=https:%2F%2Fcs.chromium.org%2F Very conveniently, there's a comment above that method that cites the spec :) I think the relevant part is from https://dom.spec.whatwg.org/#concept-node-replace, where it says if "parent has an element child that is not child", i.e., the document element already has a child (presumably |body|) and we're trying to replace that child with another one, then we should throw an exception. </quote> > Can't we have <!DOCTYPE html><html style="display: none">? That'd force the document element to not have a renderer? Document renderer was not null for <!DOCTYPE html><html style="display: none"> Document::CanAcceptChild is present in both WebKit and Blink. Next week I will try fixing the bug inside Document::CanAcceptChild by throwing JS exception. Please let me know if you want to keep null pointer check (which has pros and cons).
Ryosuke Niwa
Comment 18 2020-03-20 17:12:46 PDT
(In reply to Eugene But from comment #17) > > We should probably do something about this. What does HTML / DOM spec say? > > Chatted with Ali, and he said this: > > <quote> > Chrome throws an exception here: > https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/ > renderer/core/dom/document.cc;drc=ee4cff87f02e46e1fbbdaef0aa123e05761b35e8; > l=5018?originalUrl=https:%2F%2Fcs.chromium.org%2F > > Very conveniently, there's a comment above that method that cites the spec > :) I think the relevant part is from > https://dom.spec.whatwg.org/#concept-node-replace, where it says if "parent > has an element child that is not child", i.e., the document element already > has a child (presumably |body|) and we're trying to replace that child with > another one, then we should throw an exception. > </quote> > > > Can't we have <!DOCTYPE html><html style="display: none">? That'd force the document element to not have a renderer? > > Document renderer was not null for <!DOCTYPE html><html style="display: > none"> > > > Document::CanAcceptChild is present in both WebKit and Blink. Next week I > will try fixing the bug inside Document::CanAcceptChild by throwing JS > exception. Please let me know if you want to keep null pointer check (which > has pros and cons). I think we can forego with a null check for the renderer for now. If we find that there are other cases in which we'd hit the same code path, we can reconsider.
alan
Comment 19 2020-03-20 17:14:52 PDT
> > There might be a few different ways to get into the state when body is > > detached and renderer is null (original test case produced by the Fuzzer is > > slightly different from test case in comment #11). Do you prefer fixing all > > use cases which prevent WebKit from getting into crashing state or do you > > think it is more practical to not crash, when WebKit gets into this bad > > state with detached body and null renderer? > > Can't we have <!DOCTYPE html><html style="display: none">? That'd force the > document element to not have a renderer? and then we would not have a body renderer either to set the style on.
Jack
Comment 20 2020-03-24 13:21:07 PDT
Hi Eugene, Would you like to submit the null check or prefer us to make the patch? Please feel free to let me know. Thanks, Jack (In reply to Ryosuke Niwa from comment #18) > I think we can forego with a null check for the renderer for now. If we find > that there are other cases in which we'd hit the same code path, we can > reconsider.
Eugene But
Comment 21 2020-03-24 16:12:58 PDT
Hi Jack. I was going to work on a proper fix tomorrow (sorry I had to switch and work on something else). That fix would throw JS exception from Document::CanAcceptChild to make sure that body does not get detached. Ryosuke advised against null check. What would be a good way for me to communicate when I actively work on the bug and when I don't? In the future can leave the comments when I have to switch and work on something else instead of disappearing for a few days. For this specific bug, are you ok to wait for 1-2 days or there is some deadline when you have to fix the crash?
Jack
Comment 22 2020-03-24 17:55:37 PDT
Hi Eugene, Oh, sorry, there is no rush. The old patch was not obsoleted so I thought you would like to have a temporary fix before finding some time to work on the exception. Let me change the assignee so there is no confusion in communication. I will update our internal tracking system when the patch is ready. Thanks for working on this issue. Jack (In reply to Eugene But from comment #21) > Hi Jack. I was going to work on a proper fix tomorrow (sorry I had to switch > and work on something else). That fix would throw JS exception from > Document::CanAcceptChild to make sure that body does not get detached. > Ryosuke advised against null check. > > What would be a good way for me to communicate when I actively work on the > bug and when I don't? In the future can leave the comments when I have to > switch and work on something else instead of disappearing for a few days. > > For this specific bug, are you ok to wait for 1-2 days or there is some > deadline when you have to fix the crash?
Eugene But
Comment 23 2020-03-25 14:09:29 PDT
Created attachment 394543 [details] Patch
Eugene But
Comment 24 2020-03-25 14:13:32 PDT
The last patch fixes the crash. The patch was uploaded to run the tests. I'm still trying to find a better way to write the test.
Eugene But
Comment 25 2020-03-25 16:17:09 PDT
Created attachment 394560 [details] Patch
Eugene But
Comment 26 2020-03-25 16:18:41 PDT
Updated the test. I still have to check if the fix cause other tests to fail.
Ryosuke Niwa
Comment 27 2020-03-25 17:35:22 PDT
There is no security implication here.
Ryosuke Niwa
Comment 28 2020-03-25 17:46:44 PDT
Comment on attachment 394560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394560&action=review > Source/WebCore/ChangeLog:25 > + Hence the fix is to return false from Document::canAcceptChild if |newChild| is ELEMENT_NODE and > + |refChild| does not exist. This logic was copied from Document::CanAcceptChild method in Chromium's Blink: Why? This doesn't follow from from the above spec text at all. For starters your code change applies to both pre-insert and replace, which seems incorrect, and there is nothing in the spec which suggests that we must throw HierarchyRequestError when child is null in that concept to replace a node. For starters, it's non sense for refChild to be null when replacing a node. > Source/WebCore/ChangeLog:28 > + Reviewed by NOBODY (OOPS!). This line should appear before the long description bug after the bug URL. See other entries.
Eugene But
Comment 29 2020-03-25 18:22:54 PDT
>> Why? This doesn't follow from from the above spec text at all. Yep, sorry about that. I misread |if (num_elements > 1 || num_doctypes > 1) {| and for some reason thought that num_elements is expected to be greater than 0. I will try counting the number of elements, like Chromium does.
Eugene But
Comment 30 2020-03-25 18:55:28 PDT
My assumption about bug in Document::canAcceptChild was incorrect. canAcceptChild correctly returns false if |newChild| is ELEMENT_NODE and Document already has ELEMENT_NODE child. Maybe it's actually ok that document().documentElement()->renderer() is null for detached body, because body does not have a document? Unless the spec forbids detaching body in the first place.
Ryosuke Niwa
Comment 31 2020-03-25 19:04:42 PDT
(In reply to Eugene But from comment #30) > My assumption about bug in Document::canAcceptChild was incorrect. > canAcceptChild correctly returns false if |newChild| is ELEMENT_NODE and > Document already has ELEMENT_NODE child. > > Maybe it's actually ok that document().documentElement()->renderer() is null > for detached body, because body does not have a document? Unless the spec > forbids detaching body in the first place. First off, document().documentElement() is typically a HTML element or SVG element, not body element. The issue here isn't so much about body being detached but rather than there are two elements (desc & body) that are direct children of a document, which is clearly not allowed in the spec bug. It looks we're getting to that state based on the observation on https://bugs.webkit.org/show_bug.cgi?id=208311#c8
Ryosuke Niwa
Comment 32 2020-03-25 19:06:29 PDT
For example, if I try to execute document.appendChild(document.createElement('div')) in about:blank, I get HierarchyRequestError. So in theory, we should never get to a state where we have two elements as direct children of document. We need to figure out why & how we're getting to that state given the check we have in Document::canAcceptChild.
Eugene But
Comment 33 2020-03-25 19:45:54 PDT
Thanks, this makes sense. Looks like Document::canAcceptChild properly rejects SVG elements, but allows adding <body> as the second child. Digging into this.
Eugene But
Comment 34 2020-03-25 20:30:12 PDT
Perhaps this is the piece of code in ContainerNode.cpp where things go wrong: // If oldChild == newChild then oldChild no longer has a parent at this point. if (oldChild.parentNode()) { auto removeResult = removeChild(oldChild); if (removeResult.hasException()) return removeResult.releaseException(); // Does this one more time because removeChild() fires a MutationEvent. for (auto& child : targets) { validityResult = checkPreReplacementValidity(*this, child, oldChild, ShouldValidateChildParent::No); removeChild(oldChild) fires mutation events and appends SVG to the document. checkPreReplacementValidity calls canAcceptChild, which allows adding body as the second child of the document: bool Document::canAcceptChild(const Node& newChild, const Node* refChild, AcceptChildOperation operation) const { if (operation == AcceptChildOperation::Replace && refChild->nodeType() == newChild.nodeType()) return true; Chromium on the other hand does an extra checks in RecheckNodeInsertionStructuralPrereq method: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/container_node.cc?type=cs&sq=package:chromium&g=0&l=287 where oldChild/refChild argument is passed as null.
Eugene But
Comment 35 2020-03-25 22:39:34 PDT
Created attachment 394578 [details] Patch
Eugene But
Comment 36 2020-03-25 22:40:55 PDT
I don't particularly like the fix in the last patch, but it may work without breaking other things and I'd like to run tests for it.
Ryosuke Niwa
Comment 37 2020-03-25 22:43:32 PDT
Comment on attachment 394578 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394578&action=review > Source/WebCore/dom/ContainerNode.cpp:528 > + validityResult = checkAcceptChild( Please don't use 80-character line warp. All of this code should fit in a single line. > Source/WebCore/dom/ContainerNode.cpp:535 > + // oldChild could be removed inside MutationEvent handler. In > + // that case use InsertOrAdd operation, which has stricter > + // checks comparing to Replace. > + oldChild.parentNode() ? > + Document::AcceptChildOperation::Replace : > + Document::AcceptChildOperation::InsertOrAdd, We should probably compute this in a separate statement in the prior line.
Eugene But
Comment 38 2020-03-26 10:23:40 PDT
The last patch fixes the crash, but replace-child.html layout test failed: test('replacing doctype with element', function() { doc = parser.parseFromString('<!DOCTYPE html><body/>', 'text/xml'); doc.removeChild(doc.documentElement); newChild = doc.createElement('bar'); shouldNotThrow('doc.replaceChild(newChild, doc.doctype)'); }); In this test doc.replaceChild(newChild, doc.doctype) actually threw an exception. I will try another approach, which still fixes the crash and pass replace-child.html
Eugene But
Comment 39 2020-03-26 10:30:59 PDT
Created attachment 394622 [details] Patch
Eugene But
Comment 40 2020-03-26 13:43:07 PDT
Should I worry about failing tests on mac-debug-wk1? Failing tests that I checked are all related to threading assertions.
Ali Juma
Comment 41 2020-03-26 13:55:48 PDT
(In reply to Eugene But from comment #40) > Should I worry about failing tests on mac-debug-wk1? Failing tests that I > checked are all related to threading assertions. Those failures look unrelated.
Ryosuke Niwa
Comment 42 2020-03-26 14:33:36 PDT
Comment on attachment 394622 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394622&action=review > Source/WebCore/dom/Document.cpp:3876 > - if (operation == AcceptChildOperation::Replace && refChild->nodeType() == newChild.nodeType()) > + if (operation == AcceptChildOperation::Replace && refChild->parentNode() && refChild->nodeType() == newChild.nodeType()) What is this check about? Maybe we're trying to check that refChild->parentNode() == this to detect mutation event related changes?
Eugene But
Comment 43 2020-03-26 14:40:44 PDT
Created attachment 394658 [details] Patch
Ryosuke Niwa
Comment 44 2020-03-26 14:57:41 PDT
Comment on attachment 394658 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394658&action=review > Source/WebCore/ChangeLog:25 > + Replace operations if new child had the same type as old child, even if old child does not have parent. This check isn't quite right. The refChild could be removed from Document but then inserted elsewhere. > Source/WebCore/dom/Document.cpp:3876 > + if (operation == AcceptChildOperation::Replace && refChild->parentNode() && refChild->nodeType() == newChild.nodeType()) So this should really be checking refChild->parentNode() == this. I also suggest writing a test case where you'd insert the old body into some other element (e.g. just document.createElement('div').appendChild(body) inside the mutation event);
Eugene But
Comment 45 2020-03-26 15:00:39 PDT
Comment on attachment 394622 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394622&action=review >> Source/WebCore/dom/Document.cpp:3876 >> + if (operation == AcceptChildOperation::Replace && refChild->parentNode() && refChild->nodeType() == newChild.nodeType()) > > What is this check about? > Maybe we're trying to check that refChild->parentNode() == this to detect mutation event related changes? Sorry for confusion. I've uploaded this patch to test if the fix work without explaining the fix. Now, to answer your question. It is my understanding that the original code |if (operation == Replace && refChild->nodeType() == newChild.nodeType()) return true;| is optimization. The code after early return (line 3879 and below) performs all required and correct checks but it's just slower, because it has to check if document has element child. I think original code assumes that Replace consists of 2 steps: #1 remove old child; #2 add new child. This assumption does not take into account that mutation event could add a child between steps 1 and 2. canAcceptChild() can still use this optimization iff old child has a parent that is "this document". But optimization with early return can't be used if |refChild| was removed. After writing this explanation, now I see a flaw in my patch. Instead of checking that |refChild->parentNode()| is not null, we should actually check that |refChild->parentNode() == this|, because mutation event can cause refChild to be reattached to another parent. I'm going to upload a new patch to run the tests.
Eugene But
Comment 46 2020-03-26 15:01:21 PDT
Comment on attachment 394622 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394622&action=review >>> Source/WebCore/dom/Document.cpp:3876 >>> + if (operation == AcceptChildOperation::Replace && refChild->parentNode() && refChild->nodeType() == newChild.nodeType()) >> >> What is this check about? >> Maybe we're trying to check that refChild->parentNode() == this to detect mutation event related changes? > > Sorry for confusion. I've uploaded this patch to test if the fix work without explaining the fix. > > Now, to answer your question. It is my understanding that the original code |if (operation == Replace && refChild->nodeType() == newChild.nodeType()) return true;| is optimization. The code after early return (line 3879 and below) performs all required and correct checks but it's just slower, because it has to check if document has element child. > > I think original code assumes that Replace consists of 2 steps: #1 remove old child; #2 add new child. This assumption does not take into account that mutation event could add a child between steps 1 and 2. canAcceptChild() can still use this optimization iff old child has a parent that is "this document". But optimization with early return can't be used if |refChild| was removed. > > After writing this explanation, now I see a flaw in my patch. Instead of checking that |refChild->parentNode()| is not null, we should actually check that |refChild->parentNode() == this|, because mutation event can cause refChild to be reattached to another parent. I'm going to upload a new patch to run the tests. Sorry for confusion. I've uploaded this patch to test if the fix work without explaining the fix. Now, to answer your question. It is my understanding that the original code |if (operation == Replace && refChild->nodeType() == newChild.nodeType()) return true;| is optimization. The code after early return (line 3879 and below) performs all required and correct checks but it's just slower, because it has to check if document has element child. I think original code assumes that Replace consists of 2 steps: #1 remove old child; #2 add new child. This assumption does not take into account that mutation event could add a child between steps 1 and 2. canAcceptChild() can still use this optimization iff old child has a parent that is "this document". But optimization with early return can't be used if |refChild| was removed. After writing this explanation, now I see a flaw in my patch. Instead of checking that |refChild->parentNode()| is not null, we should actually check that |refChild->parentNode() == this| as you suggested, because mutation event can cause refChild to be reattached to another parent. I'm going to upload a new patch to run the tests.
Eugene But
Comment 47 2020-03-26 16:13:04 PDT
Created attachment 394669 [details] Patch
Ryosuke Niwa
Comment 48 2020-03-26 16:47:42 PDT
Comment on attachment 394669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394669&action=review Cool. New patch makes sense to me. > Source/WebCore/ChangeLog:26 > + Replace operations if new child had the same type as old child, even if old child does not have parent. > + The absence of parent (or parent that is not "this document") means that old child was removed from Now that we've fixed the bug in your previous patch, this explanation should be updated too. Also, please insert a blank line to split these two paragraphs. > Source/WebCore/ChangeLog:32 > + Test: fast/dom/add-document-child-during-document-child-replacement.html > + Test: fast/dom/add-document-child-and-reparent-old-child-during-document-child-replacement.html This should instead be: Tests: fast/dom/add-document-child-during-document-child-replacement.html fast/dom/add-document-child-and-reparent-old-child-during-document-child-replacement.html
Eugene But
Comment 49 2020-03-26 17:39:08 PDT
Created attachment 394682 [details] Patch
Eugene But
Comment 50 2020-03-26 17:53:39 PDT
Comment on attachment 394669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394669&action=review >> Source/WebCore/ChangeLog:26 >> + The absence of parent (or parent that is not "this document") means that old child was removed from > > Now that we've fixed the bug in your previous patch, this explanation should be updated too. > Also, please insert a blank line to split these two paragraphs. Done. >> Source/WebCore/ChangeLog:32 >> + Test: fast/dom/add-document-child-and-reparent-old-child-during-document-child-replacement.html > > This should instead be: > Tests: fast/dom/add-document-child-during-document-child-replacement.html > fast/dom/add-document-child-and-reparent-old-child-during-document-child-replacement.html Done.
Darin Adler
Comment 51 2020-03-26 18:55:51 PDT
Comment on attachment 394682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394682&action=review > Source/WebCore/dom/Document.cpp:3874 > bool Document::canAcceptChild(const Node& newChild, const Node* refChild, AcceptChildOperation operation) const Can’t help noticing that this function is not written with efficiency in mind. The nodeType virtual function is pretty expensive and we call it 3 times!
Eugene But
Comment 52 2020-03-27 07:05:07 PDT
Comment on attachment 394682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394682&action=review >> Source/WebCore/dom/Document.cpp:3874 >> bool Document::canAcceptChild(const Node& newChild, const Node* refChild, AcceptChildOperation operation) const > > Can’t help noticing that this function is not written with efficiency in mind. The nodeType virtual function is pretty expensive and we call it 3 times! newChild.nodeType() is called twice and refChild->nodeType() is called once. We could create a local variable to compute newChild.nodeType() only once. Given that "this patch" makes canAcceptChild() slower, do you prefer to add optimization with local variable in this patch or in follow up patch to separate bug fix from performance optimization?
Darin Adler
Comment 53 2020-03-27 09:47:10 PDT
Comment on attachment 394682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394682&action=review >>> Source/WebCore/dom/Document.cpp:3874 >>> bool Document::canAcceptChild(const Node& newChild, const Node* refChild, AcceptChildOperation operation) const >> >> Can’t help noticing that this function is not written with efficiency in mind. The nodeType virtual function is pretty expensive and we call it 3 times! > > newChild.nodeType() is called twice and refChild->nodeType() is called once. We could create a local variable to compute newChild.nodeType() only once. Given that "this patch" makes canAcceptChild() slower, do you prefer to add optimization with local variable in this patch or in follow up patch to separate bug fix from performance optimization? Follow up in a separate bug. It’s unrelated to this patch. Not clear that micro-optimization here would be good, but this can be optimized for all the common cases to not call nodeType at all, using functions like isElementNode and isTextNode that are inline single bit checks. Only exotic cases would require calls to nodeType.
Eugene But
Comment 54 2020-03-27 11:08:34 PDT
Comment on attachment 394682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394682&action=review >>>> Source/WebCore/dom/Document.cpp:3874 >>>> bool Document::canAcceptChild(const Node& newChild, const Node* refChild, AcceptChildOperation operation) const >>> >>> Can’t help noticing that this function is not written with efficiency in mind. The nodeType virtual function is pretty expensive and we call it 3 times! >> >> newChild.nodeType() is called twice and refChild->nodeType() is called once. We could create a local variable to compute newChild.nodeType() only once. Given that "this patch" makes canAcceptChild() slower, do you prefer to add optimization with local variable in this patch or in follow up patch to separate bug fix from performance optimization? > > Follow up in a separate bug. It’s unrelated to this patch. > > Not clear that micro-optimization here would be good, but this can be optimized for all the common cases to not call nodeType at all, using functions like isElementNode and isTextNode that are inline single bit checks. Only exotic cases would require calls to nodeType. Filed https://bugs.webkit.org/show_bug.cgi?id=209667
Ryosuke Niwa
Comment 55 2020-03-27 20:54:58 PDT
Comment on attachment 394682 [details] Patch Great. New patch looks good! By the way, in the future, you can also set cq? for the commit queue. You can specify --commit-queue to webkit-patch if you're using that script to upload a patch.
EWS
Comment 56 2020-03-27 21:00:43 PDT
Committed r259152: <https://trac.webkit.org/changeset/259152> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394682 [details].
Eugene But
Comment 57 2020-03-30 09:26:12 PDT
(In reply to Ryosuke Niwa from comment #55) > Comment on attachment 394682 [details] > Patch > > Great. New patch looks good! By the way, in the future, you can also set cq? > for the commit queue. You can specify --commit-queue to webkit-patch if > you're using that script to upload a patch. Thanks. I will use --commit-queue for patches which in my opinion are ready for commit. Is it still ok to upload patches just to run tests?
Ryosuke Niwa
Comment 58 2020-03-30 11:21:50 PDT
(In reply to Eugene But from comment #57) > (In reply to Ryosuke Niwa from comment #55) > > Comment on attachment 394682 [details] > > Patch > > > > Great. New patch looks good! By the way, in the future, you can also set cq? > > for the commit queue. You can specify --commit-queue to webkit-patch if > > you're using that script to upload a patch. > > Thanks. I will use --commit-queue for patches which in my opinion are ready > for commit. Is it still ok to upload patches just to run tests? Yes, if you're uploading tests just to get through EWS, you can pass --no-review or unset r? from the patch (you can do this from "Details"). Once you think the patch is ready for a code review, you can either set r? on an existing patch, or upload a new patch with r?.
Note You need to log in before you can comment on or make changes to this bug.