Attachment 339285 Details for Bug 182188 – Removes the release assert

[patch] Removes the release assert

bug-182188-20180501235927.patch (text/plain), 2.89 KB, created by Ryosuke Niwa on 2018-05-01 23:59:27 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-05-01 23:59:27 PDT

Size: 2.89 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 231225)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,34 @@
>+2018-05-01  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        REGRESSION(r225868): Release assert when removing an SVGUseElement from Document::m_svgUseElements
>+        https://bugs.webkit.org/show_bug.cgi?id=182188
>+        <rdar://problem/36689240>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fixed the crash by removing up the release assert.
>+
>+        The crash is likely caused by re-entrancy to Document::resolveStyle during SVGUseElement::updateShadowTree.
>+        Because Document::resolveStyle invokes updateShadowTree on SVG use elements in Document::m_svgUseElements
>+        without clearing the map, the nested call to resolveStyle ends up calling updateShadowTree() for all elements
>+        in m_svgUseElements and removing them all from the map. When the stack frame eventually comes back to the outer
>+        invocation of Document::resolveStyle, updateShadowTree gets invoked for the second time on SVG use elements
>+        which whose shadow tree had already been updated within the inner invocation to updateShadowTree, and hits
>+        the release assertion.
>+
>+        There is an alternative fix: avoid calling updateShadowTree on a svg element when shadowTreeNeedsUpdate returns
>+        true on the element in resolveStyle. However, removing the relese assert is a sure way to fix the crash so
>+        this patch opts for that fix instead especially since we don't have any reproducible test case for this crash.
>+
>+        This release assertion was added in r225868 as a cautious measure to catch any use-after-frees of SVGUseElement's
>+        since m_svgUseElements stored raw pointes to SVG use elements but this crash is not an indicative of any UAF,
>+        and there is no evidence that r225868 has led to new UAFs even after five months.
>+
>+        No new tests. I couldn't find a way to trigger a nested style update inside SVGUseElement::updateShadowTree.
>+
>+        * dom/Document.cpp:
>+        (WebCore::Document::removeSVGUseElement):
>+
> 2018-05-01  Yusuke Suzuki  <utatane.tea@gmail.com>
> 
>         Use default std::optional if it is provided
>Index: Source/WebCore/dom/Document.cpp
>===================================================================
>--- Source/WebCore/dom/Document.cpp	(revision 231220)
>+++ Source/WebCore/dom/Document.cpp	(working copy)
>@@ -5322,8 +5322,8 @@ void Document::addSVGUseElement(SVGUseEl
> 
> void Document::removeSVGUseElement(SVGUseElement& element)
> {
>-    bool didRemove = m_svgUseElements.remove(&element);
>-    RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(didRemove);
>+    m_svgUseElements.remove(&element);
>+    // FIXME: Assert that element was in m_svgUseElements once re-entrancy to update style and layout have been removed.
> }
> 
> bool Document::hasSVGRootNode() const

Actions: View | Formatted Diff | Diff

Attachments on bug 182188: 332456 | 332733 | 339285 | 339286