Shrink critical section into writeModFile
...from the calling configmgr::Components::WriteThread::execute. This is relevant at least for an upcoming Emscripten Qt6 JSPI/non-PROXY_TO_PTHREAD mode, where part of the LO event handling (which would normally take place on the main thread) will be offloaded to an additional thread. There, opening e.g. a fresh Writer document shortly after starting LO could have lead to a deadlock: The configmgrWriter thread had the configmgr::Components mutex locked and tried to do a mkdir, which it needs to proxy to the main thread on Emscripten, > $emscripten_futex_wait (soffice.wasm:0x83dbf64) > $__timedwait_cp (soffice.wasm:0x83e7f34) > $__pthread_cond_timedwait (soffice.wasm:0x83e8168) > $pthread_cond_wait (soffice.wasm:0x83e85a2) > $emscripten_proxy_sync_with_ctx (soffice.wasm:0x83e6709) > $emscripten_proxy_sync (soffice.wasm:0x83e6a08) > $_emscripten_run_on_main_thread_js (soffice.wasm:0x83e6d77) > ret.<computed> (soffice.js:8709) > (anonymous) (soffice.js:1285) > proxyToMainThread (soffice.js:1617) > ___syscall_mkdirat (soffice.js:6613) > $mkdir (soffice.wasm:0x83e3e0a) > $osl::mkdir(rtl::OString const&, unsigned int) (soffice.wasm:0xbce7b3) > $create_dir_with_callback(char*, void (*)(void*, _rtl_uString*), void*) (soffice.wasm:0xbdc4d6) > $create_dir_recursively_(char*, void (*)(void*, _rtl_uString*), void*) (soffice.wasm:0xbdc422) > $osl_createDirectoryPath (soffice.wasm:0xbdc35f) > $configmgr::writeModFile(configmgr::Components&, rtl::OUString const&, configmgr::Data const&) (soffice.wasm:0x4218d80) > $configmgr::Components::WriteThread::execute() (soffice.wasm:0x41f561d) > $non-virtual thunk to salhelper::Thread::run() (soffice.wasm:0xd369fd) > $threadFunc (soffice.wasm:0xd36890) > $osl_thread_start_Impl(void*) (soffice.wasm:0xbca617) ...while the main thread was serving a proxied call from the additional "event handling offload" thread, during which it accesses configmgr code and tried to lock the configmgr::Components mutex, > $a_cas_p (soffice.wasm:0x83dc0d9) > $futex_wait_main_browser_thread (soffice.wasm:0x83dbfed) > $emscripten_futex_wait (soffice.wasm:0x83dbf23) > $__timedwait_cp (soffice.wasm:0x83e7f34) > $__timedwait (soffice.wasm:0x83e7fd2) > $__pthread_mutex_timedlock (soffice.wasm:0x83e9323) > $__pthread_mutex_lock (soffice.wasm:0x83e9208) > $osl_acquireMutex (soffice.wasm:0xbbc750) > $configmgr::Access::thisIs(int) (soffice.wasm:0x41dc351) > $configmgr::Access::getByName(rtl::OUString const&) (soffice.wasm:0x41e13b3) > $non-virtual thunk to configmgr::Access::getByName(rtl::OUString const&) (soffice.wasm:0x41e159a) > $comphelper::detail::ConfigurationWrapper::getPropertyValue(std::__2::basic_string_view<char16_t, std::__2::char_traits<char16_t>>) const (soffice.wasm:0xc2f8fe) > $comphelper::ConfigurationProperty<officecfg::Office::Common::Misc::ViewerAppMode, bool>::get(com::sun::star::uno::Reference<com::sun::star::uno::XComponentContext> const&) (soffice.wasm:0xdfbc05) > $SfxDispatcher::FindServer_(unsigned short, SfxSlotServer&) (soffice.wasm:0xdfb6d4) > $SfxDispatcher::GetShellAndSlot_Impl(unsigned short, SfxShell**, SfxSlot const**, bool, bool) (soffice.wasm:0xdfb398) > $SfxDispatcher::QueryState(unsigned short, com::sun::star::uno::Any&) (soffice.wasm:0xe01fe3) > $SfxDispatchController_Impl::addStatusListener(com::sun::star::uno::Reference<com::sun::star::frame::XStatusListener> const&, com::sun::star::util::URL const&) (soffice.wasm:0xe1d5bd) > $SfxOfficeDispatch::addStatusListener(com::sun::star::uno::Reference<com::sun::star::frame::XStatusListener> const&, com::sun::star::util::URL const&) (soffice.wasm:0xe1d4ce) > $non-virtual thunk to SfxOfficeDispatch::addStatusListener(com::sun::star::uno::Reference<com::sun::star::frame::XStatusListener> const&, com::sun::star::util::URL const&) (soffice.wasm:0xe1daa6) > $svt::PopupMenuControllerBase::updateCommand(rtl::OUString const&) (soffice.wasm:0x20cd630) > $svt::PopupMenuControllerBase::updatePopupMenu() (soffice.wasm:0x20cd460) > $framework::HeaderMenuController::updatePopupMenu() (soffice.wasm:0x29a0323) > $svt::PopupMenuControllerBase::setPopupMenu(com::sun::star::uno::Reference<com::sun::star::awt::XPopupMenu> const&) (soffice.wasm:0x20cf1bb) > $non-virtual thunk to svt::PopupMenuControllerBase::setPopupMenu(com::sun::star::uno::Reference<com::sun::star::awt::XPopupMenu> const&) (soffice.wasm:0x20cf1e0) > $framework::MenuBarManager::CreatePopupMenuController(framework::MenuBarManager::MenuItemHandler*, com::sun::star::uno::Reference<com::sun::star::frame::XDispatchProvider> const&, rtl::OUString const&) (soffice.wasm:0x217c209) > $framework::MenuBarManager::Activate(Menu*) (soffice.wasm:0x218130b) > $framework::MenuBarManager::Activate(Menu*) (soffice.wasm:0x2180b17) > $framework::MenuBarManager::LinkStubActivate(void*, Menu*) (soffice.wasm:0x2180126) > $Menu::Activate() (soffice.wasm:0x14d68e0) > $Menu::HandleMenuActivateEvent(Menu*) const (soffice.wasm:0x14df42c) > $QtMenu::DoFullMenuUpdate(Menu*) (soffice.wasm:0x6617932) > $QtMenu::SetFrame(SalFrame const*) (soffice.wasm:0x661738a) > $std::__2::__function::__func<QtMenu::SetFrame(SalFrame const*)::$_0, std::__2::allocator<QtMenu::SetFrame(SalFrame const*)::$_0>, void ()>::operator()() (soffice.wasm:0x661c275) > $QtInstance::EmscriptenProxyToMainThread(std::__2::function<void ()>)::$_0::__invoke(void*) (soffice.wasm:0x65b24e4) Change-Id: Iee58b86a96147273b7ae288524513e0d174287a8 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/181353 Tested-by: Jenkins Reviewed-by: Stephan Bergmann <stephan.bergmann@allotropia.de>
This commit is contained in:
Stephan Bergmann
@@ -204,7 +204,6 @@ void Components::WriteThread::execute() {
|
||||
}
|
||||
delayOrTerminate_.wait(std::chrono::seconds(1));
|
||||
// must not throw; result_error is harmless and ignored
|
||||
osl::MutexGuard g(*lock_); // must not throw
|
||||
try {
|
||||
try {
|
||||
writeModFile(components_, url_, data_);
|
||||
|
||||
@@ -98,6 +98,8 @@ public:
|
||||
css::beans::Optional< css::uno::Any >
|
||||
getExternalValue(std::u16string_view descriptor);
|
||||
|
||||
osl::Mutex & getLock() const { return *lock_; }
|
||||
|
||||
private:
|
||||
Components(const Components&) = delete;
|
||||
Components& operator=(const Components&) = delete;
|
||||
|
||||
@@ -30,6 +30,7 @@
|
||||
#include <o3tl/safeint.hxx>
|
||||
#include <osl/file.h>
|
||||
#include <osl/file.hxx>
|
||||
#include <osl/mutex.hxx>
|
||||
#include <rtl/string.h>
|
||||
#include <rtl/string.hxx>
|
||||
#include <rtl/textcvt.h>
|
||||
@@ -40,6 +41,7 @@
|
||||
#include <sal/types.h>
|
||||
#include <xmlreader/span.hxx>
|
||||
|
||||
#include "components.hxx"
|
||||
#include "data.hxx"
|
||||
#include "groupnode.hxx"
|
||||
#include "localizedpropertynode.hxx"
|
||||
@@ -601,36 +603,41 @@ void writeModFile(
|
||||
// come from the .xcs/.xcu files, anyway (but had been added dynamically
|
||||
// instead):
|
||||
|
||||
// For profilesafemode it is necessary to detect changes in the
|
||||
// registrymodifications file, this is done based on file size in bytes and crc32.
|
||||
// Unfortunately this write is based on writing unordered map entries, which creates
|
||||
// valid and semantically equal XML-Files, bubt with different crc32 checksums. For
|
||||
// the future usage it will be preferable to have easily comparable config files
|
||||
// which is guaranteed by writing the entries in sorted order. Indeed with this change
|
||||
// (and in the recursive writeModifications call) the same config files get written
|
||||
|
||||
// copy configmgr::Modifications::Node's to a sortable list. Use pointers
|
||||
// to just reference the data instead of copying it
|
||||
std::vector< const ModNodePairEntry* > ModNodePairEntryVector;
|
||||
ModNodePairEntryVector.reserve(data.modifications.getRoot().children.size());
|
||||
|
||||
for (const auto& rCand : data.modifications.getRoot().children)
|
||||
{
|
||||
ModNodePairEntryVector.push_back(&rCand);
|
||||
osl::MutexGuard g(components.getLock());
|
||||
|
||||
// For profilesafemode it is necessary to detect changes in the
|
||||
// registrymodifications file, this is done based on file size in bytes and crc32.
|
||||
// Unfortunately this write is based on writing unordered map entries, which creates
|
||||
// valid and semantically equal XML-Files, bubt with different crc32 checksums. For
|
||||
// the future usage it will be preferable to have easily comparable config files
|
||||
// which is guaranteed by writing the entries in sorted order. Indeed with this change
|
||||
// (and in the recursive writeModifications call) the same config files get written
|
||||
|
||||
// copy configmgr::Modifications::Node's to a sortable list. Use pointers
|
||||
// to just reference the data instead of copying it
|
||||
std::vector< const ModNodePairEntry* > ModNodePairEntryVector;
|
||||
ModNodePairEntryVector.reserve(data.modifications.getRoot().children.size());
|
||||
|
||||
for (const auto& rCand : data.modifications.getRoot().children)
|
||||
{
|
||||
ModNodePairEntryVector.push_back(&rCand);
|
||||
}
|
||||
|
||||
// sort the list
|
||||
std::sort(ModNodePairEntryVector.begin(), ModNodePairEntryVector.end(), PairEntrySorter());
|
||||
|
||||
// now use the list to write entries in sorted order
|
||||
// instead of random as from the unordered map
|
||||
for (const auto& j : ModNodePairEntryVector)
|
||||
{
|
||||
writeModifications(
|
||||
components, tmp, u"", rtl::Reference< Node >(), j->first,
|
||||
data.getComponents().findNode(Data::NO_LAYER, j->first),
|
||||
j->second);
|
||||
}
|
||||
}
|
||||
|
||||
// sort the list
|
||||
std::sort(ModNodePairEntryVector.begin(), ModNodePairEntryVector.end(), PairEntrySorter());
|
||||
|
||||
// now use the list to write entries in sorted order
|
||||
// instead of random as from the unordered map
|
||||
for (const auto& j : ModNodePairEntryVector)
|
||||
{
|
||||
writeModifications(
|
||||
components, tmp, u"", rtl::Reference< Node >(), j->first,
|
||||
data.getComponents().findNode(Data::NO_LAYER, j->first),
|
||||
j->second);
|
||||
}
|
||||
tmp.writeString("</oor:items>\n");
|
||||
tmp.closeAndRename(url);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user