chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx | 75 ++++++++-- chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx | 56 ++++++- chart2/source/controller/chartapiwrapper/DiagramWrapper.cxx | 65 +++++++- chart2/source/controller/chartapiwrapper/TitleWrapper.cxx | 11 + chart2/source/controller/chartapiwrapper/WrappedStockProperties.cxx | 2 5 files changed, 176 insertions(+), 33 deletions(-)
New commits: commit 0b3ab2a39733cdd3b828e3475216bafc9e7b13ef Author: Andras Timar <[email protected]> AuthorDate: Wed Feb 25 10:44:23 2026 +0100 Commit: Miklos Vajna <[email protected]> CommitDate: Wed Feb 25 13:42:44 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]> diff --git a/chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx b/chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx index 771ca721b781..b5f9c96f2627 100644 --- a/chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx +++ b/chart2/source/controller/chartapiwrapper/Chart2ModelContact.cxx @@ -149,12 +149,18 @@ 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 ExplicitValueProvider::getExplicitNumberFormatKeyForAxis( xAxis, xCooSys - , m_xChartModel.get() ); + , xChartModel ); } sal_Int32 Chart2ModelContact::getExplicitNumberFormatKeyForSeries( @@ -166,23 +172,42 @@ sal_Int32 Chart2ModelContact::getExplicitNumberFormatKeyForSeries( awt::Size Chart2ModelContact::GetPageSize() const { - return ChartModelHelper::getPageSize(m_xChartModel.get()); + rtl::Reference<ChartModel> xChartModel( m_xChartModel ); + if( !xChartModel ) + { + SAL_WARN("chart2", "Chart2ModelContact: ChartModel expired"); + return awt::Size(); + } + return ChartModelHelper::getPageSize(xChartModel); } 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 = ExplicitValueProvider::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 = ExplicitValueProvider::AddSubtractAxisTitleSizes( - *m_xChartModel.get(), getChartView().get(), aRect, false ); + *xChartModel, getChartView().get(), aRect, false ); return aRect; } @@ -190,10 +215,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 { ExplicitValueProvider* pProvider( getExplicitValueProvider() ); @@ -206,10 +237,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 { ExplicitValueProvider* pProvider( getExplicitValueProvider() ); @@ -225,8 +262,14 @@ awt::Size Chart2ModelContact::GetLegendSize() const ExplicitValueProvider* pProvider( getExplicitValueProvider() ); if( pProvider ) { - 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( pProvider->getRectangleOfObject( aCID ) ); } return aSize; @@ -238,8 +281,14 @@ awt::Point Chart2ModelContact::GetLegendPosition() const ExplicitValueProvider* pProvider( getExplicitValueProvider() ); if( pProvider ) { - 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( pProvider->getRectangleOfObject( aCID ) ); } return aPoint; diff --git a/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx b/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx index ab6de48e0fe1..7a60806adfcc 100644 --- a/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx +++ b/chart2/source/controller/chartapiwrapper/ChartDocumentWrapper.cxx @@ -453,7 +453,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; @@ -474,8 +480,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 @@ -656,7 +668,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; @@ -666,7 +684,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; @@ -755,7 +779,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 ) ); } @@ -933,7 +963,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 ); @@ -946,7 +982,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 ea38fcc87735..3407f5f8d34b 100644 --- a/chart2/source/controller/chartapiwrapper/DiagramWrapper.cxx +++ b/chart2/source/controller/chartapiwrapper/DiagramWrapper.cxx @@ -674,7 +674,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; @@ -704,7 +710,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; @@ -737,7 +749,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() ) { @@ -759,8 +777,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) ); @@ -787,8 +811,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) ); @@ -799,7 +829,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 ); } @@ -1547,7 +1583,10 @@ void WrappedNumberOfLinesProperty::setPropertyValue( const Any& rOuterValue, con try { // locked controllers - ControllerLockGuardUNO aCtrlLockGuard( m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + 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 ); @@ -1826,7 +1865,13 @@ void WrappedIncludeHiddenCellsProperty::setPropertyValue( const Any& rOuterValue if( ! (rOuterValue >>= bNewValue) ) throw lang::IllegalArgumentException( u"Property IncludeHiddenCells requires boolean value"_ustr, nullptr, 0 ); - ChartModelHelper::setIncludeHiddenCells( bNewValue, *m_spChart2ModelContact->getDocumentModel() ); + rtl::Reference<ChartModel> xModel( m_spChart2ModelContact->getDocumentModel() ); + if( !xModel ) + { + SAL_WARN("chart2", "ChartModel expired, skip operation"); + return; + } + ChartModelHelper::setIncludeHiddenCells( bNewValue, *xModel ); } 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 & )
