rhbz#1870501 crash on reexport of odg

where SdrObjects in a list have no navigation position set

a regression from

450cd772aa734cfcb989c8cedd3c0a454db74a34

Fix fdo#64512 Handle xml:id correctly on multi-image draw:frames

this just reverts that and adds the test case that crashed

Change-Id: I1a49dab9578699c42fe845e8ec42de40159dec3d
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/101074
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolanm@redhat.com>

2 files changed, 64 insertions, 42 deletions

diff --git a/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx b/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx
index d2e29422ae1c..ce54fe971d44 100644
--- a/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx
+++ b/xmloff/source/core/unointerfacetouniqueidentifiermapper.cxx
@@ -21,6 +21,7 @@
 
 #include <o3tl/safeint.hxx>
 #include <xmloff/unointerfacetouniqueidentifiermapper.hxx>
+#include <algorithm>
 
 using namespace ::com::sun::star;
 using css::uno::Reference;
@@ -64,25 +65,41 @@ bool UnoInterfaceToUniqueIdentifierMapper::registerReference( const OUString& rI
     {
         return rIdentifier != (*aIter).first;
     }
-    else if( findIdentifier( rIdentifier, aIter ) )
+    else if( findIdentifier( rIdentifier, aIter ) || findReserved( rIdentifier ) )
     {
         return false;
     }
     else
     {
-        insertReference( rIdentifier, xRef );
+        maEntries.insert( IdMap_t::value_type( rIdentifier, xRef ) );
+
+        // see if this is a reference like something we would generate in the future
+        const sal_Unicode *p = rIdentifier.getStr();
+        sal_Int32 nLength = rIdentifier.getLength();
+
+        // see if the identifier is 'id' followed by a pure integer value
+        if( nLength < 2 || p[0] != 'i' || p[1] != 'd' )
+            return true;
+
+        nLength -= 2;
+        p += 2;
+
+        while(nLength--)
+        {
+            if( (*p < '0') || (*p > '9') )
+                return true; // a custom id, that will never conflict with genereated id's
+            p++;
+        }
+
+        // the identifier is a pure integer value
+        // so we make sure we will never generate
+        // an integer value like this one
+        sal_Int32 nId = rIdentifier.copy(2).toInt32();
+        if (nId > 0 && mnNextId <= o3tl::make_unsigned(nId))
+            mnNextId = nId + 1;
+
+        return true;
     }
-
-    return true;
-}
-
-void UnoInterfaceToUniqueIdentifierMapper::registerReferenceAlways( const OUString& rIdentifier, const Reference< XInterface >& rInterface )
-{
-    // Be certain that the references we store in our table are to the
-    // leading / primary XInterface - cf. findReference
-    uno::Reference< uno::XInterface > xRef( rInterface, uno::UNO_QUERY );
-
-    insertReference( rIdentifier, xRef );
 }
 
 const OUString& UnoInterfaceToUniqueIdentifierMapper::getIdentifier( const Reference< XInterface >& rInterface ) const
@@ -135,38 +152,42 @@ bool UnoInterfaceToUniqueIdentifierMapper::findIdentifier( const OUString& rIden
     return rIter != maEntries.end();
 }
 
-void UnoInterfaceToUniqueIdentifierMapper::insertReference( const OUString& rIdentifier, const Reference< XInterface >& rInterface )
+bool UnoInterfaceToUniqueIdentifierMapper::reserveIdentifier( const OUString& rIdentifier )
 {
-    maEntries[rIdentifier] = rInterface;
+    if ( findReserved( rIdentifier ) )
+        return false;
 
-    // see if this is a reference like something we would generate in the future
-    const sal_Unicode *p = rIdentifier.getStr();
-    sal_Int32 nLength = rIdentifier.getLength();
+    maReserved.push_back( rIdentifier );
+    return true;
+}
 
-    // see if the identifier is 'id' followed by a pure integer value
-    if( nLength < 2 || p[0] != 'i' || p[1] != 'd' )
-        return;
+bool UnoInterfaceToUniqueIdentifierMapper::registerReservedReference(
+        const OUString& rIdentifier,
+        const css::uno::Reference< css::uno::XInterface >& rInterface )
+{
+    Reserved_t::const_iterator aIt;
+    if ( !findReserved( rIdentifier, aIt ) )
+        return false;
 
-    nLength -= 2;
-    p += 2;
+    Reserved_t::iterator aRemoveIt( maReserved.begin() + ( aIt - maReserved.begin() ) );
+    maReserved.erase( aRemoveIt );
+    registerReference( rIdentifier, rInterface );
 
-    while(nLength--)
-    {
-        if( (*p < '0') || (*p > '9') )
-            return; // a custom id, that will never conflict with generated id's
+    return true;
+}
 
-        p++;
-    }
+bool UnoInterfaceToUniqueIdentifierMapper::findReserved( const OUString& rIdentifier ) const
+{
+    Reserved_t::const_iterator aDummy;
+    return findReserved( rIdentifier, aDummy );
+}
 
-    // the identifier is a pure integer value
-    // so we make sure we will never generate
-    // an integer value like this one
-    sal_Int32 nId = rIdentifier.copy(2).toInt32();
-    if (nId > 0 && mnNextId <= o3tl::make_unsigned(nId))
-    {
-        mnNextId = nId;
-        ++mnNextId;
-    }
+bool UnoInterfaceToUniqueIdentifierMapper::findReserved(
+        const OUString& rIdentifier,
+        Reserved_t::const_iterator& rIter ) const
+{
+    rIter = std::find( maReserved.begin(), maReserved.end(), rIdentifier );
+    return rIter != maReserved.end();
 }
 
 }
diff --git a/xmloff/source/draw/ximpshap.cxx b/xmloff/source/draw/ximpshap.cxx
index c7cf5b4c08e2..e0e467b31501 100644
--- a/xmloff/source/draw/ximpshap.cxx
+++ b/xmloff/source/draw/ximpshap.cxx
@@ -3459,6 +3459,9 @@ SvXMLImportContextRef SdXMLFrameShapeContext::CreateChildContext( sal_uInt16 nPr
 
         if(getSupportsMultipleContents() && dynamic_cast< SdXMLGraphicObjectShapeContext* >(xContext.get()))
         {
+            if ( !maShapeId.isEmpty() )
+                GetImport().getInterfaceToIdentifierMapper().reserveIdentifier( maShapeId );
+
             addContent(*mxImplContext);
         }
     }
@@ -3568,13 +3571,11 @@ void SdXMLFrameShapeContext::EndElement()
     SvXMLImportContextRef const pSelectedContext(solveMultipleImages());
     const SdXMLGraphicObjectShapeContext* pShapeContext(
         dynamic_cast<const SdXMLGraphicObjectShapeContext*>(pSelectedContext.get()));
-    if ( pShapeContext && !maShapeId.isEmpty() )
+    if ( pShapeContext )
     {
-        // fdo#64512 and fdo#60075 - make sure *this* shape is
-        // registered for given ID
         assert( mxImplContext.is() );
         const uno::Reference< uno::XInterface > xShape( pShapeContext->getShape() );
-        GetImport().getInterfaceToIdentifierMapper().registerReferenceAlways( maShapeId, xShape );
+        GetImport().getInterfaceToIdentifierMapper().registerReservedReference( maShapeId, xShape );
     }
 
     if( !mxImplContext.is() )