Make replacement graphic management more atomic

Commit 8872f7121b4ae4dd0b51820366d3510a88f7aac2 (crashtesting: crash
on exporting kde274105-6.docx to .rtf, 2024-03-27) added some safety
code in EmbeddedObjectRef::GetReplacement. It mentioned, that there
are likely some bugs in the management of the graphic.

This tries to fix this management, avoiding the intermediate states,
and only changing the graphic when all the data is ready. This also
reverts the changes of the mentioned commit, obsoleted now; and of
commit 8780fa41dcd164af244742461f4e57a4bcf4c7a4 (svtools: fix lost
replacement grpahic when updating it via OLE fails, 2018-10-30);
but keeps commit 24231f2a336c50eac5e6a1621c57e60ebe1e1cf4 (svtools:
fix lost replacement non-rendered graphic when updating it fails,
2022-02-17).

This has revealed that the second part of unit test for tdf#143222
("Check export of embedded worksheet in slide"), introduced in
commit 92a407b7f90a98704a238c5ffa3a3491eaf3263a (tdf143222 Handle
alternate content of graphicData element., 2021-07-08), has never
really worked: the "pGraphic != nullptr" check would never fail;
in fact, that used to always return an empty graphic. The problem
was filed as tdf#163064, and the test was modified accordingly.

Commit 5d997c029e53c31a3651a08f5012645097cec48f (sw XHTML export:
improve dummy OLE object handling, 2018-08-30) made ReqIF export
handle missing replacement graphic. However, it had assumed that
SwOLENode::GetGraphic always returns a valid pointer even for the
missing data. That is fixed here in OutHTML_FrameFormatOLENodeGrf.

Other places, where the pointer was dereferenced unconditionally,
were fixed (keeping current behavior).

Change-Id: Ica97a691ecc11b856bdb003f89467ea3392684bd
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/173716
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>

1 files changed, 14 insertions, 49 deletions

diff --git a/svtools/source/misc/embedhlp.cxx b/svtools/source/misc/embedhlp.cxx
index 9e14fb5d37a8..78b5909f1602 100644
--- a/svtools/source/misc/embedhlp.cxx
+++ b/svtools/source/misc/embedhlp.cxx
@@ -452,37 +452,21 @@ const Link<LinkParamNone*, bool> & EmbeddedObjectRef::GetIsProtectedHdl() const
 
 void EmbeddedObjectRef::GetReplacement( bool bUpdate )
 {
-    Graphic aOldGraphic;
-    OUString aOldMediaType;
-
     if ( bUpdate )
     {
-        if (mpImpl->oGraphic)
-            aOldGraphic = *mpImpl->oGraphic;
-        aOldMediaType = mpImpl->aMediaType;
-
         // Do not clear / reset mpImpl->oGraphic, because it would appear as no replacement
         // on any call to getReplacementGraphic during the external calls to the OLE object,
         // which may release mutexes. Only replace it when done.
         mpImpl->aMediaType.clear();
-        mpImpl->mnGraphicVersion++;
-    }
-    else if ( !mpImpl->oGraphic )
-    {
-        mpImpl->mnGraphicVersion++;
     }
-    else
+    else if (mpImpl->oGraphic)
     {
         OSL_FAIL("No update, but replacement exists already!");
         return;
     }
 
-    // Missing graphic can crash
-    if (!mpImpl->oGraphic)
-        mpImpl->oGraphic.emplace();
-
     std::unique_ptr<SvStream> pGraphicStream(GetGraphicStream( bUpdate ));
-    if (!pGraphicStream && aOldGraphic.IsNone())
+    if (!pGraphicStream && bUpdate && (!mpImpl->oGraphic || mpImpl->oGraphic->IsNone()))
     {
         // We have no old graphic, tried to get an updated one, but that failed. Try to get an old
         // graphic instead of having no graphic at all.
@@ -491,31 +475,16 @@ void EmbeddedObjectRef::GetReplacement( bool bUpdate )
                  "EmbeddedObjectRef::GetReplacement: failed to get updated graphic stream");
     }
 
-    if (!mpImpl->oGraphic)
-    {
-        // note that UpdateReplacementOnDemand which resets mpImpl->oGraphic to null may have been called
-        // e.g. when exporting ooo58458-1.odt to doc or kde274105-6.docx to rtf. Those looks like bugs as
-        // presumably generating the replacement graphic shouldn't re-trigger that the graphic needs
-        // to be updated, bodge this to work as callers naturally expect
-        SAL_WARN("svtools.misc", "EmbeddedObjectRef::GetReplacement generating replacement image modified object to claim it needs to update replacement");
-        mpImpl->oGraphic.emplace();
-    }
-
     if ( pGraphicStream )
     {
         GraphicFilter& rGF = GraphicFilter::GetGraphicFilter();
-        rGF.ImportGraphic( *mpImpl->oGraphic, u"", *pGraphicStream );
-        mpImpl->mnGraphicVersion++;
-    }
-
-    if (bUpdate && mpImpl->oGraphic->IsNone() && !aOldGraphic.IsNone())
-    {
-        // We used to have an old graphic, tried to update and the update
-        // failed. Go back to the old graphic instead of having no graphic at
-        // all.
-        mpImpl->oGraphic.emplace(aOldGraphic);
-        mpImpl->aMediaType = aOldMediaType;
-        SAL_WARN("svtools.misc", "EmbeddedObjectRef::GetReplacement: failed to update graphic");
+        Graphic aNewGraphic;
+        rGF.ImportGraphic(aNewGraphic, u"", *pGraphicStream);
+        if (!aNewGraphic.IsNone())
+        {
+            mpImpl->oGraphic.emplace(aNewGraphic);
+            mpImpl->mnGraphicVersion++;
+        }
     }
 }
 
@@ -601,17 +570,13 @@ Size EmbeddedObjectRef::GetSize( MapMode const * pTargetMapMode ) const
 void EmbeddedObjectRef::SetGraphicStream( const uno::Reference< io::XInputStream >& xInGrStream,
                                             const OUString& rMediaType )
 {
-    mpImpl->oGraphic.emplace();
-    mpImpl->aMediaType = rMediaType;
-    mpImpl->mnGraphicVersion++;
-
+    Graphic aNewGraphic;
     std::unique_ptr<SvStream> pGraphicStream(::utl::UcbStreamHelper::CreateStream( xInGrStream ));
 
     if ( pGraphicStream )
     {
         GraphicFilter& rGF = GraphicFilter::GetGraphicFilter();
-        rGF.ImportGraphic( *mpImpl->oGraphic, u"", *pGraphicStream );
-        mpImpl->mnGraphicVersion++;
+        rGF.ImportGraphic(aNewGraphic, u"", *pGraphicStream);
 
         if ( mpImpl->pContainer )
         {
@@ -622,8 +587,10 @@ void EmbeddedObjectRef::SetGraphicStream( const uno::Reference< io::XInputStream
         }
     }
 
+    mpImpl->oGraphic.emplace(aNewGraphic);
+    mpImpl->aMediaType = rMediaType;
+    mpImpl->mnGraphicVersion++;
     mpImpl->bNeedUpdate = false;
-
 }
 
 void EmbeddedObjectRef::SetGraphic( const Graphic& rGraphic, const OUString& rMediaType )
@@ -955,9 +922,7 @@ void EmbeddedObjectRef::UpdateOleObject( bool bUpdateOle )
 
 void EmbeddedObjectRef::UpdateReplacementOnDemand()
 {
-    mpImpl->oGraphic.reset();
     mpImpl->bNeedUpdate = true;
-    mpImpl->mnGraphicVersion++;
 
     if( mpImpl->pContainer )
     {