From 49ad697403f7031f292924096851b498f788bbfd Mon Sep 17 00:00:00 2001 From: Javier Fernandez Date: Mon, 7 Mar 2016 15:05:43 +0100 Subject: Breaking several cyclic references between SVG animated properties. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Based on upstream fix by sabouhallawa@apple.com at http://trac.webkit.org/changeset/196268 The leak happens because of cyclic reference between SVGListPropertyTearOff and SVGAnimatedListPropertyTearOff which is derived from SVGAnimatedProperty. There is also cyclic reference between SVGAnimatedProperty and SVGElement and this causes the whole document to be leaked. So if the JS requests, for example, an instance of SVGPolylineElement.points, the whole document will be leaked. The fix depends on having the cyclic reference as is since the owning and the owned classes have to live together if any of them is referenced. But the owning class caches a raw 'ref-counted' pointer of the owned class. If it is requested for an instance of the owned class it returned a RefPtr<> of it. Once the owned class is not used, it can delete itself. The only thing needed here is to notify the owner class of the deletion so it cleans its caches and be able to create a new pointer if it is requested for an instance of the owned class later. Revert the change of r181345 in SVGAnimatedProperty::lookupOrCreateWrapper() to break the cyclic reference between SVGElement and SVGAnimatedProperty. Also apply the same approach in SVGAnimatedListPropertyTearOff::baseVal() and animVal() to break cyclic reference between SVGListPropertyTearOff and SVGAnimatedListPropertyTearOff. Change-Id: Ied6a077299e47855feb235a1c9310f1a58aad91b Reviewed-by: Konstantin Tokarev Reviewed-by: Michael Brüning --- Source/WebCore/svg/SVGPathElement.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'Source/WebCore/svg/SVGPathElement.h') diff --git a/Source/WebCore/svg/SVGPathElement.h b/Source/WebCore/svg/SVGPathElement.h index c81ead198..0ca66ac11 100644 --- a/Source/WebCore/svg/SVGPathElement.h +++ b/Source/WebCore/svg/SVGPathElement.h @@ -83,10 +83,10 @@ public: PassRefPtr createSVGPathSegCurvetoQuadraticSmoothRel(float x, float y, SVGPathSegRole role = PathSegUndefinedRole); // Used in the bindings only. - SVGPathSegListPropertyTearOff* pathSegList(); - SVGPathSegListPropertyTearOff* animatedPathSegList(); - SVGPathSegListPropertyTearOff* normalizedPathSegList(); - SVGPathSegListPropertyTearOff* animatedNormalizedPathSegList(); + RefPtr pathSegList(); + RefPtr animatedPathSegList(); + RefPtr normalizedPathSegList(); + RefPtr animatedNormalizedPathSegList(); SVGPathByteStream* pathByteStream() const; -- cgit v1.2.1 From 7205faf1a546a690f68176989100109e9a3335b7 Mon Sep 17 00:00:00 2001 From: Javier Fernandez Date: Mon, 7 Mar 2016 23:56:13 +0100 Subject: Many assertion failures and crashes on SVG path animation cases when JS garbage collection happens quickly. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Based on upstream fix by said@apple.com at http://trac.webkit.org/changeset/197125 Since the whole document was leaking once an SVGAnimatedProperty was created so there was no way to produce this bug. After fixing the leak, one crash and one assert got uncovered. Both of them happen because of the fact: "if an SVGAnimatedProperty is not referenced it will be deleted." * svg/SVGPathElement.cpp: (WebCore::SVGPathElement::lookupOrCreateDWrapper): The code in this function was assuming that the wrapper will be created only once which happens when SVGAnimatedProperty::lookupOrCreateWrapper() is called. Before making this single call, lookupOrCreateDWrapper() was building an initial SVGPathSegList from byte stream. But now SVGAnimatedProperty::lookupWrapper() can return false even after creating the SVGAnimatedProperty because it was deleted later. Calling buildSVGPathSegListFromByteStream() more than once was causing SVGAnimatedListPropertyTearOff::animationStarted() to fire the assertion ASSERT(m_values.size() == m_wrappers.size()) because the path segments were appended twice to m_values which is in fact SVGPathElement::m_pathSegList.value. The fix is to build the initial SVGPathSegList only once which should happen when m_pathSegList.value.isEmpty(). (WebCore::SVGPathElement::animatedPropertyWillBeDeleted): * svg/SVGPathElement.h: * svg/properties/SVGAnimatedPathSegListPropertyTearOff.h: (WebCore::SVGAnimatedPathSegListPropertyTearOff::~SVGAnimatedPathSegListPropertyTearOff): SVGPathElement is assuming the following equivalence relation: m_pathSegList.shouldSynchronize ~ SVGAnimatedProperty_is_created_and_not_null. SVGPathElement::animatedPathSegList() and animatedNormalizedPathSegList() set m_pathSegList.shouldSynchronize to true when SVGAnimatedProperty is created but nothing sets m_pathSegList.shouldSynchronize back to false. This was not a problem when the SVGAnimatedProperty was leaking but after ensuring it is deleted when it is not referenced this equivalence relation becomes untrue sometimes. This caused SVGPathElement::svgAttributeChanged() to crash when we check m_pathSegList.shouldSynchronize and if it is true we assume that SVGAnimatedProperty::lookupWrapper() will return a non-null pointer and therefore we deference this pointer and call SVGAnimatedProperty::isAnimating(). To fix this crash we need to set m_pathSegList.shouldSynchronize back to false when the associated SVGAnimatedProperty is deleted. Change-Id: I05be755635b02d0d76105fc2eb21c2f013298c4e Reviewed-by: Michael Brüning --- Source/WebCore/svg/SVGPathElement.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'Source/WebCore/svg/SVGPathElement.h') diff --git a/Source/WebCore/svg/SVGPathElement.h b/Source/WebCore/svg/SVGPathElement.h index 0ca66ac11..e9db8a56e 100644 --- a/Source/WebCore/svg/SVGPathElement.h +++ b/Source/WebCore/svg/SVGPathElement.h @@ -98,7 +98,9 @@ public: bool isAnimValObserved() const { return m_isAnimValObserved; } -private: + void animatedPropertyWillBeDeleted(); + + private: SVGPathElement(const QualifiedName&, Document*); virtual bool isValid() const { return SVGTests::isValid(); } -- cgit v1.2.1