From 5195530f3ac43f071bc5676fd7ad1a0adc63e9d8 Mon Sep 17 00:00:00 2001 From: Michael Stahl Date: Fri, 14 Apr 2023 18:48:44 +0200 Subject: xmlhelp: fix lots of deadlocks Not sure if any of this makes sense but it doesn't deadlock immediatly. (regression from comit 2c37a0ca31095165d05740a2ff4969f625b4a75b) Change-Id: I4a5e01acb06b56a78453900a143d36cad1156f21 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/150431 Tested-by: Jenkins Reviewed-by: Michael Stahl --- xmlhelp/source/cxxhelp/provider/databases.cxx | 120 ++++++++++++++++++-------- xmlhelp/source/cxxhelp/provider/databases.hxx | 27 ++++-- 2 files changed, 104 insertions(+), 43 deletions(-) (limited to 'xmlhelp') diff --git a/xmlhelp/source/cxxhelp/provider/databases.cxx b/xmlhelp/source/cxxhelp/provider/databases.cxx index be5f964ca292..351625e7dbcb 100644 --- a/xmlhelp/source/cxxhelp/provider/databases.cxx +++ b/xmlhelp/source/cxxhelp/provider/databases.cxx @@ -277,6 +277,11 @@ OUString Databases::getInstallPathAsURL() return m_aInstallDirectory; } +OUString Databases::getInstallPathAsURL(std::unique_lock& ) +{ + return m_aInstallDirectory; +} + const std::vector< OUString >& Databases::getModuleList( const OUString& Language ) { if( m_avModules.empty() ) @@ -430,20 +435,29 @@ OUString Databases::processLang( std::unique_lock& /*rGuard*/, const return ret; } -helpdatafileproxy::Hdf* Databases::getHelpDataFile( std::u16string_view Database, +helpdatafileproxy::Hdf* Databases::getHelpDataFile(std::u16string_view Database, const OUString& Language, bool helpText, const OUString* pExtensionPath ) +{ + std::unique_lock aGuard( m_aMutex ); + + return getHelpDataFile(aGuard, Database, Language, helpText, pExtensionPath); +} + +helpdatafileproxy::Hdf* Databases::getHelpDataFile(std::unique_lock& rGuard, + std::u16string_view Database, + const OUString& Language, bool helpText, + const OUString* pExtensionPath ) + { if( Database.empty() || Language.isEmpty() ) return nullptr; - std::unique_lock aGuard( m_aMutex ); - OUString aFileExt( helpText ? OUString(".ht") : OUString(".db") ); OUString dbFileName = OUString::Concat("/") + Database + aFileExt; OUString key; if( pExtensionPath == nullptr ) - key = processLang( aGuard, Language ) + dbFileName; + key = processLang( rGuard, Language ) + dbFileName; else key = *pExtensionPath + Language + dbFileName; // make unique, don't change language @@ -458,7 +472,7 @@ helpdatafileproxy::Hdf* Databases::getHelpDataFile( std::u16string_view Database OUString fileURL; if( pExtensionPath ) - fileURL = expandURL(aGuard, *pExtensionPath) + Language + dbFileName; + fileURL = expandURL(rGuard, *pExtensionPath) + Language + dbFileName; else fileURL = m_aInstallDirectory + key; @@ -480,12 +494,10 @@ helpdatafileproxy::Hdf* Databases::getHelpDataFile( std::u16string_view Database } Reference< XCollator > -Databases::getCollator( const OUString& Language ) +Databases::getCollator(std::unique_lock&, const OUString& Language) { OUString key = Language; - std::unique_lock aGuard( m_aMutex ); - CollatorTable::iterator it = m_aCollatorTable.emplace( key, Reference< XCollator >() ).first; @@ -728,7 +740,7 @@ KeywordInfo* Databases::getKeyword( const OUString& Database, bool bExtension = false; for (;;) { - fileURL = aDbFileIt.nextDbFile( bExtension ); + fileURL = aDbFileIt.nextDbFile(aGuard, bExtension); if( fileURL.isEmpty() ) break; OUString fileNameHDFHelp( fileURL ); @@ -741,7 +753,7 @@ KeywordInfo* Databases::getKeyword( const OUString& Database, helpdatafileproxy::HDFData aValue; if( aHdf.startIteration() ) { - helpdatafileproxy::Hdf* pHdf = getHelpDataFile( Database,Language ); + helpdatafileproxy::Hdf* pHdf = getHelpDataFile(aGuard, Database,Language ); if( pHdf != nullptr ) { pHdf->releaseHashMap(); @@ -776,7 +788,7 @@ KeywordInfo* Databases::getKeyword( const OUString& Database, } // sorting - Reference< XCollator > xCollator = getCollator( Language ); + Reference xCollator = getCollator(aGuard, Language); KeywordElementComparator aComparator( xCollator ); std::sort(aVector.begin(),aVector.end(),aComparator); @@ -786,7 +798,8 @@ KeywordInfo* Databases::getKeyword( const OUString& Database, return it->second.get(); } -Reference< XHierarchicalNameAccess > Databases::jarFile( std::u16string_view jar, +Reference< XHierarchicalNameAccess > Databases::jarFile( + std::unique_lock& rGuard, std::u16string_view jar, const OUString& Language ) { if( jar.empty() || Language.isEmpty() ) @@ -794,9 +807,7 @@ Reference< XHierarchicalNameAccess > Databases::jarFile( std::u16string_view jar return Reference< XHierarchicalNameAccess >( nullptr ); } - std::unique_lock aGuard( m_aMutex ); - - OUString key = processLang(aGuard, Language) + "/" + jar; + OUString key = processLang(rGuard, Language) + "/" + jar; ZipFileTable::iterator it = m_aZipFileTable.emplace( key,Reference< XHierarchicalNameAccess >(nullptr) ).first; @@ -814,7 +825,7 @@ Reference< XHierarchicalNameAccess > Databases::jarFile( std::u16string_view jar std::u16string_view aExtensionPath = jar.substr( nQuestionMark1 + 1, nQuestionMark2 - nQuestionMark1 - 1 ); std::u16string_view aPureJar = jar.substr( nQuestionMark2 + 1 ); - zipFile = expandURL( aGuard, OUString::Concat(aExtensionPath) + "/" + aPureJar ); + zipFile = expandURL(rGuard, OUString::Concat(aExtensionPath) + "/" + aPureJar); } else { @@ -878,12 +889,14 @@ Reference< XHierarchicalNameAccess > Databases::findJarFileForPath return xNA; } + ::std::unique_lock aGuard(m_aMutex); + JarFileIterator aJarFileIt( m_xContext, *this, jar, Language ); Reference< XHierarchicalNameAccess > xTestNA; Reference< deployment::XPackage > xParentPackageBundle; for (;;) { - xTestNA = aJarFileIt.nextJarFile( xParentPackageBundle, o_pExtensionPath, o_pExtensionRegistryPath ); + xTestNA = aJarFileIt.nextJarFile(aGuard, xParentPackageBundle, o_pExtensionPath, o_pExtensionRegistryPath); if( !xTestNA.is() ) break; if( xTestNA.is() && xTestNA->hasByHierarchicalName( path ) ) @@ -1291,6 +1304,40 @@ Reference< deployment::XPackage > ExtensionIteratorBase::implGetNextBundledHelpP return xHelpPackage; } +OUString ExtensionIteratorBase::implGetFileFromPackage( + std::unique_lock & rGuard, + std::u16string_view rFileExtension, const Reference< deployment::XPackage >& xPackage ) +{ + // No extension -> search for pure language folder + bool bLangFolderOnly = rFileExtension.empty(); + + OUString aFile; + OUString aLanguage = m_aLanguage; + for( sal_Int32 iPass = 0 ; iPass < 2 ; ++iPass ) + { + OUString aStr = xPackage->getRegistrationDataURL().Value + "/" + aLanguage; + if( !bLangFolderOnly ) + { + aStr += OUString::Concat("/help") + rFileExtension; + } + + aFile = m_rDatabases.expandURL(rGuard, aStr); + if( iPass == 0 ) + { + if( m_xSFA->exists( aFile ) ) + break; + + ::std::vector< OUString > av; + implGetLanguageVectorFromPackage( av, xPackage ); + ::std::vector< OUString >::const_iterator pFound = LanguageTag::getFallback( av, m_aLanguage ); + if( pFound != av.end() ) + aLanguage = *pFound; + } + } + return aFile; +} + + OUString ExtensionIteratorBase::implGetFileFromPackage( std::u16string_view rFileExtension, const Reference< deployment::XPackage >& xPackage ) { @@ -1467,7 +1514,7 @@ helpdatafileproxy::Hdf* DataBaseIterator::implGetHdfFromPackage( const Reference //returns a file URL -OUString KeyDataBaseFileIterator::nextDbFile( bool& o_rbExtension ) +OUString KeyDataBaseFileIterator::nextDbFile(std::unique_lock& rGuard, bool& o_rbExtension) { OUString aRetFile; @@ -1476,8 +1523,8 @@ OUString KeyDataBaseFileIterator::nextDbFile( bool& o_rbExtension ) switch( m_eState ) { case IteratorState::InitialModule: - aRetFile = m_rDatabases.getInstallPathAsURL() + - m_rDatabases.processLang(m_aLanguage) + + aRetFile = m_rDatabases.getInstallPathAsURL(rGuard) + + m_rDatabases.processLang(rGuard, m_aLanguage) + "/" + m_aInitialModule + ".key"; @@ -1497,7 +1544,7 @@ OUString KeyDataBaseFileIterator::nextDbFile( bool& o_rbExtension ) if( !xHelpPackage.is() ) break; - aRetFile = implGetDbFileFromPackage( xHelpPackage ); + aRetFile = implGetDbFileFromPackage(rGuard, xHelpPackage); o_rbExtension = true; break; } @@ -1509,7 +1556,7 @@ OUString KeyDataBaseFileIterator::nextDbFile( bool& o_rbExtension ) if( !xHelpPackage.is() ) break; - aRetFile = implGetDbFileFromPackage( xHelpPackage ); + aRetFile = implGetDbFileFromPackage(rGuard, xHelpPackage); o_rbExtension = true; break; } @@ -1521,7 +1568,7 @@ OUString KeyDataBaseFileIterator::nextDbFile( bool& o_rbExtension ) if( !xHelpPackage.is() ) break; - aRetFile = implGetDbFileFromPackage( xHelpPackage ); + aRetFile = implGetDbFileFromPackage(rGuard, xHelpPackage); o_rbExtension = true; break; } @@ -1536,18 +1583,20 @@ OUString KeyDataBaseFileIterator::nextDbFile( bool& o_rbExtension ) } //Returns a file URL, that does not contain macros -OUString KeyDataBaseFileIterator::implGetDbFileFromPackage - ( const Reference< deployment::XPackage >& xPackage ) +OUString KeyDataBaseFileIterator::implGetDbFileFromPackage( + std::unique_lock& rGuard, + const Reference& xPackage) { OUString aExpandedURL = - implGetFileFromPackage( u".key", xPackage ); + implGetFileFromPackage(rGuard, u".key", xPackage); return aExpandedURL; } -Reference< XHierarchicalNameAccess > JarFileIterator::nextJarFile - ( Reference< deployment::XPackage >& o_xParentPackageBundle, +Reference JarFileIterator::nextJarFile( + std::unique_lock& rGuard, + Reference< deployment::XPackage >& o_xParentPackageBundle, OUString* o_pExtensionPath, OUString* o_pExtensionRegistryPath ) { Reference< XHierarchicalNameAccess > xNA; @@ -1557,7 +1606,7 @@ Reference< XHierarchicalNameAccess > JarFileIterator::nextJarFile switch( m_eState ) { case IteratorState::InitialModule: - xNA = m_rDatabases.jarFile( m_aInitialModule, m_aLanguage ); + xNA = m_rDatabases.jarFile(rGuard, m_aInitialModule, m_aLanguage); m_eState = IteratorState::UserExtensions; // Later: SHARED_MODULE break; @@ -1571,7 +1620,7 @@ Reference< XHierarchicalNameAccess > JarFileIterator::nextJarFile if( !xHelpPackage.is() ) break; - xNA = implGetJarFromPackage( xHelpPackage, o_pExtensionPath, o_pExtensionRegistryPath ); + xNA = implGetJarFromPackage(rGuard, xHelpPackage, o_pExtensionPath, o_pExtensionRegistryPath); break; } @@ -1581,7 +1630,7 @@ Reference< XHierarchicalNameAccess > JarFileIterator::nextJarFile if( !xHelpPackage.is() ) break; - xNA = implGetJarFromPackage( xHelpPackage, o_pExtensionPath, o_pExtensionRegistryPath ); + xNA = implGetJarFromPackage(rGuard, xHelpPackage, o_pExtensionPath, o_pExtensionRegistryPath); break; } @@ -1591,7 +1640,7 @@ Reference< XHierarchicalNameAccess > JarFileIterator::nextJarFile if( !xHelpPackage.is() ) break; - xNA = implGetJarFromPackage( xHelpPackage, o_pExtensionPath, o_pExtensionRegistryPath ); + xNA = implGetJarFromPackage(rGuard, xHelpPackage, o_pExtensionPath, o_pExtensionRegistryPath); break; } @@ -1604,13 +1653,14 @@ Reference< XHierarchicalNameAccess > JarFileIterator::nextJarFile return xNA; } -Reference< XHierarchicalNameAccess > JarFileIterator::implGetJarFromPackage -( const Reference< deployment::XPackage >& xPackage, OUString* o_pExtensionPath, OUString* o_pExtensionRegistryPath ) +Reference< XHierarchicalNameAccess > JarFileIterator::implGetJarFromPackage( + std::unique_lock& rGuard, + const Reference& xPackage, OUString* o_pExtensionPath, OUString* o_pExtensionRegistryPath) { Reference< XHierarchicalNameAccess > xNA; OUString zipFile = - implGetFileFromPackage( u".jar", xPackage ); + implGetFileFromPackage(rGuard, u".jar", xPackage); try { diff --git a/xmlhelp/source/cxxhelp/provider/databases.hxx b/xmlhelp/source/cxxhelp/provider/databases.hxx index 1711cba5278c..448d1f92cd71 100644 --- a/xmlhelp/source/cxxhelp/provider/databases.hxx +++ b/xmlhelp/source/cxxhelp/provider/databases.hxx @@ -146,6 +146,7 @@ namespace chelp { OString getImageTheme() const; OUString getInstallPathAsURL(); + OUString getInstallPathAsURL(std::unique_lock& rGuard); const std::vector< OUString >& getModuleList( const OUString& Language ); @@ -159,12 +160,17 @@ namespace chelp { helpdatafileproxy::Hdf* getHelpDataFile( std::u16string_view Module, const OUString& Language, bool helpText = false, const OUString* pExtensionPath = nullptr ); + helpdatafileproxy::Hdf* getHelpDataFile(std::unique_lock& rGuard, + std::u16string_view Module, + const OUString& Language, bool helpText = false, + const OUString* pExtensionPath = nullptr ); + /** * The following method returns the Collator for the given language-country combination */ css::uno::Reference< css::i18n::XCollator > - getCollator( const OUString& Language ); + getCollator(std::unique_lock& rGuard, const OUString& Language); /** * Returns the cascading style sheet used to format the HTML-output. @@ -194,7 +200,7 @@ namespace chelp { */ css::uno::Reference< css::container::XHierarchicalNameAccess > - jarFile( std::u16string_view jar, + jarFile(std::unique_lock& rGuard, std::u16string_view jar, const OUString& Language ); css::uno::Reference< css::container::XHierarchicalNameAccess > @@ -206,6 +212,7 @@ namespace chelp { * Maps a given language-locale combination to language or locale. */ OUString processLang( const OUString& Language ); + OUString processLang( std::unique_lock& rGuard, const OUString& Language ); void replaceName( OUString& oustring ) const; @@ -213,13 +220,12 @@ namespace chelp { const OUString& getProductVersion() const { return m_vReplacement[1]; } OUString expandURL( const OUString& aURL ); + OUString expandURL( std::unique_lock& rGuard, const OUString& aURL ); static OUString expandURL( const OUString& aURL, const css::uno::Reference< css::uno::XComponentContext >& xContext ); private: - OUString expandURL( std::unique_lock& rGuard, const OUString& aURL ); - OUString processLang( std::unique_lock& rGuard, const OUString& Language ); std::mutex m_aMutex; css::uno::Reference< css::uno::XComponentContext > m_xContext; @@ -327,6 +333,9 @@ namespace chelp { ( css::uno::Reference< css::deployment::XPackage >& o_xParentPackageBundle ); OUString implGetFileFromPackage( std::u16string_view rFileExtension, const css::uno::Reference< css::deployment::XPackage >& xPackage ); + OUString implGetFileFromPackage(std::unique_lock& rGuard, + std::u16string_view rFileExtension, + const css::uno::Reference< css::deployment::XPackage >& xPackage ); void implGetLanguageVectorFromPackage( ::std::vector< OUString > &rv, const css::uno::Reference< css::deployment::XPackage >& xPackage ); @@ -390,10 +399,10 @@ namespace chelp { : ExtensionIteratorBase( xContext, rDatabases, aInitialModule, aLanguage ) {} //Returns a file URL - OUString nextDbFile( bool& o_rbExtension ); + OUString nextDbFile(std::unique_lock& rGuard, bool& o_rbExtension); private: - OUString implGetDbFileFromPackage( + OUString implGetDbFileFromPackage(std::unique_lock& rGuard, const css::uno::Reference< css::deployment::XPackage >& xPackage ); }; // end class KeyDataBaseFileIterator @@ -407,12 +416,14 @@ namespace chelp { {} css::uno::Reference< css::container::XHierarchicalNameAccess > - nextJarFile( css::uno::Reference< css::deployment::XPackage >& o_xParentPackageBundle, + nextJarFile(std::unique_lock& rGuard, + css::uno::Reference& o_xParentPackageBundle, OUString* o_pExtensionPath, OUString* o_pExtensionRegistryPath ); private: css::uno::Reference< css::container::XHierarchicalNameAccess > - implGetJarFromPackage(const css::uno::Reference< css::deployment::XPackage >& xPackage, + implGetJarFromPackage(std::unique_lock& rGuard, + const css::uno::Reference< css::deployment::XPackage >& xPackage, OUString* o_pExtensionPath, OUString* o_pExtensionRegistryPath ); }; // end class JarFileIterator -- cgit