| Summary: | REGRESSION(r181345): SVG polyline and polygon leak page | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Vienneau <chris.vno> | ||||||||||||
| Component: | SVG | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | commit-queue, darin, nakim, nikos.andronikos, sabouhallawa, simon.fraser, thorton, webkit-bug-importer, zimmermann | ||||||||||||
| Priority: | P2 | Keywords: | InRadar, Regression | ||||||||||||
| Version: | WebKit Local Build | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows 7 | ||||||||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=151810 https://bugs.webkit.org/show_bug.cgi?id=83856 https://bugs.webkit.org/show_bug.cgi?id=114280 |
||||||||||||||
| Attachments: |
|
||||||||||||||
Chris, can you attach a test case to this bug please? Created attachment 268332 [details]
Repro Page
Created attachment 268333 [details]
Image for repro page
I’ve attached a repro page, which is pretty basic, I think the real trick here is recognizing that anything is leaking. I suggest you ensure that page caching and memory cache are off, this will make the leak more predictable and apparent. Breakpoints on SVGAnimatedProperty
and ~SVGAnimatedProperty will show the basic problem. On my local build I have quite a lot of instrumentation at the moment; for example, I enabled DUMP_NODE_STATISTICS and tweaked the code a bit, with breakpoints in:
WebCore\dom\Node.cpp
{code}
#if DUMP_NODE_STATISTICS
if (this->m_nodeFlags & IsSVGFlag)
{
DbgPrint("Added svg node:%p\n", this);
}
liveNodeSet.add(this);
#endif
}
{code}
{code}
#if DUMP_NODE_STATISTICS
if (this->m_nodeFlags & IsSVGFlag)
{
DbgPrint("Removed svg node:%p\n", this);
}
liveNodeSet.remove(this);
#endif
{code}
You’ll be able to see two svg nodes added when you visit the page, and one svg node removed when you navigate away from the page.
I initially stated that polyline and polygon leak, I now believe in fact any svg node that creates a SVGAnimatedProperty will leak. It just so happens that polyline and polygon always create it. In older code based on based on 157437 the SVGAnimatedProperty would be created less often for polyline and polygon, additionally when it was created it didn't leak. For now i'm using a workaround by restoring WebCore\rendering\svg\SVGPathData.cpp updatePathFromPolygonElement() and updatePathFromPolylineElement() to their older version in order to hit the failure case less often. I'm also immediately releasing the reference kept on the "SVGElement* contextElement" in SVGAnimatedProperty constructor; so when the error case happens we don't leak the whole page (only the SVGAnimatedProperty). This greatly reduces the memory leaked and our soaks come out pretty good now. and I haven't encountered any issue with releasing the ref early. I'm moving on to other more urgent issues for the time being, these workarounds I'm using i only consider temporary, I'd be interested in a better fix if one becomes available. Chris Said mentioned this in email: This seems to be a reference cycle between SVGAnimatedListPropertyTearOff and SVGListPropertyTearOff. In SVGAnimatedListPropertyTearOff::animVal(), m_animVal is assigned to a new Ref<SVGListPropertyTearOff> but this new Ref increments the refcount of this. This looks similar to https://bugs.webkit.org/show_bug.cgi?id=151810. (In reply to comment #6) > Said mentioned this in email: > > This seems to be a reference cycle between SVGAnimatedListPropertyTearOff > and SVGListPropertyTearOff. In SVGAnimatedListPropertyTearOff::animVal(), > m_animVal is assigned to a new Ref<SVGListPropertyTearOff> but this new Ref > increments the refcount of this. This looks similar to > https://bugs.webkit.org/show_bug.cgi?id=151810. Thanks, i'll watch that bug too. (In reply to comment #5) > I initially stated that polyline and polygon leak, I now believe in fact any > svg node that creates a SVGAnimatedProperty will leak. It just so happens > that polyline and polygon always create it. In older code based on based on > 157437 the SVGAnimatedProperty would be created less often for polyline and > polygon, additionally when it was created it didn't leak. > > For now i'm using a workaround by restoring > WebCore\rendering\svg\SVGPathData.cpp updatePathFromPolygonElement() and > updatePathFromPolylineElement() to their older version in order to hit the > failure case less often. I'm also immediately releasing the reference kept > on the "SVGElement* contextElement" in SVGAnimatedProperty constructor; so > when the error case happens we don't leak the whole page (only the > SVGAnimatedProperty). > > This greatly reduces the memory leaked and our soaks come out pretty good > now. and I haven't encountered any issue with releasing the ref early. I'm > moving on to other more urgent issues for the time being, these workarounds > I'm using i only consider temporary, I'd be interested in a better fix if > one becomes available. > > Chris Yes the SVGAnimatedProperty leak happens because of the reference cycle between SVGListPropertyTearOff and SVGAnimatedListPropertyTearOff (which is derived form SVGAnimatedProperty). But the document leak happens because we cache the SVGAnimatedProperty in a global cache which SVGAnimatedProperty::animatedPropertyCache() returns. The entries of this cache never gets deleted. And because the SVGAnimatedProperty has "RefPtr<SVGElement> m_contextElement", the SVGElement ref count never get to zero so the whole document can't be deleted. This cache is required since it is the place the property value keep changing while animation. But having it in a global cache without notification from the garbage collector that the property wrapper is no longer needed is wrong. I think the cached wrappers have to be stored with the SVGElement itself. Once all the wrappers are not referenced anymore, the SVGElement itself can be deleted and hence all the cached wrappers can be deleted also. Created attachment 270630 [details]
Patch
Attachment 270630 [details] did not pass style-queue:
ERROR: Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:126: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 1 in 19 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270630&action=review review- because of the problem in SVGPathSegWithContext::animatedProperty > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4251 > + if ($codeGenerator->IsSVGTypeNeedingTearOff($type) and $type =~ /(?<!String)List$/) { We normally try to avoid putting rules that are quite so specific into the code generator. Is there a future way to do this that’s less dependent on a regular expression and more on explicit flags in the IDL instead? > Source/WebCore/svg/SVGPathElement.cpp:354 > return 0; nullptr > Source/WebCore/svg/SVGPathSegWithContext.h:44 > + return SVGAnimatedProperty::lookupWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff>(m_element.get(), SVGPathElement::dPropertyInfo()).get(); I don’t understand how this can possibly be safe. Presumably lookupWrapper returns a RefPtr because the object it returns is new and might not have any other references. So how can it be safe to let that RefPtr be destroyed and return the raw pointer it was holding. > Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:49 > + return m_baseVal; This should say return property (or WTFMove(property)), to avoid unnecessary reference count churn. > Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:59 > + return m_animVal; This should say return property (or WTFMove(property)), to avoid unnecessary reference count churn. > Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:62 > + void propertyWillBeDeleted(ListProperty* property) This function should take a reference, not a pointer. > Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:77 > + return static_cast<ListPropertyTearOff*>(baseVal().get())->findItem(static_cast<ListItemTearOff*>(property)); Should use references here instead of pointers and avoid the .get(): return static_cast<ListPropertyTearOff&>(*baseVal()).findItem ... > Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:84 > + static_cast<ListPropertyTearOff*>(baseVal().get())->removeItemFromList(itemIndex, shouldSynchronizeWrappers); Ditto. > Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:95 > ASSERT(m_animVal); Peculiar to assert m_animVal but then use m_animatingAnimVal. > Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:41 > + return m_baseVal; Same comments as above. > Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:51 > + return m_animVal; Same comments as above. > Source/WebCore/svg/properties/SVGAnimatedTransformListPropertyTearOff.h:38 > + return m_baseVal; Same comment again. > Source/WebCore/svg/properties/SVGAnimatedTransformListPropertyTearOff.h:48 > + return m_animVal; Same comment again. Created attachment 270865 [details]
Patch
Attachment 270865 [details] did not pass style-queue:
ERROR: Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:126: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 1 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270865&action=review > Source/WebCore/svg/SVGViewSpec.cpp:153 > + return static_cast<SVGTransformListPropertyTearOff*>(static_reference_cast<SVGAnimatedTransformList>(lookupOrCreateTransformWrapper(this))->animVal().get()); Can we use static_pointer_cast here instead of using get()? Comment on attachment 270630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270630&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4251 >> + if ($codeGenerator->IsSVGTypeNeedingTearOff($type) and $type =~ /(?<!String)List$/) { > > We normally try to avoid putting rules that are quite so specific into the code generator. Is there a future way to do this that’s less dependent on a regular expression and more on explicit flags in the IDL instead? I agree. I logged https://bugs.webkit.org/show_bug.cgi?id=153994 and I kept my ugly change as is since the right fix of this issue is outside the scope of this bug. >> Source/WebCore/svg/SVGPathElement.cpp:354 >> return 0; > > nullptr Done. >> Source/WebCore/svg/SVGPathSegWithContext.h:44 >> + return SVGAnimatedProperty::lookupWrapper<SVGPathElement, SVGAnimatedPathSegListPropertyTearOff>(m_element.get(), SVGPathElement::dPropertyInfo()).get(); > > I don’t understand how this can possibly be safe. Presumably lookupWrapper returns a RefPtr because the object it returns is new and might not have any other references. So how can it be safe to let that RefPtr be destroyed and return the raw pointer it was holding. You are right. I changed the return value of this function to be RefPtr<>. So the caller will be guaranteed to have a valid pointer as long as it holds the RefPtr<>. >> Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:49 >> + return m_baseVal; > > This should say return property (or WTFMove(property)), to avoid unnecessary reference count churn. Done. >> Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:59 >> + return m_animVal; > > This should say return property (or WTFMove(property)), to avoid unnecessary reference count churn. Done. >> Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:62 >> + void propertyWillBeDeleted(ListProperty* property) > > This function should take a reference, not a pointer. Done. >> Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:77 >> + return static_cast<ListPropertyTearOff*>(baseVal().get())->findItem(static_cast<ListItemTearOff*>(property)); > > Should use references here instead of pointers and avoid the .get(): > > return static_cast<ListPropertyTearOff&>(*baseVal()).findItem ... Done. I used static_pointer_cast then I realized it is more expensive than just simply cast the raw reference. >> Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:84 >> + static_cast<ListPropertyTearOff*>(baseVal().get())->removeItemFromList(itemIndex, shouldSynchronizeWrappers); > > Ditto. Done. >> Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h:95 >> ASSERT(m_animVal); > > Peculiar to assert m_animVal but then use m_animatingAnimVal. Done. Now the assertion is ASSERT(m_animatingAnimVal) >> Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:41 >> + return m_baseVal; > > Same comments as above. Done. >> Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:51 >> + return m_animVal; > > Same comments as above. Done. >> Source/WebCore/svg/properties/SVGAnimatedTransformListPropertyTearOff.h:38 >> + return m_baseVal; > > Same comment again. Done, >> Source/WebCore/svg/properties/SVGAnimatedTransformListPropertyTearOff.h:48 >> + return m_animVal; > > Same comment again. Done. Created attachment 270868 [details]
Patch
Attachment 270868 [details] did not pass style-queue:
ERROR: Source/WebCore/svg/properties/SVGAnimatedPropertyMacros.h:126: Multi line control clauses should use braces. [whitespace/braces] [4]
Total errors found: 1 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 270865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270865&action=review >> Source/WebCore/svg/SVGViewSpec.cpp:153 >> + return static_cast<SVGTransformListPropertyTearOff*>(static_reference_cast<SVGAnimatedTransformList>(lookupOrCreateTransformWrapper(this))->animVal().get()); > > Can we use static_pointer_cast here instead of using get()? Done. I also did similar casting in SVGViewSpec::setTransformString(). Comment on attachment 270868 [details] Patch Clearing flags on attachment: 270868 Committed r196268: <http://trac.webkit.org/changeset/196268> All reviewed patches have been landed. Closing bug. |
The source of the leak appears to come from the below callstack. A cache of animation points is being created in SVGAnimatedProperty(SVGElement* contextElement, const QualifiedName& attributeName, AnimatedPropertyType animatedPropertyType), however the destructor for SVGAnimatedProperty is never called. The passed in contextElement gains a ref when the SVGAnimatedProperty is created, however I’m not seeing a code path where the animation points should be destroyed. This effects both svg polyline and polygon, and results in leaking the whole page. Note that i'm using code based off of change 188436. Thanks for any help you can provide, Chris Vienneau \WebCore\svg\properties\SVGAnimatedProperty.cpp SVGAnimatedProperty::SVGAnimatedProperty(SVGElement* contextElement, const QualifiedName& attributeName, AnimatedPropertyType animatedPropertyType) : m_contextElement(contextElement) , m_attributeName(attributeName) , m_animatedPropertyType(animatedPropertyType) , m_isAnimating(false) , m_isReadOnly(false) { } > EAWebKitd.dll!WebCore::SVGAnimatedProperty::SVGAnimatedProperty(WebCore::SVGElement * contextElement, const WebCore::QualifiedName & attributeName, WebCore::AnimatedPropertyType animatedPropertyType) Line 29 C++ EAWebKitd.dll!WebCore::SVGAnimatedListPropertyTearOff<WebCore::SVGPointList>::SVGAnimatedListPropertyTearOff<WebCore::SVGPointList>(WebCore::SVGElement * contextElement, const WebCore::QualifiedName & attributeName, WebCore::AnimatedPropertyType animatedPropertyType, WebCore::SVGPointList & values) Line 166 C++ EAWebKitd.dll!WebCore::SVGAnimatedListPropertyTearOff<WebCore::SVGPointList>::create(WebCore::SVGElement * contextElement, const WebCore::QualifiedName & attributeName, WebCore::AnimatedPropertyType animatedPropertyType, WebCore::SVGPointList & values) Line 159 C++ EAWebKitd.dll!WebCore::SVGAnimatedProperty::lookupOrCreateWrapper<WebCore::SVGPolyElement,WebCore::SVGAnimatedListPropertyTearOff<WebCore::SVGPointList>,WebCore::SVGPointList>(WebCore::SVGPolyElement * element, const WebCore::SVGPropertyInfo * info, WebCore::SVGPointList & property) Line 57 C++ EAWebKitd.dll!WebCore::SVGPolyElement::lookupOrCreatePointsWrapper(WebCore::SVGElement * contextElement) Line 117 C++ EAWebKitd.dll!WebCore::SVGPolyElement::animatedPoints() Line 130 C++ EAWebKitd.dll!WebCore::updatePathFromPolylineElement(WebCore::SVGElement * element, WebCore::Path & path) Line 106 C++ EAWebKitd.dll!WebCore::updatePathFromGraphicsElement(WebCore::SVGElement * element, WebCore::Path & path) Line 172 C++ EAWebKitd.dll!WebCore::RenderSVGShape::updateShapeFromElement() Line 84 C++ EAWebKitd.dll!WebCore::RenderSVGPath::updateShapeFromElement() Line 48 C++ EAWebKitd.dll!WebCore::RenderSVGShape::layout() Line 164 C++ EAWebKitd.dll!WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderElement & start, bool selfNeedsLayout) Line 281 C++ EAWebKitd.dll!WebCore::RenderSVGRoot::layout() Line 181 C++ EAWebKitd.dll!WebCore::RenderElement::layoutIfNeeded() Line 135 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutLineBoxes(bool relayoutChildren, WebCore::LayoutUnit & repaintLogicalTop, WebCore::LayoutUnit & repaintLogicalBottom) Line 1621 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutInlineChildren(bool relayoutChildren, WebCore::LayoutUnit & repaintLogicalTop, WebCore::LayoutUnit & repaintLogicalBottom) Line 652 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlock(bool relayoutChildren, WebCore::LayoutUnit pageLogicalHeight) Line 484 C++ EAWebKitd.dll!WebCore::RenderBlock::layout() Line 930 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox & child, WebCore::RenderBlockFlow::MarginInfo & marginInfo, WebCore::LayoutUnit & previousFloatLogicalBottom, WebCore::LayoutUnit & maxFloatLogicalBottom) Line 712 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlockChildren(bool relayoutChildren, WebCore::LayoutUnit & maxFloatLogicalBottom) Line 633 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlock(bool relayoutChildren, WebCore::LayoutUnit pageLogicalHeight) Line 488 C++ EAWebKitd.dll!WebCore::RenderBlock::layout() Line 930 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox & child, WebCore::RenderBlockFlow::MarginInfo & marginInfo, WebCore::LayoutUnit & previousFloatLogicalBottom, WebCore::LayoutUnit & maxFloatLogicalBottom) Line 712 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlockChildren(bool relayoutChildren, WebCore::LayoutUnit & maxFloatLogicalBottom) Line 633 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlock(bool relayoutChildren, WebCore::LayoutUnit pageLogicalHeight) Line 488 C++ EAWebKitd.dll!WebCore::RenderBlock::layout() Line 930 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox & child, WebCore::RenderBlockFlow::MarginInfo & marginInfo, WebCore::LayoutUnit & previousFloatLogicalBottom, WebCore::LayoutUnit & maxFloatLogicalBottom) Line 712 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlockChildren(bool relayoutChildren, WebCore::LayoutUnit & maxFloatLogicalBottom) Line 633 C++ EAWebKitd.dll!WebCore::RenderBlockFlow::layoutBlock(bool relayoutChildren, WebCore::LayoutUnit pageLogicalHeight) Line 488 C++ EAWebKitd.dll!WebCore::RenderBlock::layout() Line 930 C++ EAWebKitd.dll!WebCore::RenderView::layoutContent(const WebCore::LayoutState & state) Line 256 C++ EAWebKitd.dll!WebCore::RenderView::layout() Line 382 C++ EAWebKitd.dll!WebCore::FrameView::layout(bool allowSubtree) Line 1426 C++ EAWebKitd.dll!WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive() Line 4153 C++ EAWebKitd.dll!EA::WebKit::View::Paint() Line 278 C++ EAWebKitDemoUTFWin.exe!EA::Browser::BrowserWinView::OnTick() Line 1039 C++ EAWebKitDemoUTFWin.exe!EA::UTFWin::CustomWindow::DoMessage(const EA::UTFWin::Message & msg) Line 46 C++ EAWebKitDemoUTFWin.exe!EA::Browser::BrowserWinView::DoMessage(const EA::UTFWin::Message & msg) Line 649 C++ EAWebKitDemoUTFWin.exe!EA::UTFWin::WindowMgr::DispatchMsgToWindow(EA::UTFWin::Window * target, const EA::UTFWin::Message & msg, bool outbound) Line 2120 C++ EAWebKitDemoUTFWin.exe!EA::UTFWin::WindowMgr::SendMsg(EA::UTFWin::IWindow * src, EA::UTFWin::IWindow * dst0, const EA::UTFWin::Message & msg, bool inheritable, bool reversePriority) Line 249 C++ EAWebKitDemoUTFWin.exe!EA::UTFWin::WindowMgr::ProcessMessages() Line 451 C++ EAWebKitDemoUTFWin.exe!EA::Browser::BrowserApp::TickEAWebKitThread() Line 781 C++ EAWebKitDemoUTFWin.exe!EA::Browser::BrowserApp::RunEAWebKit(void * instance) Line 838 C++ EAWebKitDemoUTFWin.exe!EA::Debug::ExceptionHandler::ExecuteUserFunction(EA::Debug::ExceptionHandler::UserFunctionUnion userFunctionUnion, EA::Debug::ExceptionHandler::UserFunctionType userFunctionType, void * pContext) Line 900 C++ EAWebKitDemoUTFWin.exe!EA::Debug::ExceptionHandlerWin32::RunTrapped(EA::Debug::ExceptionHandler::UserFunctionUnion userFunctionUnion, EA::Debug::ExceptionHandler::UserFunctionType userFunctionType, void * pContext, bool & exceptionCaught) Line 529 C++ EAWebKitDemoUTFWin.exe!EA::Debug::ExceptionHandler::RunTrappedInternal(EA::Debug::ExceptionHandler::UserFunctionUnion userFunctionUnion, EA::Debug::ExceptionHandler::UserFunctionType userFunctionType, void * pContext, bool & exceptionCaught) Line 881 C++ EAWebKitDemoUTFWin.exe!EA::Debug::ExceptionHandler::RunTrapped(void (void *) * userFunction, void * pContext) Line 925 C++ EAWebKitDemoUTFWin.exe!EA::Browser::BrowserApp::Run(void * __formal) Line 855 C++ EAWebKitDemoUTFWin.exe!RunnableObjectInternal(void * pContext) Line 608 C++ EAWebKitDemoUTFWin.exe!invoke_thread_procedure(unsigned int (void *) * const procedure, void * const context) Line 92 C++ EAWebKitDemoUTFWin.exe!thread_start<unsigned int (__cdecl*)(void * __ptr64)>(void * const parameter) Line 115 C++ [External Code]