try to fix shutdown crashes in ConfigurationWrapper

associated with
    commit 7df433cdc3
    optimize ConfigurationProperty::get()

it seems it introduced
https://crashreport.libreoffice.org/stats/signature/void%20rtl::str::release%3C_rtl_uString%3E(_rtl_uString*)
which, at the moment, is the most reported crash in
https://crashreport.libreoffice.org/stats/version/7.4.3.2

We need to tie the lifetime of ConfigurationWrapper and the cache
objects together, so they die as one unit, otherwise the
unpredicatable ordering of destruction of static objects is a
problem.

Change-Id: I80db5a4c28510a68f40b919902b01f3981e32bfb
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/144840
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
This commit is contained in:
Noel Grandin
2022-12-27 17:23:17 +00:00
committed by Noel Grandin
parent 63dcb9b5ee
commit 1b9ca3689d
2 changed files with 44 additions and 35 deletions
+27 -32
View File
@@ -10,9 +10,6 @@
#include <sal/config.h>
#include <cassert>
#include <mutex>
#include <string_view>
#include <unordered_map>
#include <com/sun/star/beans/NamedValue.hpp>
#include <com/sun/star/beans/PropertyAttribute.hpp>
@@ -24,11 +21,12 @@
#include <com/sun/star/container/XHierarchicalNameReplace.hpp>
#include <com/sun/star/container/XNameAccess.hpp>
#include <com/sun/star/container/XNameContainer.hpp>
#include <com/sun/star/util/XChangesListener.hpp>
#include <com/sun/star/util/XChangesNotifier.hpp>
#include <com/sun/star/lang/DisposedException.hpp>
#include <com/sun/star/lang/XLocalizable.hpp>
#include <com/sun/star/uno/Any.hxx>
#include <com/sun/star/uno/Reference.hxx>
#include <com/sun/star/util/XChangesListener.hpp>
#include <com/sun/star/util/XChangesNotifier.hpp>
#include <comphelper/solarmutex.hxx>
#include <comphelper/configuration.hxx>
#include <comphelper/configurationlistener.hxx>
@@ -109,37 +107,32 @@ comphelper::detail::ConfigurationWrapper::get()
return WRAPPER;
}
namespace
{
std::mutex gMutex;
std::unordered_map<OUString, css::uno::Any> gPropertyCache;
css::uno::Reference< css::util::XChangesNotifier > gNotifier;
css::uno::Reference< css::util::XChangesListener > gListener;
class ConfigurationChangesListener
class comphelper::detail::ConfigurationChangesListener
: public ::cppu::WeakImplHelper<css::util::XChangesListener>
{
comphelper::detail::ConfigurationWrapper& mrConfigurationWrapper;
public:
ConfigurationChangesListener()
ConfigurationChangesListener(comphelper::detail::ConfigurationWrapper& rWrapper)
: mrConfigurationWrapper(rWrapper)
{}
// util::XChangesListener
virtual void SAL_CALL changesOccurred( const css::util::ChangesEvent& ) override
{
std::scoped_lock aGuard(gMutex);
gPropertyCache.clear();
std::scoped_lock aGuard(mrConfigurationWrapper.maMutex);
mrConfigurationWrapper.maPropertyCache.clear();
}
virtual void SAL_CALL disposing(const css::lang::EventObject&) override
{
std::scoped_lock aGuard(gMutex);
gPropertyCache.clear();
std::scoped_lock aGuard(mrConfigurationWrapper.maMutex);
mrConfigurationWrapper.mbDisposed = true;
mrConfigurationWrapper.maPropertyCache.clear();
}
};
} // namespace
comphelper::detail::ConfigurationWrapper::ConfigurationWrapper():
context_(comphelper::getProcessComponentContext()),
access_(css::configuration::ReadWriteAccess::create(context_, "*"))
access_(css::configuration::ReadWriteAccess::create(context_, "*")),
mbDisposed(false)
{
// Set up a configuration notifier to invalidate the cache as needed.
try
@@ -156,10 +149,10 @@ comphelper::detail::ConfigurationWrapper::ConfigurationWrapper():
= xConfigProvider->createInstanceWithArguments(u"com.sun.star.configuration.ConfigurationAccess",
params);
gNotifier = css::uno::Reference< css::util::XChangesNotifier >(xCfg, css::uno::UNO_QUERY);
assert(gNotifier.is());
gListener = css::uno::Reference< ConfigurationChangesListener >(new ConfigurationChangesListener());
gNotifier->addChangesListener(gListener);
maNotifier = css::uno::Reference< css::util::XChangesNotifier >(xCfg, css::uno::UNO_QUERY);
assert(maNotifier.is());
maListener = css::uno::Reference< ConfigurationChangesListener >(new ConfigurationChangesListener(*this));
maNotifier->addChangesListener(maListener);
}
catch(const css::uno::Exception&)
{
@@ -169,9 +162,9 @@ comphelper::detail::ConfigurationWrapper::ConfigurationWrapper():
comphelper::detail::ConfigurationWrapper::~ConfigurationWrapper()
{
gPropertyCache.clear();
gNotifier.clear();
gListener.clear();
maPropertyCache.clear();
maNotifier.clear();
maListener.clear();
}
bool comphelper::detail::ConfigurationWrapper::isReadOnly(OUString const & path)
@@ -185,10 +178,12 @@ bool comphelper::detail::ConfigurationWrapper::isReadOnly(OUString const & path)
css::uno::Any comphelper::detail::ConfigurationWrapper::getPropertyValue(OUString const& path) const
{
std::scoped_lock aGuard(gMutex);
std::scoped_lock aGuard(maMutex);
if (mbDisposed)
throw css::lang::DisposedException();
// Cache the configuration access, since some of the keys are used in hot code.
auto it = gPropertyCache.find(path);
if( it != gPropertyCache.end())
auto it = maPropertyCache.find(path);
if( it != maPropertyCache.end())
return it->second;
sal_Int32 idx = path.lastIndexOf("/");
@@ -199,7 +194,7 @@ css::uno::Any comphelper::detail::ConfigurationWrapper::getPropertyValue(OUStrin
css::uno::Reference<css::container::XNameAccess> access(
access_->getByHierarchicalName(parentPath), css::uno::UNO_QUERY_THROW);
css::uno::Any property = access->getByName(childName);
gPropertyCache.emplace(path, property);
maPropertyCache.emplace(path, property);
return property;
}
+17 -3
View File
@@ -12,15 +12,16 @@
#include <sal/config.h>
#include <optional>
#include <string_view>
#include <com/sun/star/uno/Any.hxx>
#include <com/sun/star/uno/Reference.h>
#include <comphelper/comphelperdllapi.h>
#include <comphelper/processfactory.hxx>
#include <sal/types.h>
#include <memory>
#include <mutex>
#include <optional>
#include <string_view>
#include <unordered_map>
namespace com::sun::star {
namespace configuration { class XReadWriteAccess; }
@@ -31,6 +32,10 @@ namespace com::sun::star {
class XNameContainer;
}
namespace uno { class XComponentContext; }
namespace util {
class XChangesListener;
class XChangesNotifier;
}
}
namespace comphelper {
@@ -80,8 +85,11 @@ private:
namespace detail {
class ConfigurationChangesListener;
/// @internal
class COMPHELPER_DLLPUBLIC ConfigurationWrapper {
friend class ConfigurationChangesListener;
public:
static ConfigurationWrapper const & get();
@@ -135,6 +143,12 @@ private:
// css.beans.XHierarchicalPropertySetInfo), but then
// configmgr::Access::asProperty() would report all properties as
// READONLY, so isReadOnly() would not work
mutable std::mutex maMutex;
bool mbDisposed;
mutable std::unordered_map<OUString, css::uno::Any> maPropertyCache;
css::uno::Reference< css::util::XChangesNotifier > maNotifier;
css::uno::Reference< css::util::XChangesListener > maListener;
};
/// @internal