chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx | 75 ++++++++-- chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx | 56 ++++++- chart2/source/controller/chartapiwrapper/DiagramWrapper.cxx | 69 +++++++-- chart2/source/controller/chartapiwrapper/TitleWrapper.cxx | 11 + chart2/source/controller/chartapiwrapper/WrappedStockProperties.cxx | 2 5 files changed, 180 insertions(+), 33 deletions(-)
New commits: commit d8e005d7656c38da13ab6477d68a67306f5ad85c Author: Andras Timar <[email protected]> AuthorDate: Wed Feb 25 10:44:23 2026 +0100 Commit: Andras Timar <[email protected]> CommitDate: Wed Feb 25 17:59:18 2026 +0100 chart2: hold strong ref to ChartModel to prevent race conditions The weak reference in Chart2ModelContact can expire at any time when the ChartModel is destroyed (e.g. during collaborative editing). The previous fix (5f4e12f5e4a9) added null checks in ControllerLockGuardUNO, but callers would silently continue without controller locking and then crash in subsequent calls like GetPageSize() or SubstractAxisTitleSizes(). Fix this properly: resolve the weak reference once into a strong rtl::Reference at the start of each operation, bail out early if the model is gone, and reuse the strong reference throughout. This both prevents the race condition (strong ref keeps the model alive for the scope) and avoids proceeding with unlocked controllers. Also fix unguarded m_xChartModel.get() dereferences in Chart2ModelContact itself (GetPageSize, GetDiagramRectangle*, GetLegendSize/Position, etc.) which had the same crash potential. Add SAL_WARN logging to all early-return paths so that the expired model condition is traceable in logs. Change-Id: I7b3e2a1f5c9d4e8b6a0f2d3c5e7a9b1d4f6e8c0a Reviewed-on: https://gerrit.libreoffice.org/c/core/+/200304 Reviewed-by: Miklos Vajna <[email protected]> Tested-by: Jenkins CollaboraOffice <[email protected]> Reviewed-on: https://gerrit.libreoffice.org/c/core/+/200323 Reviewed-by: Andras Timar <[email protected]> diff --git a/chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx b/chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx index 6bd6d71e1481..56dfe4b205fa 100644 --- a/chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx +++ b/chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx @@ -132,33 +132,58 @@ void Chart2ModelContact::getExplicitValuesForAxis( sal_Int32 Chart2ModelContact::getExplicitNumberFormatKeyForAxis( const rtl::Reference< ::chart::Axis >& xAxis ) { + rtl::Reference<ChartModel> xChartModel( m_xChartModel ); + if( !xChartModel ) + { + SAL_WARN("chart2", "Chart2ModelContact: ChartModel expired"); + return 0; + } rtl::Reference< BaseCoordinateSystem > xCooSys( AxisHelper::getCoordinateSystemOfAxis( - xAxis, m_xChartModel.get()->getFirstChartDiagram() ) ); + xAxis, xChartModel->getFirstChartDiagram() ) ); return ChartView::getExplicitNumberFormatKeyForAxis( xAxis, xCooSys - , m_xChartModel.get() ); + , xChartModel ); } awt::Size Chart2ModelContact::GetPageSize() const { - return m_xChartModel.get()->getPageSize(); + rtl::Reference<ChartModel> xChartModel( m_xChartModel ); + if( !xChartModel ) + { + SAL_WARN("chart2", "Chart2ModelContact: ChartModel expired"); + return awt::Size(); + } + return xChartModel->getPageSize(); } awt::Rectangle Chart2ModelContact::SubstractAxisTitleSizes( const awt::Rectangle& rPositionRect ) { + rtl::Reference<ChartModel> xChartModel( m_xChartModel ); + if( !xChartModel ) + { + SAL_WARN("chart2", "Chart2ModelContact: ChartModel expired"); + return rPositionRect; + } awt::Rectangle aRect = ChartView::AddSubtractAxisTitleSizes( - *m_xChartModel.get(), getChartView().get(), rPositionRect, true ); + *xChartModel, getChartView().get(), rPositionRect, true ); return aRect; } awt::Rectangle Chart2ModelContact::GetDiagramRectangleIncludingTitle() const { + rtl::Reference<ChartModel> xChartModel( m_xChartModel ); + if( !xChartModel ) + { + SAL_WARN("chart2", "Chart2ModelContact: ChartModel expired"); + return awt::Rectangle(0,0,0,0); + } + awt::Rectangle aRect( GetDiagramRectangleIncludingAxes() ); //add axis title sizes to the diagram size aRect = ChartView::AddSubtractAxisTitleSizes( - *m_xChartModel.get(), getChartView().get(), aRect, false ); + *xChartModel, getChartView().get(), aRect, false ); return aRect; } @@ -166,10 +191,16 @@ awt::Rectangle Chart2ModelContact::GetDiagramRectangleIncludingTitle() const awt::Rectangle Chart2ModelContact::GetDiagramRectangleIncludingAxes() const { awt::Rectangle aRect(0,0,0,0); - rtl::Reference< Diagram > xDiagram = m_xChartModel.get()->getFirstChartDiagram(); + rtl::Reference<ChartModel> xChartModel( m_xChartModel ); + if( !xChartModel ) + { + SAL_WARN("chart2", "Chart2ModelContact: ChartModel expired"); + return aRect; + } + rtl::Reference< Diagram > xDiagram = xChartModel->getFirstChartDiagram(); if( xDiagram && xDiagram->getDiagramPositioningMode() == DiagramPositioningMode::Including ) - aRect = DiagramHelper::getDiagramRectangleFromModel(m_xChartModel.get()); + aRect = DiagramHelper::getDiagramRectangleFromModel(xChartModel); else { rtl::Reference< ChartView > const & rxChartView = getChartView(); @@ -182,10 +213,16 @@ awt::Rectangle Chart2ModelContact::GetDiagramRectangleIncludingAxes() const awt::Rectangle Chart2ModelContact::GetDiagramRectangleExcludingAxes() const { awt::Rectangle aRect(0,0,0,0); - rtl::Reference< Diagram > xDiagram = m_xChartModel.get()->getFirstChartDiagram(); + rtl::Reference<ChartModel> xChartModel( m_xChartModel ); + if( !xChartModel ) + { + SAL_WARN("chart2", "Chart2ModelContact: ChartModel expired"); + return aRect; + } + rtl::Reference< Diagram > xDiagram = xChartModel->getFirstChartDiagram(); if( xDiagram && xDiagram->getDiagramPositioningMode() == DiagramPositioningMode::Excluding ) - aRect = DiagramHelper::getDiagramRectangleFromModel(m_xChartModel.get()); + aRect = DiagramHelper::getDiagramRectangleFromModel(xChartModel); else { rtl::Reference< ChartView > const & rxChartView = getChartView(); @@ -201,8 +238,14 @@ awt::Size Chart2ModelContact::GetLegendSize() const rtl::Reference< ChartView > const & rxChartView = getChartView(); if( rxChartView ) { - rtl::Reference< Legend > xLegend = LegendHelper::getLegend( *m_xChartModel.get() ); - OUString aCID( ObjectIdentifier::createClassifiedIdentifierForObject( xLegend, m_xChartModel ) ); + rtl::Reference<ChartModel> xChartModel( m_xChartModel ); + if( !xChartModel ) + { + SAL_WARN("chart2", "Chart2ModelContact: ChartModel expired"); + return aSize; + } + rtl::Reference< Legend > xLegend = LegendHelper::getLegend( *xChartModel ); + OUString aCID( ObjectIdentifier::createClassifiedIdentifierForObject( xLegend, xChartModel ) ); aSize = ToSize( rxChartView->getRectangleOfObject( aCID ) ); } return aSize; @@ -214,8 +257,14 @@ awt::Point Chart2ModelContact::GetLegendPosition() const rtl::Reference< ChartView > const & rxChartView = getChartView(); if( rxChartView ) { - rtl::Reference< Legend > xLegend = LegendHelper::getLegend( *m_xChartModel.get() ); - OUString aCID( ObjectIdentifier::createClassifiedIdentifierForObject( xLegend, m_xChartModel ) ); + rtl::Reference<ChartModel> xChartModel( m_xChartModel ); + if( !xChartModel ) + { + SAL_WARN("chart2", "Chart2ModelContact: ChartModel expired"); + return aPoint; + } + rtl::Reference< Legend > xLegend = LegendHelper::getLegend( *xChartModel ); + OUString aCID( ObjectIdentifier::createClassifiedIdentifierForObject( xLegend, xChartModel ) ); aPoint = ToPoint( rxChartView->getRectangleOfObject( aCID ) ); } return aPoint; diff --git a/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx b/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx index 020e8124818f..59d1b2fc9dc0 100644 --- a/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx +++ b/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx @@ -452,7 +452,13 @@ void WrappedHasLegendProperty::setPropertyValue( const Any& rOuterValue, const R try { - rtl::Reference< Legend > xLegend = LegendHelper::getLegend( *m_spChart2ModelContact->getDocumentModel(), m_spChart2ModelContact->m_xContext,bNewValue ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + rtl::Reference< Legend > xLegend = LegendHelper::getLegend( *xModel, m_spChart2ModelContact->m_xContext,bNewValue ); if(xLegend.is()) { bool bOldValue = true; @@ -473,8 +479,14 @@ Any WrappedHasLegendProperty::getPropertyValue( const Reference< beans::XPropert Any aRet; try { + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return aRet; + } rtl::Reference< Legend > xLegend = - LegendHelper::getLegend( *m_spChart2ModelContact->getDocumentModel() ); + LegendHelper::getLegend( *xModel ); if( xLegend.is()) aRet = xLegend->getPropertyValue(u"Show"_ustr); else @@ -721,7 +733,13 @@ Reference< drawing::XShape > SAL_CALL ChartDocumentWrapper::getTitle() { if( !m_xTitle.is() ) { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return m_xTitle; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); m_xTitle = new TitleWrapper( TitleHelper::MAIN_TITLE, m_spChart2ModelContact ); } return m_xTitle; @@ -731,7 +749,13 @@ Reference< drawing::XShape > SAL_CALL ChartDocumentWrapper::getSubTitle() { if( !m_xSubTitle.is() ) { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return m_xSubTitle; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); m_xSubTitle = new TitleWrapper( TitleHelper::SUB_TITLE, m_spChart2ModelContact ); } return m_xSubTitle; @@ -820,7 +844,13 @@ void SAL_CALL ChartDocumentWrapper::attachData( const Reference< XChartData >& x if( !xNewData.is() ) return; - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); m_xChartData.set( new ChartDataWrapper( m_spChart2ModelContact, xNewData ) ); } @@ -998,7 +1028,13 @@ void ChartDocumentWrapper::impl_resetAddIn() void ChartDocumentWrapper::setBaseDiagram( const OUString& rBaseDiagram ) { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); m_aBaseDiagram = rBaseDiagram; uno::Reference< XDiagram > xDiagram( ChartDocumentWrapper::createInstance( rBaseDiagram ), uno::UNO_QUERY ); @@ -1011,7 +1047,13 @@ void ChartDocumentWrapper::setAddIn( const Reference< util::XRefreshable >& xAdd if( m_xAddIn == xAddIn ) return; - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); impl_resetAddIn(); m_xAddIn = xAddIn; // initialize AddIn with this as chart document diff --git a/chart2/source/controller/chartapiwrapper/DiagramWrapper.cxx b/chart2/source/controller/chartapiwrapper/DiagramWrapper.cxx index 255727bfafb6..be0320725163 100644 --- a/chart2/source/controller/chartapiwrapper/DiagramWrapper.cxx +++ b/chart2/source/controller/chartapiwrapper/DiagramWrapper.cxx @@ -68,6 +68,7 @@ #include <com/sun/star/util/XRefreshable.hpp> #include <comphelper/diagnose_ex.hxx> #include <o3tl/string_view.hxx> +#include <sal/log.hxx> #include <utility> using namespace ::com::sun::star; @@ -681,7 +682,13 @@ awt::Point SAL_CALL DiagramWrapper::getPosition() void SAL_CALL DiagramWrapper::setPosition( const awt::Point& aPosition ) { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); Reference< beans::XPropertySet > xProp( getInnerPropertySet() ); if( !xProp.is() ) return; @@ -711,7 +718,13 @@ awt::Size SAL_CALL DiagramWrapper::getSize() void SAL_CALL DiagramWrapper::setSize( const awt::Size& aSize ) { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); Reference< beans::XPropertySet > xProp( getInnerPropertySet() ); if( !xProp.is() ) return; @@ -744,7 +757,13 @@ OUString SAL_CALL DiagramWrapper::getShapeType() void SAL_CALL DiagramWrapper::setAutomaticDiagramPositioning() { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); uno::Reference< beans::XPropertySet > xDiaProps( getDiagram(), uno::UNO_QUERY ); if( xDiaProps.is() ) { @@ -766,8 +785,14 @@ sal_Bool SAL_CALL DiagramWrapper::isAutomaticDiagramPositioning( ) } void SAL_CALL DiagramWrapper::setDiagramPositionExcludingAxes( const awt::Rectangle& rPositionRect ) { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); - DiagramHelper::setDiagramPositioning( m_spChart2ModelContact->getDocumentModel(), rPositionRect ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); + DiagramHelper::setDiagramPositioning( xModel, rPositionRect ); uno::Reference< beans::XPropertySet > xDiaProps( getDiagram(), uno::UNO_QUERY ); if( xDiaProps.is() ) xDiaProps->setPropertyValue(u"PosSizeExcludeAxes"_ustr, uno::Any(true) ); @@ -794,8 +819,14 @@ awt::Rectangle SAL_CALL DiagramWrapper::calculateDiagramPositionExcludingAxes( } void SAL_CALL DiagramWrapper::setDiagramPositionIncludingAxes( const awt::Rectangle& rPositionRect ) { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); - DiagramHelper::setDiagramPositioning( m_spChart2ModelContact->getDocumentModel(), rPositionRect ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); + DiagramHelper::setDiagramPositioning( xModel, rPositionRect ); uno::Reference< beans::XPropertySet > xDiaProps( getDiagram(), uno::UNO_QUERY ); if( xDiaProps.is() ) xDiaProps->setPropertyValue(u"PosSizeExcludeAxes"_ustr, uno::Any(false) ); @@ -806,7 +837,13 @@ awt::Rectangle SAL_CALL DiagramWrapper::calculateDiagramPositionIncludingAxes( } void SAL_CALL DiagramWrapper::setDiagramPositionIncludingAxesAndAxisTitles( const awt::Rectangle& rPositionRect ) { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); awt::Rectangle aRect( m_spChart2ModelContact->SubstractAxisTitleSizes(rPositionRect) ); DiagramWrapper::setDiagramPositionIncludingAxes( aRect ); } @@ -1554,7 +1591,13 @@ void WrappedNumberOfLinesProperty::setPropertyValue( const Any& rOuterValue, con try { // locked controllers - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); uno::Reference< beans::XPropertySet > xProp( static_cast<cppu::OWeakObject*>(xTemplate.get()), uno::UNO_QUERY ); xProp->setPropertyValue( u"NumberOfLines"_ustr, uno::Any(nNewValue) ); xTemplate->changeDiagram( xDiagram ); @@ -1833,7 +1876,13 @@ void WrappedIncludeHiddenCellsProperty::setPropertyValue( const Any& rOuterValue if( ! (rOuterValue >>= bNewValue) ) throw lang::IllegalArgumentException( u"Property IncludeHiddenCells requires boolean value"_ustr, nullptr, 0 ); - m_spChart2ModelContact->getDocumentModel()->setIncludeHiddenCells(bNewValue); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + xModel->setIncludeHiddenCells(bNewValue); } Any WrappedIncludeHiddenCellsProperty::getPropertyValue( const Reference< beans::XPropertySet >& /*xInnerPropertySet*/ ) const diff --git a/chart2/source/controller/chartapiwrapper/TitleWrapper.cxx b/chart2/source/controller/chartapiwrapper/TitleWrapper.cxx index e4ca74c37f19..fdbb5bc4f85b 100644 --- a/chart2/source/controller/chartapiwrapper/TitleWrapper.cxx +++ b/chart2/source/controller/chartapiwrapper/TitleWrapper.cxx @@ -38,6 +38,7 @@ #include <algorithm> #include <rtl/ustrbuf.hxx> +#include <sal/log.hxx> #include <cppuhelper/propshlp.hxx> #include <utility> @@ -243,9 +244,15 @@ TitleWrapper::TitleWrapper( ::chart::TitleHelper::eTitleType eTitleType, m_spChart2ModelContact(std::move( spChart2ModelContact )), m_eTitleType(eTitleType) { - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "TitleWrapper: ChartModel is gone, skip title creation"); + return; + } + ControllerLockGuardUNO aCtrlLockGuard( xModel ); if( !getTitleObject().is() ) //#i83831# create an empty title at the model, thus references to properties can be mapped correctly - TitleHelper::createTitle( m_eTitleType, OUString(), m_spChart2ModelContact->getDocumentModel(), m_spChart2ModelContact->m_xContext ); + TitleHelper::createTitle( m_eTitleType, OUString(), xModel, m_spChart2ModelContact->m_xContext ); } TitleWrapper::~TitleWrapper() diff --git a/chart2/source/controller/chartapiwrapper/WrappedStockProperties.cxx b/chart2/source/controller/chartapiwrapper/WrappedStockProperties.cxx index afd0d0d7e85a..9eee66867e26 100644 --- a/chart2/source/controller/chartapiwrapper/WrappedStockProperties.cxx +++ b/chart2/source/controller/chartapiwrapper/WrappedStockProperties.cxx @@ -97,7 +97,7 @@ void WrappedStockProperty::setPropertyValue( const css::uno::Any& rOuterValue, c try { // locked controllers - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + ControllerLockGuardUNO aCtrlLockGuard( xChartDoc ); xTemplate->changeDiagram( xDiagram ); } catch( const uno::Exception & )
