configmgr/source/components.cxx | 1 configmgr/source/components.hxx | 2 + configmgr/source/writemodfile.cxx | 57 +++++++++++++++++++++----------------- 3 files changed, 34 insertions(+), 26 deletions(-)
New commits: commit a0ffaeeff972710a4172e7b4ee72610ad858c4df Author: Stephan Bergmann <[email protected]> AuthorDate: Mon Feb 10 14:38:33 2025 +0100 Commit: Stephan Bergmann <[email protected]> CommitDate: Mon Feb 10 17:38:57 2025 +0100 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 <[email protected]> diff --git a/configmgr/source/components.cxx b/configmgr/source/components.cxx index 86f2d0928dc6..d40d04fbdb03 100644 --- a/configmgr/source/components.cxx +++ b/configmgr/source/components.cxx @@ -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_); diff --git a/configmgr/source/components.hxx b/configmgr/source/components.hxx index 8f5bd24c8925..c83b56f5a66e 100644 --- a/configmgr/source/components.hxx +++ b/configmgr/source/components.hxx @@ -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; diff --git a/configmgr/source/writemodfile.cxx b/configmgr/source/writemodfile.cxx index 51a738d8d31c..10b8a75d4849 100644 --- a/configmgr/source/writemodfile.cxx +++ b/configmgr/source/writemodfile.cxx @@ -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 + { + osl::MutexGuard g(components.getLock()); - // 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 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 - for (const auto& rCand : data.modifications.getRoot().children) - { - ModNodePairEntryVector.push_back(&rCand); - } + // 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()); - // sort the list - std::sort(ModNodePairEntryVector.begin(), ModNodePairEntryVector.end(), PairEntrySorter()); + for (const auto& rCand : data.modifications.getRoot().children) + { + ModNodePairEntryVector.push_back(&rCand); + } - // 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> "); tmp.closeAndRename(url); }
