The SVGUseElement is already removed from the set Document::m_svgUseElements and we are trying to remove it again. Here is the call stack: 0 WebCore 0x000000018e9a8720 WebCore::Document::removeSVGUseElement(WebCore::SVGUseElement&) + 44 (Document.cpp:5287) 1 WebCore 0x000000018e9a8710 WebCore::Document::removeSVGUseElement(WebCore::SVGUseElement&) + 28 (Document.cpp:5286) 2 WebCore 0x000000018f1936a4 WebCore::SVGUseElement::updateShadowTree() + 124 (SVGUseElement.cpp:241) 3 WebCore 0x000000018e99854c WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType) + 192 (Document.cpp:1799) 4 WebCore 0x000000018ddd5554 WebCore::Document::updateStyleIfNeeded() + 252 (Document.cpp:1965) 5 WebCore 0x000000018ddd10a0 WebCore::Document::finishedParsing() + 260 (Document.cpp:5390) 6 WebCore 0x000000018ddcfc40 WebCore::HTMLDocumentParser::prepareToStopParsing() + 172 (HTMLDocumentParser.cpp:406) 7 WebCore 0x000000018ddcfb54 WebCore::HTMLDocumentParser::finish() + 224 (HTMLDocumentParser.cpp:427) 8 WebCore 0x000000018ddcf370 WebCore::DocumentWriter::end() + 92 (DocumentWriter.cpp:276) 9 WebCore 0x000000018ec7aa58 WebCore::DocumentLoader::finishedLoading() + 472 (DocumentLoader.cpp:429) 10 WebCore 0x000000018de0111c WebCore::CachedResource::checkNotify() + 324 (CachedResource.cpp:348) 11 WebCore 0x000000018ecdd3bc WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 364 (CachedResource.cpp:364) 12 WebCore 0x000000018ecb9a40 WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&) + 520 (SubresourceLoader.cpp:589) 13 WebKit 0x00000001957bb53c WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&) + 220 (WebResourceLoader.cpp:150) 14 WebKit 0x00000001957bc2b0 void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)) + 92 (HandleMessage.h:40) 15 WebKit 0x00000001955c85dc WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 416 (NetworkProcessConnection.cpp:69) 16 WebKit 0x000000019553dc38 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 164 (Connection.cpp:901) 17 WebKit 0x0000000195540628 IPC::Connection::dispatchOneMessage() + 232 (Connection.cpp:959) 18 JavaScriptCore 0x000000018d27290c WTF::RunLoop::performWork() + 348 (Function.h:56) 19 JavaScriptCore 0x000000018d272b7c WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38) 20 CoreFoundation 0x00000001853e09f4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24 (CFRunLoop.c:1982) 21 CoreFoundation 0x00000001853e021c __CFRunLoopDoSources0 + 276 (CFRunLoop.c:2017) 22 CoreFoundation 0x00000001853ddd8c __CFRunLoopRun + 1204 (CFRunLoop.c:2920) 23 CoreFoundation 0x00000001852fe498 CFRunLoopRunSpecific + 552 (CFRunLoop.c:3245) 24 Foundation 0x0000000185d740e4 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 304 (NSRunLoop.m:367) 25 Foundation 0x0000000185dc5d4c -[NSRunLoop(NSRunLoop) run] + 88 (NSRunLoop.m:389) 26 libxpc.dylib 0x00000001850aabdc _xpc_objc_main + 516 (main.m:167) 27 libxpc.dylib 0x00000001850accb0 xpc_main + 180 (init.c:1476) 28 com.apple.WebKit.WebContent 0x00000001008db5ac main + 380 (XPCServiceMain.mm:148) 29 libdyld.dylib 0x0000000184d9dfc0 start + 4
<rdar://problem/36689240>
Created attachment 332456 [details] Patch
Comment on attachment 332456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332456&action=review r- because I don't think this approach is desirable. > Source/WebCore/ChangeLog:30 > + An assertion will fire if Document::resolveStyle() calls updateShadowTree() > + for an SVGUseElement and as a side effect it causes removedFromAncestor() > + to be called for an element which is also exist in Document::m_svgUseElements > + and has not been processed yet. Sounds like we should be able to make a test case for this. > Source/WebCore/ChangeLog:33 > + The fix is to replace SVGUseElement::m_shadowTreeNeedsUpdate by a function > + which checks whether the element is in Document::m_svgUseElements. By doing This will make the removal of a SVG element a lot more expensive. I don't think we should do that. > Source/WebCore/ChangeLog:65 > + (WebCore::SVGUseElement::updateShadowTree): No need to check isConnected(). > + here. This function is called only from Document::resolveStyle(). Make the > + call to removeShadowTreeNeedsUpdate() happens the last thing to ensure > + no invalidate-update loops will happen. What is a no invalidate-update loop? > Source/WebCore/dom/Document.cpp:1801 > + if (m_invalidatedSVGUseElements.contains(element.get())) > + element->updateShadowTree(); A better fix would be for this code to be checking isConnected() & shadowTreeNeedsUpdate() on each element before calling updateShadowTree(). > Source/WebCore/dom/Document.cpp:5288 > - RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(result.isNewEntry); > + auto result = m_invalidatedSVGUseElements.add(&element); > + ASSERT_UNUSED(result, result.isNewEntry); Replacing a release assert by a debug assert would only pamper over a real issue. > Source/WebCore/dom/Document.cpp:5294 > + bool didRemove = m_invalidatedSVGUseElements.remove(&element); > + ASSERT_UNUSED(didRemove, didRemove); Ditto.
Created attachment 332733 [details] Patch
Comment on attachment 332456 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=332456&action=review What exactly is not desirable in this patch? Do you consider using HashSet<>::contains() to be a lot more expensive than accessing a data member? >> Source/WebCore/ChangeLog:30 >> + and has not been processed yet. > > Sounds like we should be able to make a test case for this. I could not find a test case for this crash. >> Source/WebCore/ChangeLog:33 >> + which checks whether the element is in Document::m_svgUseElements. By doing > > This will make the removal of a SVG element a lot more expensive. I don't think we should do that. Checking a HashSet should be O(1). and SVGDocumentExtensions has many HashMaps for animations, resource loading and SVGUseElement dependency graph. >> Source/WebCore/ChangeLog:65 >> + no invalidate-update loops will happen. > > What is a no invalidate-update loop? I meant if updating an element causes invalidating itself, this will cause an infinite loop of invalidate and update. >> Source/WebCore/dom/Document.cpp:1801 >> + element->updateShadowTree(); > > A better fix would be for this code to be checking isConnected() & shadowTreeNeedsUpdate() on each element before calling updateShadowTree(). I added if (!shadowTreeNeedsUpdate()) at the beginning of SVGUseElement::updateShadowTree() to be the opposite of SVGUseElement::invalidateShadowTree() which starts by returning if (!isConnected() || shadowTreeNeedsUpdate()). >> Source/WebCore/dom/Document.cpp:5288 >> + ASSERT_UNUSED(result, result.isNewEntry); > > Replacing a release assert by a debug assert would only pamper over a real issue. I replaced this debug assertions with release assertions as they we were before.
This is not a security bug.
(In reply to Said Abou-Hallawa from comment #5) > Comment on attachment 332456 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=332456&action=review > > What exactly is not desirable in this patch? Do you consider using > HashSet<>::contains() to be a lot more expensive than accessing a data > member? Yes. It's considerably more expensive. Also, rewriting the way hash map works completely isn't a way to address this crash we don't even have a test case for. We really need a more thorough analysis of what might be happening instead of rewriting the entire thing.
This bug doesn't not address the root cause of the release assertion.
Reopening to attach new patch.
Created attachment 339285 [details] Removes the release assert
Attachment 339285 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:23: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 339286 [details] Fixed typos
Attachment 339286 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:22: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use-after-free [changelog/unwantedsecurityterms] [3] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r231267: <https://trac.webkit.org/changeset/231267>