Apache OpenOffice (AOO) Bugzilla – Issue 113743
chart2: attributed data points leak
Last modified: 2017-05-20 11:41:49 UTC
This obvious memory leak problem can be recreated with loading the sample file which has charts inside. The callstack when creating the leaked DataSeries are, chartmodelmi.dll!chart::DataSeries::DataSeries(const com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> & xContext={...}) Line 144 C++ chartmodelmi.dll!chart::DataSeries::create(const com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> & xContext={...}) Line 92 + 0x4b bytes C++ cppuhelper3MSC.dll!cppu::OSingleFactoryHelper::createInstanceEveryTime(const com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> & xContext={...}) Line 177 + 0xe bytes C++ cppuhelper3MSC.dll!cppu::OSingleFactoryHelper::createInstanceWithContext(const com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> & xContext={...}) Line 218 + 0x17 bytes C++ cppuhelper3MSC.dll!cppu::OFactoryComponentHelper::createInstanceWithContext(const com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> & xContext={...}) Line 502 + 0x11 bytes C++ cppuhelper3MSC.dll!cppu::ORegistryFactoryHelper::createInstanceEveryTime(const com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> & xContext={...}) Line 766 + 0x26 bytes C++ cppuhelper3MSC.dll!cppu::OSingleFactoryHelper::createInstanceWithContext(const com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> & xContext={...}) Line 218 + 0x17 bytes C++ cppuhelper3MSC.dll!cppu::OFactoryComponentHelper::createInstanceWithContext(const com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> & xContext={...}) Line 502 + 0x11 bytes C++ bootstrap.uno.dll!stoc_smgr::OServiceManager::createInstanceWithContext(const rtl::OUString & rServiceSpecifier={...}, const com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> & xContext={...}) Line 1276 + 0x23 bytes C++ bootstrap.uno.dll!stoc_smgr::OServiceManager::createInstance(const rtl::OUString & rServiceSpecifier={...}) Line 1386 + 0x1f bytes C++ scfiltmi.dll!ScfApiHelper::CreateInstance(com::sun::star::uno::Reference<com::sun::star::lang::XMultiServiceFactory> xFactory={...}, const rtl::OUString & rServiceName={...}) Line 94 + 0x1a bytes C++ scfiltmi.dll!ScfApiHelper::CreateInstance(const rtl::OUString & rServiceName={...}) Line 111 + 0x20 bytes C++ > scfiltmi.dll!XclImpChSeries::CreateDataSeries() Line 1735 + 0x65 bytes C++ scfiltmi.dll!XclImpChTypeGroup::CreateDataSeries(com::sun::star::uno::Reference<com::sun::star::chart2::XChartType> xChartType={...}, long nApiAxesSetIdx=0x00000000) Line 2515 + 0x14 bytes C++ scfiltmi.dll!XclImpChTypeGroup::CreateChartType(com::sun::star::uno::Reference<com::sun::star::chart2::XDiagram> xDiagram={...}, long nApiAxesSetIdx=0x00000000) Line 2427 C++ scfiltmi.dll!XclImpChAxesSet::CreateCoordSystem(com::sun::star::uno::Reference<com::sun::star::chart2::XDiagram> xDiagram={...}) Line 3213 + 0x75 bytes C++ The root cause is that, in api "XclImpChSeries::CreateDataSeries() const", after creating the DataSeries uno object, it then calls api, > chartmodelmi.dll!chart::DataSeries::getDataPointByIndex(long nIndex=0x00000000) Line 406 C++ scfiltmi.dll!`anonymous namespace'::lclGetPointPropSet(com::sun::star::uno::Reference<com::sun::star::chart2::XDataSeries> xDataSeries={...}, unsigned short nPointIdx=0x0000) Line 1706 + 0x1e bytes C++ scfiltmi.dll!XclImpChSeries::CreateDataSeries() Line 1801 + 0x5b bytes C++ scfiltmi.dll!XclImpChTypeGroup::CreateDataSeries(com::sun::star::uno::Reference<com::sun::star::chart2::XChartType> xChartType={...}, long nApiAxesSetIdx=0x00000000) Line 2515 + 0x14 bytes C++ if you check api chart::DataSeries::getDataPointByIndex(), you may find it creates, aResult.set( new DataPoint( this )); ModifyListenerHelper::addListener( aResult, m_xModifyEventForwarder ); m_aAttributedDataPoints[ nIndex ] = aResult; This piece of code introduces cyclic reference between DataSeies and DataPoint objects stored in m_aAttributedDataPoints. To fix the problem, there must be a code point where to break the cyclic reference.
Created attachment 71018 [details] sample file to recreate the memory leak
Created attachment 71019 [details] It is an ugly patch, but it did fix the leak. Just for your reference. You can change to use the normal standard way - implement XComponent pattern to fix this problem.
Problem confirmed. But within the patch the condition: 1 == (m_refCount - m_aAttributedDataPoints.size()) looks wrong to me. @zhangjfibm, would you mind to provide a corrected patch using the XComponent pattern or using WeakReferences to avoid cyclic references at all?
Such a patch is still not ready yet. I need a few time to work out one.
Created attachment 71025 [details] New patch using XComponent pattern. Please check if it works and is acceptable.
@zhangjfibm, first thanks for providing a second patch! There are a few issues. The patch does not compile out of the box in the OOo environment with warning==errors because of 'unused parameter' warning for parameter xListener and a 'signed/unsigned mismatch' warning within the for-loop with a sal_Int32 index variable for a vector. The second can be avoided by using an iterator instead. I also would shorten the namespaces within the c++ files. Having done that changes the patch does fix the obvious memory leak. When the attached example document is opened the DataPoint constructor is reached 18 times and when the document is closed the destructor is reached 18 times too now! Thats good. I will check next week whether there are some not so obvious scenarios where the leak still exist - e.g. removing a series via dialog or API. Thanks again for the patch and for pointing out the problem!
It looks good, thank you.
Created attachment 71164 [details] example showing the leak
I now have checked the not so obvious scenarios for this leak. There are still problems. Load the attached document DataPointMemoryLeak.ods and run the macro. While running the macro multiple times look at the soffice.bin process. It is consuming more and more memory. The macro does the following: A second series is added to a chart. One data point of the second series is given a color (this creates the data point and the cyclic reference). The series is removed from the chart again by choosing a different cell range. This is done multiple times. Conclusion: The XComponent interface is not the correct pattern to fix this memory leak because the series objects do not always have a defined owner. The correct approach here is to use weak references instead of hard references. A weak reference does not keep the object alive it points to. This pattern avoids the cyclic reference completely in advance and has no administrative overhead - very elegant.
Created attachment 71165 [details] patch using weak ref pattern fixing both obvious and not so obvious leak
Fixed in CWS chart49. http://hg.services.openoffice.org/cws/chart49/rev/2d63883e5ea4
zhangjfibm, please verify the fix in CWS chart49. Thanks!
verified, thanks.
reopen it because wrongly closed.
change state because it was wrongly closed.
Mark it as verified.