Created attachment 240274 [details] Test case The failing test case: <!DOCTYPE html> <a> <p> <b> <u id="test"/> <keygen form="test"/> </b> </a> The bug is also present in Blink and it's reported under crbug.com/426005. Backtrace: SHOULD NEVER BE REACHED ../../Source/WebCore/dom/DocumentOrderedMap.cpp(155) : WebCore::Element* WebCore::DocumentOrderedMap::get(const WTF::AtomicStringImpl&, const WebCore::TreeScope&) const [with bool (* keyMatches)(const WTF::AtomicStringImpl&, const WebCore::Element&) = WebCore::keyMatchesId] Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fff98c1f700 (LWP 20393)] 0x00007fffedae91b5 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321 321 *(int *)(uintptr_t)0xbbadbeef = 0; #0 0x00007fffedae91b5 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321 #1 0x00007ffff307b3fc in WebCore::DocumentOrderedMap::get<&WebCore::keyMatchesId> (this=0x796f80, key=..., scope=...) at ../../Source/WebCore/dom/DocumentOrderedMap.cpp:155 #2 0x00007ffff307a655 in WebCore::DocumentOrderedMap::getElementById (this=0x796f80, key=..., scope=...) at ../../Source/WebCore/dom/DocumentOrderedMap.cpp:161 #3 0x00007ffff3134d70 in WebCore::TreeScope::getElementById (this=0x788d78, elementId=...) at ../../Source/WebCore/dom/TreeScope.cpp:107 #4 0x00007ffff3248d24 in WebCore::FormAssociatedElement::findAssociatedForm (element=0x798160, currentAssociatedForm=0x0) at ../../Source/WebCore/html/FormAssociatedElement.cpp:100 #5 0x00007ffff3248f77 in WebCore::FormAssociatedElement::resetFormOwner (this=0x7981c8) at ../../Source/WebCore/html/FormAssociatedElement.cpp:153 #6 0x00007ffff324943e in WebCore::FormAssociatedElement::formAttributeTargetChanged (this=0x7981c8) at ../../Source/WebCore/html/FormAssociatedElement.cpp:250 #7 0x00007ffff324952c in WebCore::FormAttributeTargetObserver::idTargetChanged (this=0x94d330) at ../../Source/WebCore/html/FormAssociatedElement.cpp:272 #8 0x00007ffff30b3d3b in WebCore::IdTargetObserverRegistry::notifyObserversInternal (this=0xa7bfa0, id=...) at ../../Source/WebCore/dom/IdTargetObserverRegistry.cpp:70 #9 0x00007ffff3136270 in WebCore::IdTargetObserverRegistry::notifyObservers (this=0xa7bfa0, id=...) at ../../Source/WebCore/dom/IdTargetObserverRegistry.h:73 #10 0x00007ffff3134f7d in WebCore::TreeScope::removeElementById (this=0x788d78, elementId=..., element=...) at ../../Source/WebCore/dom/TreeScope.cpp:143 #11 0x00007ffff3094242 in WebCore::Element::updateIdForTreeScope (this=0x85e0a0, scope=..., oldId=..., newId=...) at ../../Source/WebCore/dom/Element.cpp:2658 #12 0x00007ffff308f321 in WebCore::Element::removedFrom (this=0x85e0a0, insertionPoint=...) at ../../Source/WebCore/dom/Element.cpp:1391 #13 0x00007ffff300b6af in WebCore::ChildNodeRemovalNotifier::notifyNodeRemovedFromDocument (this=0x7fffffffce00, node=...) at ../../Source/WebCore/dom/ContainerNodeAlgorithms.h:240 #14 0x00007ffff301085d in WebCore::ChildNodeRemovalNotifier::notifyDescendantRemovedFromDocument (this=0x7fffffffce00, node=...) at ../../Source/WebCore/dom/ContainerNodeAlgorithms.cpp:71 #15 0x00007ffff300b6dd in WebCore::ChildNodeRemovalNotifier::notifyNodeRemovedFromDocument (this=0x7fffffffce00, node=...) at ../../Source/WebCore/dom/ContainerNodeAlgorithms.h:243 #16 0x00007ffff301085d in WebCore::ChildNodeRemovalNotifier::notifyDescendantRemovedFromDocument (this=0x7fffffffce00, node=...) at ../../Source/WebCore/dom/ContainerNodeAlgorithms.cpp:71 #17 0x00007ffff300b6dd in WebCore::ChildNodeRemovalNotifier::notifyNodeRemovedFromDocument (this=0x7fffffffce00, node=...) at ../../Source/WebCore/dom/ContainerNodeAlgorithms.h:243 #18 0x00007ffff300b7cf in WebCore::ChildNodeRemovalNotifier::notify (this=0x7fffffffce00, node=...) at ../../Source/WebCore/dom/ContainerNodeAlgorithms.h:258 #19 0x00007ffff30083e9 in WebCore::ContainerNode::parserRemoveChild (this=0xa7cab0, oldChild=...) at ../../Source/WebCore/dom/ContainerNode.cpp:627 #20 0x00007ffff338ba2d in WebCore::insert (task=...) at ../../Source/WebCore/html/parser/HTMLConstructionSite.cpp:93 #21 0x00007ffff338bcff in WebCore::executeInsertAlreadyParsedChildTask (task=...) at ../../Source/WebCore/html/parser/HTMLConstructionSite.cpp:127 #22 0x00007ffff338bdb6 in WebCore::executeTask (task=...) at ../../Source/WebCore/html/parser/HTMLConstructionSite.cpp:147 #23 0x00007ffff338c104 in WebCore::HTMLConstructionSite::executeQueuedTasks (this=0xa7c818) at ../../Source/WebCore/html/parser/HTMLConstructionSite.cpp:193 #24 0x00007ffff33bde90 in WebCore::HTMLTreeBuilder::constructTree (this=0xa7c800, token=0x7fffffffcf90) at ../../Source/WebCore/html/parser/HTMLTreeBuilder.cpp:367 #25 0x00007ffff3395827 in WebCore::HTMLDocumentParser::constructTreeFromHTMLToken (this=0x8562f0, rawToken=...) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:352 #26 0x00007ffff339545d in WebCore::HTMLDocumentParser::pumpTokenizer (this=0x8562f0, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:309 #27 0x00007ffff3394bf5 in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible (this=0x8562f0, mode=WebCore::HTMLDocumentParser::AllowYield) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:189 #28 0x00007ffff3395dbd in WebCore::HTMLDocumentParser::append (this=0x8562f0, inputSource=...) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:428 #29 0x00007ffff301f521 in WebCore::DecodedDataDocumentParser::flush (this=0x8562f0, writer=...) at ../../Source/WebCore/dom/DecodedDataDocumentParser.cpp:60 #30 0x00007ffff35031d7 in WebCore::DocumentWriter::end (this=0x850b90) at ../../Source/WebCore/loader/DocumentWriter.cpp:243 #31 0x00007ffff34ee75b in WebCore::DocumentLoader::finishedLoading (this=0x850af0, finishTime=0) at ../../Source/WebCore/loader/DocumentLoader.cpp:441 #32 0x00007ffff34ee4c4 in WebCore::DocumentLoader::notifyFinished (this=0x850af0, resource=0x84c560) at ../../Source/WebCore/loader/DocumentLoader.cpp:375 #33 0x00007ffff35a0a5a in WebCore::CachedResource::checkNotify (this=0x84c560) at ../../Source/WebCore/loader/cache/CachedResource.cpp:347 #34 0x00007ffff35a0b64 in WebCore::CachedResource::finishLoading (this=0x84c560) at ../../Source/WebCore/loader/cache/CachedResource.cpp:363 #35 0x00007ffff359d466 in WebCore::CachedRawResource::finishLoading (this=0x84c560, data=0xa7e1e0) at ../../Source/WebCore/loader/cache/CachedRawResource.cpp:101 #36 0x00007ffff35518fa in WebCore::SubresourceLoader::didFinishLoading (this=0x84cad0, finishTime=0) at ../../Source/WebCore/loader/SubresourceLoader.cpp:309 #37 0x00007ffff354d57d in WebCore::ResourceLoader::didFinishLoading (this=0x84cad0, finishTime=0) at ../../Source/WebCore/loader/ResourceLoader.cpp:512 #38 0x00007ffff3ef1333 in WebCore::readCallback (asyncResult=0xa7c1c0, data=0x85d170) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:1306 #39 0x00007fffeb86c7d6 in async_ready_callback_wrapper (source_object=0x9c5ed0, res=0xa7c1c0, user_data=user_data@entry=0x85d170) at ginputstream.c:523 #40 0x00007fffeb8920d5 in g_task_return_now (task=0xa7c1c0) at gtask.c:1077 #41 0x00007fffeb8920f9 in complete_in_idle_cb (task=0xa7c1c0) at gtask.c:1086 #42 0x00007fffeaad1a2d in g_main_dispatch (context=0x6777f0) at gmain.c:3064 #43 g_main_context_dispatch (context=context@entry=0x6777f0) at gmain.c:3663 #44 0x00007fffeaad1d98 in g_main_context_iterate (context=0x6777f0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3734 #45 0x00007fffeaad205a in g_main_loop_run (loop=0xb00db0) at gmain.c:3928 #46 0x00007ffff457c386 in WTF::RunLoop::run () at ../../Source/WTF/wtf/gtk/RunLoopGtk.cpp:59 #47 0x00007ffff2ad6a46 in WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> (argc=2, argv=0x7fffffffd8b8) at ../../Source/WebKit2/Shared/unix/ChildProcessMain.h:61 #48 0x00007ffff2ad68ab in WebKit::WebProcessMainUnix (argc=2, argv=0x7fffffffd8b8) at ../../Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:73 #49 0x0000000000400871 in main (argc=2, argv=0x7fffffffd8b8) at ../../Source/WebKit2/WebProcess/EntryPoint/unix/WebProcessMain.cpp:44
Chris, do you think this might be related to a top crasher we have?
Still crashes on WebKit ToT: Crashed Thread: 0 Dispatch queue: com.apple.main-thread Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef Exception Note: EXC_CORPSE_NOTIFY VM Regions Near 0xbbadbeef: --> __TEXT 0000000102a79000-0000000102a7b000 [ 8K] r-x/rwx SM=COW /Users/USER/*/Safari-Gala-Debug-SafariTen-199294-82021.app/Contents/Frameworks/WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.Development.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development Application Specific Information: Bundle controller class: BrowserBundleController Process Model: Multiple Web Processes Global Trace Buffer (reverse chronological seconds): 2.641555 CFNetwork 0x00007fff9146eed7 Explicitly setting CF cookie storage singleton 2.641944 CFNetwork 0x00007fff914a58ad Explicitly setting cookie storage singleton Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x0000000107085847 WTFCrash + 39 1 com.apple.WebCore 0x0000000109aaf221 WebCore::Element* WebCore::DocumentOrderedMap::get<WebCore::DocumentOrderedMap::getElementById(WTF::AtomicStringImpl const&, WebCore::TreeScope const&) const::$_0>(WTF::AtomicStringImpl const&, WebCore::TreeScope const&, WebCore::DocumentOrderedMap::getElementById(WTF::AtomicStringImpl const&, WebCore::TreeScope const&) const::$_0 const&) const + 1089 2 com.apple.WebCore 0x0000000109aaedd9 WebCore::DocumentOrderedMap::getElementById(WTF::AtomicStringImpl const&, WebCore::TreeScope const&) const + 41 3 com.apple.WebCore 0x000000010b7979fc WebCore::TreeScope::getElementById(WTF::AtomicString const&) const + 188 4 com.apple.WebCore 0x0000000109d2cdb4 WebCore::FormAssociatedElement::findAssociatedForm(WebCore::HTMLElement const*, WebCore::HTMLFormElement*) + 116 5 com.apple.WebCore 0x0000000109d2cfa8 WebCore::FormAssociatedElement::resetFormOwner() + 56 6 com.apple.WebCore 0x0000000109d2d575 WebCore::FormAssociatedElement::formAttributeTargetChanged() + 21 7 com.apple.WebCore 0x0000000109d2d6d9 WebCore::FormAttributeTargetObserver::idTargetChanged() + 25 8 com.apple.WebCore 0x000000010a1887b7 WebCore::IdTargetObserverRegistry::notifyObserversInternal(WTF::AtomicStringImpl const&) + 263 9 com.apple.WebCore 0x0000000109be5bc0 WebCore::IdTargetObserverRegistry::notifyObservers(WTF::AtomicStringImpl const&) + 192 10 com.apple.WebCore 0x000000010b798084 WebCore::TreeScope::removeElementById(WTF::AtomicStringImpl const&, WebCore::Element&, bool) + 196 11 com.apple.WebCore 0x0000000109bdc4f2 WebCore::Element::updateIdForTreeScope(WebCore::TreeScope&, WTF::AtomicString const&, WTF::AtomicString const&, WebCore::Element::NotifyObservers) + 242 12 com.apple.WebCore 0x0000000109bdce0b WebCore::Element::removedFrom(WebCore::ContainerNode&) + 411 13 com.apple.WebCore 0x00000001096ddbd4 WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::Node&) + 100 14 com.apple.WebCore 0x00000001096ddc6e WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::Node&) + 254 15 com.apple.WebCore 0x00000001096ddc6e WebCore::notifyNodeRemovedFromDocument(WebCore::ContainerNode&, WebCore::Node&) + 254 16 com.apple.WebCore 0x00000001096ddf79 WebCore::notifyChildNodeRemoved(WebCore::ContainerNode&, WebCore::Node&) + 105 17 com.apple.WebCore 0x00000001096d0ee0 WebCore::ContainerNode::notifyChildRemoved(WebCore::Node&, WebCore::Node*, WebCore::Node*, WebCore::ContainerNode::ChildChangeSource) + 48 18 com.apple.WebCore 0x00000001096d2107 WebCore::ContainerNode::parserRemoveChild(WebCore::Node&) + 279 19 com.apple.WebCore 0x0000000109f45774 WebCore::insert(WebCore::HTMLConstructionSiteTask&) + 196 20 com.apple.WebCore 0x0000000109f4552e WebCore::executeInsertAlreadyParsedChildTask(WebCore::HTMLConstructionSiteTask&) + 78 21 com.apple.WebCore 0x0000000109f41b70 WebCore::executeTask(WebCore::HTMLConstructionSiteTask&) + 80 22 com.apple.WebCore 0x0000000109f41af5 WebCore::HTMLConstructionSite::executeQueuedTasks() + 133 23 com.apple.WebCore 0x000000010a09735e WebCore::HTMLTreeBuilder::constructTree(WebCore::AtomicHTMLToken&) + 478 24 com.apple.WebCore 0x0000000109f71b43 WebCore::HTMLDocumentParser::constructTreeFromHTMLToken(WebCore::HTMLTokenizer::TokenPtr&) + 131 25 com.apple.WebCore 0x0000000109f71a4f WebCore::HTMLDocumentParser::pumpTokenizerLoop(WebCore::HTMLDocumentParser::SynchronousMode, bool, WebCore::PumpSession&) + 1343 26 com.apple.WebCore 0x0000000109f7075d WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 445 27 com.apple.WebCore 0x0000000109f7035e WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) + 174 28 com.apple.WebCore 0x0000000109f72403 WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl>&&) + 883 29 com.apple.WebCore 0x0000000109988b8f WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&) + 143 30 com.apple.WebCore 0x0000000109ac6884 WebCore::DocumentWriter::end() + 260 31 com.apple.WebCore 0x0000000109a80a9e WebCore::DocumentLoader::finishedLoading(double) + 398 32 com.apple.WebCore 0x0000000109a80885 WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource*) + 389 33 com.apple.WebCore 0x00000001095c15c2 WebCore::CachedResource::checkNotify() + 130 34 com.apple.WebCore 0x00000001095c16d1 WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*) + 49 35 com.apple.WebCore 0x00000001095bce7a WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 218 36 com.apple.WebCore 0x000000010b576934 WebCore::SubresourceLoader::didFinishLoading(double) + 532 37 com.apple.WebKit 0x00000001033e9aa7 WebKit::WebResourceLoader::didFinishResourceLoad(double) + 151 38 com.apple.WebKit 0x00000001033ef0f3 void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double), std::__1::tuple<double>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double), std::__1::tuple<double>&&, std::index_sequence<0ul>) + 163 39 com.apple.WebKit 0x00000001033ef048 void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double), std::__1::tuple<double>, std::make_index_sequence<1ul> >(std::__1::tuple<double>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double)) + 88 40 com.apple.WebKit 0x00000001033ee162 void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double)>(IPC::MessageDecoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double)) + 226 41 com.apple.WebKit 0x00000001033ed8dc WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) + 636 42 com.apple.WebKit 0x0000000102dbc6c0 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) + 160 43 com.apple.WebKit 0x0000000102b4a5a3 IPC::Connection::dispatchMessage(IPC::MessageDecoder&) + 51 44 com.apple.WebKit 0x0000000102b413d1 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) + 785 45 com.apple.WebKit 0x0000000102b4ab9f IPC::Connection::dispatchOneMessage() + 1519 46 com.apple.WebKit 0x0000000102b5bf0d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_10::operator()() const + 29 47 com.apple.WebKit
I added some logging and here is what I see: addElementById(test) 0x1207bcc30 getElementById(test) getElementById(test) addElementById(test) 0x1207bcc98 getElementById(test) removeElementById(test) 0x1207bcc30 getElementById(test) SHOULD NEVER BE REACHED So we add 2 separate elements with the id 'test' to the DocumentOrderedMap, then remove the first one, then crash when calling getElementById('test') because the map think there second element with the id 'test' but cannot find it in the tree.
<rdar://problem/27702012>
Created attachment 329465 [details] Fixes the assertion failure
Comment on attachment 329465 [details] Fixes the assertion failure Doesn't quite work.
Created attachment 329467 [details] Fixes the assertion
Ping reviewers.
Comment on attachment 329467 [details] Fixes the assertion View in context: https://bugs.webkit.org/attachment.cgi?id=329467&action=review Looks good! I assume the original assertion is still worth having around -- you've just silenced instances of the assertion firing that were not really error conditions, right? > Source/WebCore/ChangeLog:11 > + attribute of the form associated element. When there are other elements with the same ID in the removed tree, I feel like this first sentence is missing something. What happens with the very first element with the ID of the form content attribute? Is this just missing "...form associated element was removed. When there are ..."? > Source/WebCore/dom/ContainerNodeAlgorithms.h:29 > +// FIXME: Delete this class after fixing FormAssociatedElement to avoid calling getElementById during a tree removal. Should this be a Bugzilla with a number in the FIXME?
(In reply to Brent Fulgham from comment #9) > Comment on attachment 329467 [details] > Fixes the assertion > > View in context: > https://bugs.webkit.org/attachment.cgi?id=329467&action=review > > Looks good! I assume the original assertion is still worth having around -- > you've just silenced instances of the assertion firing that were not really > error conditions, right? Right. This is an edge case which can get hit but doesn't result in any security or correct bug. > > Source/WebCore/ChangeLog:11 > > + attribute of the form associated element. When there are other elements with the same ID in the removed tree, > > I feel like this first sentence is missing something. What happens with the > very first element with the ID of the form content attribute? Is this just > missing "...form associated element was removed. When there are ..."? Will revise. > > Source/WebCore/dom/ContainerNodeAlgorithms.h:29 > > +// FIXME: Delete this class after fixing FormAssociatedElement to avoid calling getElementById during a tree removal. > > Should this be a Bugzilla with a number in the FIXME? I don't think it's worth the effort given we have a goal of fixing form associated elements in general. I can't think of a way of fixing this without rewriting the entire mechanism anyway.
Revised the first paragraph of the change log to: The assertion failure was caused by FormAssociatedElement::findAssociatedForm calling TreeScope::getElementById for a form associated element inside FormAttributeTargetObserver::idTargetChanged during the removal of the owner form element, or the first non-form element with the matching ID. If there are other elements with the same ID in the removed tree at that moment, MapEntry's count for the ID can be higher than it needs to be since Element::removedFromAncestor has not been called on those elements yet.
Committed r226095: <https://trac.webkit.org/changeset/226095>