- Revision
- 264364
- Author
- commit-qu...@webkit.org
- Date
- 2020-07-14 11:59:37 -0700 (Tue, 14 Jul 2020)
Log Message
When invalidating the clients of an SVG resource we should not go beyond the RenderSVGRoot
https://bugs.webkit.org/show_bug.cgi?id=211804
<rdar://problem/60308199>
Patch by Said Abou-Hallawa <sabouhall...@apple.com> on 2020-07-14
Reviewed by Zalan Bujtas.
Source/WebCore:
ComplexLineLayout::layoutLineBoxes() walks through the renderers of a line.
The order of this walk may include a node and its children. For example
the layout of a RenderInline may be run first, then it is followed by the
layout for a RenderSVGRoot which happens to be a child of the RenderInline.
If the RenderSVGRoot has a dirty RenderSVGResource, e.g. RenderSVGResourceClipper,
this resource will call RenderSVGResource::markForLayoutAndParentResourceInvalidation()
which will invalidate its ancestors including RenderSVGRoot and RenderInline
by setting the normalChildNeedsLayoutBit() for each of them.
The layout of SVG is hierarchical which means RenderSVGRoot will finish
its layout after finishing the layout of all its descendants including
this RenderSVGResource. So dirtying the RenderSVGRoot is this scenario
is okay since RenderSVGRoot will do another SVGRenderSupport::layoutChildren()
and will clear its needsLayout bits before it returns.
The problem happens because we set normalChildNeedsLayoutBit for the containing
RenderInline and this leaves the render tree dirty. Later Document::resolveStyle()
may called to invalidate an SVG element e.g. RenderSVGPath. So setNeedsLayout()
is called for this object. Because the normalChildNeedsLayoutBit() is set
for the RenderInline, RenderObject::markContainingBlocksForLayout() stops
in the middle and do not mark the containing RenderBlock. So we end up
with a render tree like this:
+ RenderView
+ HTML RenderBlock
+ BODY RenderBody
- RenderBlock
+ ANY-ELEMENT RenderInline
+ svg RenderSVGRoot
- clipPath RenderSVGResourceClipper
+ polygon RenderSVGPath
where the '+' means needsLayout() is true and '-' means needsLayout() is
false.
So the layout will not run for RenderBlock with '-' sign. And we end up
with dirty RenderSVGPath or even worse RenderSVGPath with uninitialized
m_path. So a null pointer deref may happen.
The fix is to prevent mutating the render tree while running the layout
of the SVG resource. This can be done by making the RenderSVGResource not
dirtying any renderer beyond the RenderSVGRoot when it finishes its layout.
The SVG resource layout should not affect the intrinsic size of the SVG
if it is embedded in an HTML document.
In RenderObject::markContainingBlocksForLayout(), we do something similar
when we break if the ancestor is objectIsRelayoutBoundary().
Test: svg/in-html/inline-svg-resource-dynamic-update.html
* rendering/svg/RenderSVGResource.cpp:
(WebCore::RenderSVGResource::markForLayoutAndParentResourceInvalidation):
* rendering/svg/RenderSVGRoot.cpp:
(WebCore::RenderSVGRoot::layout):
* rendering/svg/RenderSVGRoot.h:
LayoutTests:
* svg/in-html/inline-svg-resource-dynamic-update-expected.txt: Added.
* svg/in-html/inline-svg-resource-dynamic-update.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (264363 => 264364)
--- trunk/LayoutTests/ChangeLog 2020-07-14 18:55:16 UTC (rev 264363)
+++ trunk/LayoutTests/ChangeLog 2020-07-14 18:59:37 UTC (rev 264364)
@@ -1,3 +1,14 @@
+2020-07-14 Said Abou-Hallawa <sabouhall...@apple.com>
+
+ When invalidating the clients of an SVG resource we should not go beyond the RenderSVGRoot
+ https://bugs.webkit.org/show_bug.cgi?id=211804
+ <rdar://problem/60308199>
+
+ Reviewed by Zalan Bujtas.
+
+ * svg/in-html/inline-svg-resource-dynamic-update-expected.txt: Added.
+ * svg/in-html/inline-svg-resource-dynamic-update.html: Added.
+
2020-07-14 Hector Lopez <hector_i_lo...@apple.com>
[ WK1 Mac ] http/tests/xmlhttprequest/access-control-basic-allow-preflight-cache.html is a flaky failure
Added: trunk/LayoutTests/svg/in-html/inline-svg-resource-dynamic-update-expected.txt (0 => 264364)
--- trunk/LayoutTests/svg/in-html/inline-svg-resource-dynamic-update-expected.txt (rev 0)
+++ trunk/LayoutTests/svg/in-html/inline-svg-resource-dynamic-update-expected.txt 2020-07-14 18:59:37 UTC (rev 264364)
@@ -0,0 +1,2 @@
+Test PASSES if it does not crash.
+
Added: trunk/LayoutTests/svg/in-html/inline-svg-resource-dynamic-update.html (0 => 264364)
--- trunk/LayoutTests/svg/in-html/inline-svg-resource-dynamic-update.html (rev 0)
+++ trunk/LayoutTests/svg/in-html/inline-svg-resource-dynamic-update.html 2020-07-14 18:59:37 UTC (rev 264364)
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<script>
+ if (window.testRunner)
+ testRunner.dumpAsText();
+
+ function jsfuzzer() {
+ container.getBoundingClientRect();
+ clipPath.setAttribute("text-decoration", "underline");
+ // dirty layout
+ container.prepend(' it does not crash.');
+ container.getBoundingClientRect();
+ polygon.style.setProperty("column-span", "all");
+ // dirty layout
+ container.prepend('Test PASSES if');
+ return polygon.isPointInStroke(undefined);
+ }
+</script>
+<body _onload_=jsfuzzer()>
+ <any-element id="container">
+ <div></div>
+ <svg clip-path="url(#clipPath)" font-size="0px" stroke="url(#)">
+ <clipPath id="clipPath" />
+ <polygon id="polygon" y2="0" />
+ </svg>
+ </any-element>
+</body>
Modified: trunk/Source/WebCore/ChangeLog (264363 => 264364)
--- trunk/Source/WebCore/ChangeLog 2020-07-14 18:55:16 UTC (rev 264363)
+++ trunk/Source/WebCore/ChangeLog 2020-07-14 18:59:37 UTC (rev 264364)
@@ -1,3 +1,68 @@
+2020-07-14 Said Abou-Hallawa <sabouhall...@apple.com>
+
+ When invalidating the clients of an SVG resource we should not go beyond the RenderSVGRoot
+ https://bugs.webkit.org/show_bug.cgi?id=211804
+ <rdar://problem/60308199>
+
+ Reviewed by Zalan Bujtas.
+
+ ComplexLineLayout::layoutLineBoxes() walks through the renderers of a line.
+ The order of this walk may include a node and its children. For example
+ the layout of a RenderInline may be run first, then it is followed by the
+ layout for a RenderSVGRoot which happens to be a child of the RenderInline.
+
+ If the RenderSVGRoot has a dirty RenderSVGResource, e.g. RenderSVGResourceClipper,
+ this resource will call RenderSVGResource::markForLayoutAndParentResourceInvalidation()
+ which will invalidate its ancestors including RenderSVGRoot and RenderInline
+ by setting the normalChildNeedsLayoutBit() for each of them.
+
+ The layout of SVG is hierarchical which means RenderSVGRoot will finish
+ its layout after finishing the layout of all its descendants including
+ this RenderSVGResource. So dirtying the RenderSVGRoot is this scenario
+ is okay since RenderSVGRoot will do another SVGRenderSupport::layoutChildren()
+ and will clear its needsLayout bits before it returns.
+
+ The problem happens because we set normalChildNeedsLayoutBit for the containing
+ RenderInline and this leaves the render tree dirty. Later Document::resolveStyle()
+ may called to invalidate an SVG element e.g. RenderSVGPath. So setNeedsLayout()
+ is called for this object. Because the normalChildNeedsLayoutBit() is set
+ for the RenderInline, RenderObject::markContainingBlocksForLayout() stops
+ in the middle and do not mark the containing RenderBlock. So we end up
+ with a render tree like this:
+
+ + RenderView
+ + HTML RenderBlock
+ + BODY RenderBody
+ - RenderBlock
+ + ANY-ELEMENT RenderInline
+ + svg RenderSVGRoot
+ - clipPath RenderSVGResourceClipper
+ + polygon RenderSVGPath
+
+ where the '+' means needsLayout() is true and '-' means needsLayout() is
+ false.
+
+ So the layout will not run for RenderBlock with '-' sign. And we end up
+ with dirty RenderSVGPath or even worse RenderSVGPath with uninitialized
+ m_path. So a null pointer deref may happen.
+
+ The fix is to prevent mutating the render tree while running the layout
+ of the SVG resource. This can be done by making the RenderSVGResource not
+ dirtying any renderer beyond the RenderSVGRoot when it finishes its layout.
+ The SVG resource layout should not affect the intrinsic size of the SVG
+ if it is embedded in an HTML document.
+
+ In RenderObject::markContainingBlocksForLayout(), we do something similar
+ when we break if the ancestor is objectIsRelayoutBoundary().
+
+ Test: svg/in-html/inline-svg-resource-dynamic-update.html
+
+ * rendering/svg/RenderSVGResource.cpp:
+ (WebCore::RenderSVGResource::markForLayoutAndParentResourceInvalidation):
+ * rendering/svg/RenderSVGRoot.cpp:
+ (WebCore::RenderSVGRoot::layout):
+ * rendering/svg/RenderSVGRoot.h:
+
2020-07-14 Per Arne Vollan <pvol...@apple.com>
[iOS] Avoid loading media libraries in Document::visibilityStateChanged() if not needed
Modified: trunk/Source/WebCore/rendering/svg/RenderSVGResource.cpp (264363 => 264364)
--- trunk/Source/WebCore/rendering/svg/RenderSVGResource.cpp 2020-07-14 18:55:16 UTC (rev 264363)
+++ trunk/Source/WebCore/rendering/svg/RenderSVGResource.cpp 2020-07-14 18:59:37 UTC (rev 264364)
@@ -29,6 +29,7 @@
#include "RenderSVGResourceFilter.h"
#include "RenderSVGResourceMasker.h"
#include "RenderSVGResourceSolidColor.h"
+#include "RenderSVGRoot.h"
#include "RenderView.h"
#include "SVGResources.h"
#include "SVGResourcesCache.h"
@@ -190,8 +191,14 @@
{
ASSERT(object.node());
- if (needsLayout && !object.renderTreeBeingDestroyed())
- object.setNeedsLayout();
+ if (needsLayout && !object.renderTreeBeingDestroyed()) {
+ // If we are inside the layout of an RenderSVGRoot, do not cross the SVG boundary to
+ // invalidate the ancestor renderer because it may have finished its layout already.
+ if (is<RenderSVGRoot>(object) && downcast<RenderSVGRoot>(object).isInLayout())
+ object.setNeedsLayout(MarkOnlyThis);
+ else
+ object.setNeedsLayout(MarkContainingBlockChain);
+ }
if (is<RenderElement>(object))
removeFromCacheAndInvalidateDependencies(downcast<RenderElement>(object), needsLayout);
Modified: trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp (264363 => 264364)
--- trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp 2020-07-14 18:55:16 UTC (rev 264363)
+++ trunk/Source/WebCore/rendering/svg/RenderSVGRoot.cpp 2020-07-14 18:59:37 UTC (rev 264364)
@@ -46,6 +46,7 @@
#include "SVGViewSpec.h"
#include "TransformState.h"
#include <wtf/IsoMallocInlines.h>
+#include <wtf/SetForScope.h>
#include <wtf/StackStats.h>
namespace WebCore {
@@ -54,7 +55,6 @@
RenderSVGRoot::RenderSVGRoot(SVGSVGElement& element, RenderStyle&& style)
: RenderReplaced(element, WTFMove(style))
- , m_objectBoundingBoxValid(false)
, m_isLayoutSizeChanged(false)
, m_needsBoundariesOrTransformUpdate(true)
, m_hasBoxDecorations(false)
@@ -141,6 +141,7 @@
void RenderSVGRoot::layout()
{
+ SetForScope<bool> change(m_inLayout, true);
StackStats::LayoutCheckPoint layoutCheckPoint;
ASSERT(needsLayout());
Modified: trunk/Source/WebCore/rendering/svg/RenderSVGRoot.h (264363 => 264364)
--- trunk/Source/WebCore/rendering/svg/RenderSVGRoot.h 2020-07-14 18:55:16 UTC (rev 264363)
+++ trunk/Source/WebCore/rendering/svg/RenderSVGRoot.h 2020-07-14 18:59:37 UTC (rev 264364)
@@ -46,6 +46,7 @@
void computeIntrinsicRatioInformation(FloatSize& intrinsicSize, double& intrinsicRatio) const override;
bool isLayoutSizeChanged() const { return m_isLayoutSizeChanged; }
+ bool isInLayout() const { return m_inLayout; }
void setNeedsBoundariesUpdate() override { m_needsBoundariesOrTransformUpdate = true; }
bool needsBoundariesUpdate() override { return m_needsBoundariesOrTransformUpdate; }
void setNeedsTransformUpdate() override { m_needsBoundariesOrTransformUpdate = true; }
@@ -106,7 +107,8 @@
IntSize m_containerSize;
FloatRect m_objectBoundingBox;
- bool m_objectBoundingBoxValid;
+ bool m_objectBoundingBoxValid { false };
+ bool m_inLayout { false };
FloatRect m_strokeBoundingBox;
FloatRect m_repaintBoundingBox;
FloatRect m_repaintBoundingBoxExcludingShadow;
