Kaydet (Commit) e8dc04cb authored tarafından Noel Grandin's avatar Noel Grandin

tdf#125339 Base, Table is deleted after accessing the table

regression from
    commit 306758ab
    Date:   Fri Apr 5 15:40:27 2019 +0200
    tdf#117066 Saving ODT document with ~1500 bookmarks is slow, part 5
Before the above commit, we could have multiple child items with the
same name. Restore that behaviour, while keeping the fast lookup, by
using a std::unordered_map<OUString, std::vector<...

Also remove name from SotElement_Impl so there is no chance of the name
in the key of the map, and the name in the element getting out of sync.

Change-Id: I65c294ddc409d9b8a7006e4f4338c9a2a4446a92
Reviewed-on: https://gerrit.libreoffice.org/72544
Tested-by: Jenkins
Reviewed-by: 's avatarNoel Grandin <noel.grandin@collabora.co.uk>
üst 3f5f67c6
......@@ -48,6 +48,7 @@
#include <PackageConstants.hxx>
#include <comphelper/sequence.hxx>
#include <cppuhelper/queryinterface.hxx>
#include <cppuhelper/typeprovider.hxx>
#include <cppuhelper/exc_hlp.hxx>
......@@ -161,8 +162,7 @@ static uno::Reference< io::XInputStream > GetSeekableTempCopy( const uno::Refere
}
SotElement_Impl::SotElement_Impl(const OUString& rName, bool bStor, bool bNew)
: m_aName(rName)
, m_aOriginalName(rName)
: m_aOriginalName(rName)
, m_bIsRemoved(false)
, m_bIsInserted(bNew)
, m_bIsStorage(bStor)
......@@ -323,7 +323,8 @@ OStorage_Impl::~OStorage_Impl()
}
for (auto & pair : m_aChildrenMap)
delete pair.second;
for (auto pElement : pair.second)
delete pElement;
m_aChildrenMap.clear();
std::for_each(m_aDeletedVector.begin(), m_aDeletedVector.end(), std::default_delete<SotElement_Impl>());
......@@ -613,8 +614,7 @@ void OStorage_Impl::ReadContents()
xNewElement->m_bIsRemoved = true;
}
OUString name = xNewElement->m_aName; // because we're calling release in the next line
m_aChildrenMap.emplace(name, xNewElement.release());
m_aChildrenMap[aName].push_back(xNewElement.release());
}
}
catch( const container::NoSuchElementException& rNoSuchElementException )
......@@ -655,10 +655,10 @@ void OStorage_Impl::CopyToStorage( const uno::Reference< embed::XStorage >& xDes
throw embed::InvalidStorageException( THROW_WHERE );
for ( auto& pair : m_aChildrenMap )
for (auto pElement : pair.second)
{
auto & pElement = pair.second;
if ( !pElement->m_bIsRemoved )
CopyStorageElement( pElement, xDest, pElement->m_aName, bDirect );
CopyStorageElement( pElement, xDest, /*aName*/pair.first, bDirect );
}
// move storage properties to the destination one ( means changeable properties )
......@@ -1024,12 +1024,13 @@ void OStorage_Impl::Commit()
m_aDeletedVector.clear();
// remove removed elements
auto mapIter = m_aChildrenMap.begin();
while ( mapIter != m_aChildrenMap.end() )
for (auto mapIt = m_aChildrenMap.begin(); mapIt != m_aChildrenMap.end(); )
{
for (auto it = mapIt->second.begin(); it != mapIt->second.end(); )
{
// renamed and inserted elements must be really inserted to package later
// since they can conflict with removed elements
auto & pElement = mapIter->second;
auto & pElement = *it;
if ( pElement->m_bIsRemoved )
{
if ( m_nStorageType == embed::StorageFormats::OFOPXML && !pElement->m_bIsStorage )
......@@ -1040,18 +1041,24 @@ void OStorage_Impl::Commit()
xNewPackageFolder->removeByName( pElement->m_aOriginalName );
delete pElement;
mapIter = m_aChildrenMap.erase(mapIter);
it = mapIt->second.erase(it);
}
else
++it;
}
if (mapIt->second.empty())
mapIt = m_aChildrenMap.erase(mapIt);
else
++mapIter;
++mapIt;
}
// there should be no more deleted elements
for ( auto& pair : m_aChildrenMap )
for (auto pElement : pair.second)
{
// if it is a 'duplicate commit' inserted elements must be really inserted to package later
// since they can conflict with renamed elements
auto & pElement = pair.second;
if ( !pElement->m_bIsInserted )
{
// for now stream is opened in direct mode that means that in case
......@@ -1074,18 +1081,18 @@ void OStorage_Impl::Commit()
if ( m_bCommited || m_bIsRoot )
xNewPackageFolder->removeByName( pElement->m_aOriginalName );
pElement->m_xStorage->InsertIntoPackageFolder(pElement->m_aName, xNewPackageFolder);
pElement->m_xStorage->InsertIntoPackageFolder(/*aName*/pair.first, xNewPackageFolder);
}
else if (!pElement->m_bIsStorage && pElement->m_xStream && pElement->m_xStream->m_bFlushed)
{
if ( m_nStorageType == embed::StorageFormats::OFOPXML )
CommitStreamRelInfo( pElement );
CommitStreamRelInfo( /*aName*/pair.first, pElement );
// the renamed elements are not in new temporary storage
if ( m_bCommited || m_bIsRoot )
xNewPackageFolder->removeByName( pElement->m_aOriginalName );
pElement->m_xStream->InsertIntoPackageFolder(pElement->m_aName, xNewPackageFolder);
pElement->m_xStream->InsertIntoPackageFolder(/*aName*/pair.first, xNewPackageFolder);
}
else if ( !m_bCommited && !m_bIsRoot )
{
......@@ -1093,15 +1100,15 @@ void OStorage_Impl::Commit()
// the connection with the original package should not be lost just because
// the element is still referred by the folder in the original hierarchy
uno::Any aPackageElement = m_xPackageFolder->getByName( pElement->m_aOriginalName );
xNewPackageFolder->insertByName( pElement->m_aName, aPackageElement );
xNewPackageFolder->insertByName( /*aName*/pair.first, aPackageElement );
}
else if ( pElement->m_aName != pElement->m_aOriginalName )
else if ( pair.first != pElement->m_aOriginalName )
{
// this is the case when xNewPackageFolder refers to m_xPackageFolder
// in case the name was changed and it is not a changed storage - rename the element
uno::Any aPackageElement = xNewPackageFolder->getByName( pElement->m_aOriginalName );
xNewPackageFolder->removeByName( pElement->m_aOriginalName );
xNewPackageFolder->insertByName( pElement->m_aName, aPackageElement );
xNewPackageFolder->insertByName( /*aName*/pair.first, aPackageElement );
if ( m_nStorageType == embed::StorageFormats::OFOPXML && !pElement->m_bIsStorage )
{
......@@ -1112,21 +1119,21 @@ void OStorage_Impl::Commit()
throw uno::RuntimeException( THROW_WHERE );
}
CommitStreamRelInfo( pElement );
CommitStreamRelInfo( /*aName*/pair.first, pElement );
}
}
pElement->m_aOriginalName = pElement->m_aName;
pElement->m_aOriginalName = pair.first;
}
}
for ( auto& pair : m_aChildrenMap )
for (auto pElement : pair.second)
{
auto & pElement = pair.second;
// now inserted elements can be inserted to the package
if ( pElement->m_bIsInserted )
{
pElement->m_aOriginalName = pElement->m_aName;
pElement->m_aOriginalName = pair.first;
if ( pElement->m_bIsStorage )
{
......@@ -1136,7 +1143,7 @@ void OStorage_Impl::Commit()
if (!pElement->m_xStorage)
throw uno::RuntimeException( THROW_WHERE );
pElement->m_xStorage->InsertIntoPackageFolder(pElement->m_aName, xNewPackageFolder);
pElement->m_xStorage->InsertIntoPackageFolder(/*aName*/pair.first, xNewPackageFolder);
pElement->m_bIsInserted = false;
}
......@@ -1153,9 +1160,9 @@ void OStorage_Impl::Commit()
if (pElement->m_xStream->m_bFlushed)
{
if ( m_nStorageType == embed::StorageFormats::OFOPXML )
CommitStreamRelInfo( pElement );
CommitStreamRelInfo( /*aName*/pair.first, pElement );
pElement->m_xStream->InsertIntoPackageFolder( pElement->m_aName, xNewPackageFolder );
pElement->m_xStream->InsertIntoPackageFolder( /*aName*/pair.first, xNewPackageFolder );
pElement->m_bIsInserted = false;
}
......@@ -1217,38 +1224,31 @@ void OStorage_Impl::Revert()
// they will be created later on demand
// rebuild the map - cannot do it in-place, because we're changing some of the key values
std::unordered_map<OUString, SotElement_Impl*> oldMap;
std::unordered_map<OUString, std::vector<SotElement_Impl*>> oldMap;
std::swap(oldMap, m_aChildrenMap);
auto pElementIter = oldMap.begin();
while ( pElementIter != oldMap.end() )
for (auto & rPair : oldMap)
for (auto pElement : rPair.second)
{
auto & pElement = pElementIter->second;
if ( pElement->m_bIsInserted )
{
delete pElement;
}
else
{
ClearElement( pElement );
pElement->m_aName = pElement->m_aOriginalName;
pElement->m_bIsRemoved = false;
m_aChildrenMap.emplace(pElement->m_aName, pElement);
m_aChildrenMap[pElement->m_aOriginalName].push_back(pElement);
}
++pElementIter;
}
// return replaced removed elements
for ( auto& pDeleted : m_aDeletedVector )
{
// use original name as key, because we're updating m_aName below
m_aChildrenMap.emplace( pDeleted->m_aOriginalName, pDeleted );
m_aChildrenMap[pDeleted->m_aOriginalName].push_back(pDeleted);
ClearElement( pDeleted );
pDeleted->m_aName = pDeleted->m_aOriginalName;
pDeleted->m_bIsRemoved = false;
}
m_aDeletedVector.clear();
......@@ -1294,25 +1294,18 @@ SotElement_Impl* OStorage_Impl::FindElement( const OUString& rName )
{
::osl::MutexGuard aGuard( m_xMutex->GetMutex() );
auto it = FindElementIt(rName);
return it == m_aChildrenMap.end() ? nullptr : it->second;
}
std::unordered_map<OUString, SotElement_Impl*>::iterator OStorage_Impl::FindElementIt( const OUString& rName )
{
SAL_WARN_IF( rName.isEmpty(), "package.xstor", "Name is empty!" );
::osl::MutexGuard aGuard( m_xMutex->GetMutex() );
ReadContents();
auto pElementIter = m_aChildrenMap.find(rName);
if (pElementIter == m_aChildrenMap.end())
return m_aChildrenMap.end();
if (pElementIter->second->m_bIsRemoved)
return m_aChildrenMap.end();
auto mapIt = m_aChildrenMap.find(rName);
if (mapIt == m_aChildrenMap.end())
return nullptr;
for (auto pElement : mapIt->second)
if (!pElement->m_bIsRemoved)
return pElement;
return pElementIter;
return nullptr;
}
SotElement_Impl* OStorage_Impl::InsertStream( const OUString& aName, bool bEncr )
......@@ -1340,7 +1333,7 @@ SotElement_Impl* OStorage_Impl::InsertStream( const OUString& aName, bool bEncr
SotElement_Impl* pNewElement = InsertElement( aName, false );
pNewElement->m_xStream.reset(new OWriteStream_Impl(this, xPackageSubStream, m_xPackage, m_xContext, bEncr, m_nStorageType, true));
m_aChildrenMap.emplace( pNewElement->m_aName, pNewElement );
m_aChildrenMap[aName].push_back( pNewElement );
m_bIsModified = true;
m_bBroadcastModified = true;
......@@ -1379,7 +1372,7 @@ void OStorage_Impl::InsertRawStream( const OUString& aName, const uno::Reference
// the stream is inserted and must be treated as a committed one
pNewElement->m_xStream->SetToBeCommited();
m_aChildrenMap.emplace( pNewElement->m_aName, pNewElement );
m_aChildrenMap[aName].push_back( pNewElement );
m_bIsModified = true;
m_bBroadcastModified = true;
}
......@@ -1413,21 +1406,23 @@ SotElement_Impl* OStorage_Impl::InsertStorage( const OUString& aName, sal_Int32
pNewElement->m_xStorage = CreateNewStorageImpl(nStorageMode);
m_aChildrenMap.emplace( pNewElement->m_aName, pNewElement );
m_aChildrenMap[aName].push_back( pNewElement );
return pNewElement;
}
SotElement_Impl* OStorage_Impl::InsertElement( const OUString& aName, bool bIsStorage )
{
assert( FindElement(aName) == nullptr && "Should not try to insert existing element");
::osl::MutexGuard aGuard( m_xMutex->GetMutex() );
SotElement_Impl* pDeletedElm = nullptr;
auto it = m_aChildrenMap.find(aName);
if (it != m_aChildrenMap.end())
for (auto pElement : it->second)
{
auto pElement = it->second;
SAL_WARN_IF( !pElement->m_bIsRemoved, "package.xstor", "Try to insert an element instead of existing one!" );
if ( pElement->m_bIsRemoved )
{
......@@ -1443,7 +1438,10 @@ SotElement_Impl* OStorage_Impl::InsertElement( const OUString& aName, bool bIsSt
else
OpenSubStream( pDeletedElm );
m_aChildrenMap.erase(it);
auto & rVec = m_aChildrenMap[aName];
rVec.erase(std::remove(rVec.begin(), rVec.end(), pDeletedElm), rVec.end());
if (rVec.empty())
m_aChildrenMap.erase(aName);
m_aDeletedVector.push_back( pDeletedElm );
}
......@@ -1501,41 +1499,46 @@ uno::Sequence< OUString > OStorage_Impl::GetElementNames()
ReadContents();
sal_uInt32 nSize = m_aChildrenMap.size();
uno::Sequence< OUString > aElementNames( nSize );
std::vector< OUString > aElementNames;
aElementNames.reserve( m_aChildrenMap.size() );
sal_uInt32 nInd = 0;
for ( const auto& pair : m_aChildrenMap )
for (auto pElement : pair.second)
{
auto pElement = pair.second;
if ( !pElement->m_bIsRemoved )
aElementNames[nInd++] = pElement->m_aName;
aElementNames.push_back(pair.first);
}
aElementNames.realloc( nInd );
return aElementNames;
return comphelper::containerToSequence(aElementNames);
}
std::unordered_map<OUString, SotElement_Impl*>::iterator OStorage_Impl::RemoveElement( std::unordered_map<OUString, SotElement_Impl*>::iterator pElementIter )
void OStorage_Impl::RemoveElement( OUString const & rName, SotElement_Impl* pElement )
{
auto pElement = pElementIter->second;
assert(pElement);
if ( (pElement->m_xStorage && ( pElement->m_xStorage->m_pAntiImpl || !pElement->m_xStorage->m_aReadOnlyWrapVector.empty() ))
|| (pElement->m_xStream && ( pElement->m_xStream->m_pAntiImpl || !pElement->m_xStream->m_aInputStreamsVector.empty() )) )
throw io::IOException( THROW_WHERE ); // TODO: Access denied
auto mapIt = m_aChildrenMap.find(rName);
for (auto it = mapIt->second.begin(); it != mapIt->second.end(); ++it)
if (pElement == *it)
{
if ( pElement->m_bIsInserted )
{
delete pElementIter->second;
return m_aChildrenMap.erase(pElementIter);
delete pElement;
mapIt->second.erase(std::remove(mapIt->second.begin(), mapIt->second.end(), pElement), mapIt->second.end());
if (mapIt->second.empty())
m_aChildrenMap.erase(mapIt);
}
else
{
pElement->m_bIsRemoved = true;
ClearElement( pElement );
++pElementIter;
return pElementIter;
}
return;
}
assert(false && "not found");
// TODO/OFOPXML: the rel stream should be removed as well
}
......@@ -1622,7 +1625,7 @@ void OStorage_Impl::CreateRelStorage()
}
}
void OStorage_Impl::CommitStreamRelInfo( SotElement_Impl const * pStreamElement )
void OStorage_Impl::CommitStreamRelInfo( const OUString &rName, SotElement_Impl const * pStreamElement )
{
// this method should be used only in OStorage_Impl::Commit() method
......@@ -1632,7 +1635,7 @@ void OStorage_Impl::CommitStreamRelInfo( SotElement_Impl const * pStreamElement
if (m_nStorageType == embed::StorageFormats::OFOPXML && pStreamElement->m_xStream)
{
SAL_WARN_IF( pStreamElement->m_aName.isEmpty(), "package.xstor", "The name must not be empty!" );
SAL_WARN_IF( rName.isEmpty(), "package.xstor", "The name must not be empty!" );
if ( !m_xRelStorage.is() )
{
......@@ -1640,7 +1643,7 @@ void OStorage_Impl::CommitStreamRelInfo( SotElement_Impl const * pStreamElement
CreateRelStorage();
}
pStreamElement->m_xStream->CommitStreamRelInfo(m_xRelStorage, pStreamElement->m_aOriginalName, pStreamElement->m_aName);
pStreamElement->m_xStream->CommitStreamRelInfo(m_xRelStorage, pStreamElement->m_aOriginalName, rName);
}
}
......@@ -2395,9 +2398,9 @@ uno::Reference< embed::XStorage > SAL_CALL OStorage::openStorageElement(
if ( nStorageMode & embed::ElementModes::TRUNCATE )
{
auto pElementToDelIter = pElement->m_xStorage->m_aChildrenMap.begin();
while (pElementToDelIter != pElement->m_xStorage->m_aChildrenMap.end())
pElementToDelIter = m_pImpl->RemoveElement( pElementToDelIter );
for (auto & rPair : pElement->m_xStorage->m_aChildrenMap)
for (auto pElementToDel : rPair.second)
m_pImpl->RemoveElement( /*aName*/rPair.first, pElementToDel );
}
}
}
......@@ -2803,11 +2806,11 @@ void SAL_CALL OStorage::removeElement( const OUString& aElementName )
try
{
auto pElementIter = m_pImpl->FindElementIt(aElementName);
if ( pElementIter == m_pImpl->m_aChildrenMap.end() )
auto pElement = m_pImpl->FindElement(aElementName);
if ( !pElement )
throw container::NoSuchElementException(THROW_WHERE); //???
m_pImpl->RemoveElement(pElementIter);
m_pImpl->RemoveElement(aElementName, pElement);
m_pImpl->m_bIsModified = true;
m_pImpl->m_bBroadcastModified = true;
......@@ -2887,15 +2890,21 @@ void SAL_CALL OStorage::renameElement( const OUString& aElementName, const OUStr
if (pRefElement)
throw container::ElementExistException(THROW_WHERE); //???
auto pElementIt = m_pImpl->FindElementIt( aElementName );
if ( pElementIt == m_pImpl->m_aChildrenMap.end() )
auto pElement = m_pImpl->FindElement( aElementName );
if ( !pElement )
throw container::NoSuchElementException(THROW_WHERE); //???
auto pElement = pElementIt->second;
pElement->m_aName = aNewName;
m_pImpl->m_aChildrenMap.erase( pElementIt );
m_pImpl->m_aChildrenMap.emplace(pElement->m_aName, pElement);
auto mapIt = m_pImpl->m_aChildrenMap.find(aElementName);
auto rVec = mapIt->second;
for (auto it = rVec.begin(); it != rVec.end(); ++it)
if (pElement == *it)
{
rVec.erase(std::remove(rVec.begin(), rVec.end(), pElement), rVec.end());
if (rVec.empty())
m_pImpl->m_aChildrenMap.erase(mapIt);
break;
}
m_pImpl->m_aChildrenMap[aNewName].push_back(pElement);
m_pImpl->m_bIsModified = true;
m_pImpl->m_bBroadcastModified = true;
}
......@@ -3064,17 +3073,17 @@ void SAL_CALL OStorage::moveElementTo( const OUString& aElementName,
try
{
auto pElementIter = m_pImpl->FindElementIt( aElementName );
if ( pElementIter == m_pImpl->m_aChildrenMap.end() )
auto pElement = m_pImpl->FindElement( aElementName );
if ( !pElement )
throw container::NoSuchElementException(THROW_WHERE); //???
uno::Reference<XNameAccess> xNameAccess(xDest, uno::UNO_QUERY_THROW);
if (xNameAccess->hasByName(aNewName))
throw container::ElementExistException(THROW_WHERE);
m_pImpl->CopyStorageElement(pElementIter->second, xDest, aNewName, false);
m_pImpl->CopyStorageElement(pElement, xDest, aNewName, false);
m_pImpl->RemoveElement(pElementIter);
m_pImpl->RemoveElement(aElementName, pElement);
m_pImpl->m_bIsModified = true;
m_pImpl->m_bBroadcastModified = true;
......@@ -3623,19 +3632,18 @@ void SAL_CALL OStorage::revert()
throw lang::DisposedException(THROW_WHERE);
}
bool bThrow = std::any_of(
m_pImpl->m_aChildrenMap.begin(), m_pImpl->m_aChildrenMap.end(),
[](decltype(m_pImpl->m_aChildrenMap)::value_type const & pair) {
auto pElement = pair.second;
return (pElement->m_xStorage
for (auto & rPair : m_pImpl->m_aChildrenMap)
for (auto pElement : rPair.second)
{
bool bThrow = (pElement->m_xStorage
&& (pElement->m_xStorage->m_pAntiImpl
|| !pElement->m_xStorage->m_aReadOnlyWrapVector.empty()))
|| (pElement->m_xStream
&& (pElement->m_xStream->m_pAntiImpl
|| !pElement->m_xStream->m_aInputStreamsVector.empty()));
});
if (bThrow)
throw io::IOException(THROW_WHERE); // TODO: access denied
}
if (m_pData->m_bReadOnlyWrap || !m_pImpl->m_bListCreated)
return; // nothing to do
......
......@@ -80,7 +80,6 @@ struct OWriteStream_Impl;
struct SotElement_Impl
{
OUString m_aName;
OUString m_aOriginalName;
bool m_bIsRemoved;
bool m_bIsInserted;
......@@ -139,7 +138,7 @@ struct OStorage_Impl
return m_nModifiedListenerCount > 0 && m_pAntiImpl != nullptr;
}
std::unordered_map<OUString, SotElement_Impl*> m_aChildrenMap;
std::unordered_map<OUString, std::vector<SotElement_Impl*>> m_aChildrenMap;
SotElementVector_Impl m_aDeletedVector;
css::uno::Reference< css::container::XNameContainer > m_xPackageFolder;
......@@ -229,7 +228,6 @@ struct OStorage_Impl
bool bDirect );
SotElement_Impl* FindElement( const OUString& rName );
std::unordered_map<OUString, SotElement_Impl*>::iterator FindElementIt( const OUString& rName );
SotElement_Impl* InsertStream( const OUString& aName, bool bEncr );
void InsertRawStream( const OUString& aName, const css::uno::Reference< css::io::XInputStream >& xInStream );
......@@ -243,7 +241,7 @@ struct OStorage_Impl
css::uno::Sequence< OUString > GetElementNames();
std::unordered_map<OUString, SotElement_Impl*>::iterator RemoveElement( std::unordered_map<OUString, SotElement_Impl*>::iterator pElement );
void RemoveElement( OUString const & rName, SotElement_Impl* pElement );
static void ClearElement( SotElement_Impl* pElement );
/// @throws css::embed::InvalidStorageException
......@@ -262,7 +260,7 @@ struct OStorage_Impl
void RemoveStreamRelInfo( const OUString& aOriginalName );
void CreateRelStorage();
void CommitStreamRelInfo( SotElement_Impl const * pStreamElement );
void CommitStreamRelInfo( const OUString& rName, SotElement_Impl const * pStreamElement );
css::uno::Reference< css::io::XInputStream > GetRelInfoStreamForName( const OUString& aName );
void CommitRelInfo( const css::uno::Reference< css::container::XNameContainer >& xNewPackageFolder );
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment