[Libreoffice-commits] core.git: Branch 'distro/vector/vector-7.0' - framework/CppunitTest_framework_services.mk framework/Module_framework.mk framework/qa framework/source

Miklos Vajna (via logerrit) logerrit at kemper.freedesktop.org
Fri Sep 25 07:50:51 UTC 2020


 framework/CppunitTest_framework_services.mk |   40 ++++++++
 framework/Module_framework.mk               |    7 +
 framework/qa/cppunit/services.cxx           |  133 ++++++++++++++++++++++++++++
 framework/source/services/frame.cxx         |    7 +
 4 files changed, 187 insertions(+)

New commits:
commit 377d84d2d515b57bc408446ef8b8a9dc174e6a17
Author:     Miklos Vajna <vmiklos at collabora.com>
AuthorDate: Thu Sep 24 13:58:44 2020 +0200
Commit:     Miklos Vajna <vmiklos at collabora.com>
CommitDate: Fri Sep 25 09:49:38 2020 +0200

    framework: lock the solar mutex in loadComponentFromURL() with OnMainThread
    
    Regression from commit 2dc3a6c273cb82506842864481d78df7294debbf
    (framework: allow loading a component on the main thread, 2018-12-19),
    which was a forward-port from a 5.4-based vendor branch, where this was
    (it turns out) just working by accident, but never on master.
    
    It can happen that loadComponentFromURL() is invoked on a thread, which
    does not own the solar mutex. Then once
    vcl::SolarThreadExecutor::execute() is called, it'll try to release the
    solar mutex. But SolarMutexReleaser is unsafe: it'll release the mutex
    even if it is owned by an other thread.
    
    To make this a bit more safer, it'll abort in
    comphelper::SolarMutex::doRelease(), in case the current thread doesn't
    have the mutex already.
    
    Fix the problem by taking the solar mutex in loadComponentFromURL():
    this is meant to cause no performance problems, since the actual
    importers typically start with taking the solar mutex anyway. Taking it
    earlier would be problematic, since this can be invoked by UNO clients
    directly. Taking it later in vcl/ would be also unusual: typically vcl
    just asserts that the solar mutex is locked, doesn't take it itself.
    
    (cherry picked from commit 07b399eb91fb7860190504ca0214bdf4d30dfe2a)
    
    Change-Id: I752006a91f16a02254d1b5ac6301100ab282630b

diff --git a/framework/CppunitTest_framework_services.mk b/framework/CppunitTest_framework_services.mk
new file mode 100644
index 000000000000..f098b7e0bcf9
--- /dev/null
+++ b/framework/CppunitTest_framework_services.mk
@@ -0,0 +1,40 @@
+# -*- Mode: makefile-gmake; tab-width: 4; indent-tabs-mode: t -*-
+#
+# This file is part of the LibreOffice project.
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+
+$(eval $(call gb_CppunitTest_CppunitTest,framework_services))
+
+$(eval $(call gb_CppunitTest_add_exception_objects,framework_services, \
+    framework/qa/cppunit/services \
+))
+
+$(eval $(call gb_CppunitTest_use_sdk_api,framework_services))
+
+$(eval $(call gb_CppunitTest_use_libraries,framework_services, \
+	comphelper \
+	cppu \
+	cppuhelper \
+	sal \
+	salhelper \
+	test \
+	unotest \
+	vcl \
+))
+
+$(eval $(call gb_CppunitTest_use_external,framework_services,boost_headers))
+
+$(eval $(call gb_CppunitTest_use_sdk_api,framework_services))
+
+$(eval $(call gb_CppunitTest_use_ure,framework_services))
+$(eval $(call gb_CppunitTest_use_vcl,framework_services))
+
+$(eval $(call gb_CppunitTest_use_rdb,framework_services,services))
+
+$(eval $(call gb_CppunitTest_use_configuration,framework_services))
+
+# vim: set noet sw=4 ts=4:
diff --git a/framework/Module_framework.mk b/framework/Module_framework.mk
index 8b136ac92dc7..217838519c28 100644
--- a/framework/Module_framework.mk
+++ b/framework/Module_framework.mk
@@ -33,6 +33,13 @@ $(eval $(call gb_Module_add_slowcheck_targets,framework,\
     CppunitTest_framework_dispatch \
 ))
 
+# Not sure why this is not stable on macOS.
+ifneq ($(OS),MACOSX)
+$(eval $(call gb_Module_add_slowcheck_targets,framework,\
+    CppunitTest_framework_services \
+))
+endif
+
 $(eval $(call gb_Module_add_l10n_targets,framework,\
     AllLangMoTarget_fwk \
 ))
diff --git a/framework/qa/cppunit/services.cxx b/framework/qa/cppunit/services.cxx
new file mode 100644
index 000000000000..93d05ff65ca5
--- /dev/null
+++ b/framework/qa/cppunit/services.cxx
@@ -0,0 +1,133 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <test/bootstrapfixture.hxx>
+#include <unotest/macros_test.hxx>
+
+#include <com/sun/star/frame/Desktop.hpp>
+#include <com/sun/star/frame/XFrame.hpp>
+#include <com/sun/star/frame/XComponentLoader.hpp>
+#include <com/sun/star/frame/FrameSearchFlag.hpp>
+
+#include <comphelper/propertyvalue.hxx>
+#include <salhelper/thread.hxx>
+#include <vcl/svapp.hxx>
+#include <vcl/scheduler.hxx>
+#include <vcl/wrkwin.hxx>
+
+using namespace ::com::sun::star;
+
+namespace
+{
+/// Covers framework/source/services/ fixes.
+class Test : public test::BootstrapFixture, public unotest::MacrosTest
+{
+protected:
+    uno::Reference<lang::XComponent> mxComponent;
+
+public:
+    void setUp() override;
+    void tearDown() override;
+    uno::Reference<lang::XComponent>& getComponent() { return mxComponent; }
+};
+
+void Test::setUp()
+{
+    test::BootstrapFixture::setUp();
+
+    mxDesktop.set(frame::Desktop::create(mxComponentContext));
+}
+
+void Test::tearDown()
+{
+    if (mxComponent.is())
+        mxComponent->dispose();
+
+    test::BootstrapFixture::tearDown();
+}
+
+/// Invokes XFrameImpl::loadComponentFromURL() on a thread.
+class TestThread : public salhelper::Thread
+{
+    uno::Reference<frame::XComponentLoader> mxComponentLoader;
+    uno::Reference<lang::XComponent>& mrComponent;
+
+public:
+    TestThread(const uno::Reference<frame::XComponentLoader>& xComponentLoader,
+               uno::Reference<lang::XComponent>& rComponent);
+    void execute() override;
+};
+
+TestThread::TestThread(const uno::Reference<frame::XComponentLoader>& xComponentLoader,
+                       uno::Reference<lang::XComponent>& rComponent)
+    : salhelper::Thread("TestThread")
+    , mxComponentLoader(xComponentLoader)
+    , mrComponent(rComponent)
+{
+}
+
+void TestThread::execute()
+{
+    sal_Int32 nSearchFlags = frame::FrameSearchFlag::AUTO;
+    uno::Sequence<beans::PropertyValue> aArguments = {
+        comphelper::makePropertyValue("OnMainThread", true),
+    };
+    // Note how this is invoking loadComponentFromURL() on a frame, not on the desktop, as usual.
+    mrComponent = mxComponentLoader->loadComponentFromURL("private:factory/swriter", "_self",
+                                                          nSearchFlags, aArguments);
+}
+
+CPPUNIT_TEST_FIXTURE(Test, testLoadComponentFromURL)
+{
+    // Without the accompanying fix in place, this test would have failed with:
+    // thread 1: comphelper::SolarMutex::doRelease end: m_nCount is 1
+    // thread 2: vcl::SolarThreadExecutor::execute: before SolarMutexReleaser ctor
+    // thread 2: comphelper::SolarMutex::doRelease start: m_nCount is 1, bUnlockAll is 1
+    // thread 2: comphelper::SolarMutex::doRelease: failed IsCurrentThread() check, will abort
+    // Notice how thread 2 attempts to release the solar mutex while thread 1 holds it.
+
+    // Create a default window, so by the time the thread would post a user event, it doesn't need
+    // the solar mutex to process a SendMessageW() call on Windows.
+    VclPtrInstance<WorkWindow> xWindow(nullptr, WB_APP | WB_STDWORK);
+    // Variable is not used, it holds the default window.
+    (void)xWindow;
+
+    rtl::Reference<TestThread> xThread;
+    {
+        // Start the thread that will load the component, but hold the solar mutex for now, so we
+        // can see if it blocks.
+        SolarMutexGuard guard;
+        uno::Reference<frame::XFrame> xFrame = mxDesktop->findFrame("_blank", /*nSearchFlags=*/0);
+        uno::Reference<frame::XComponentLoader> xComponentLoader(xFrame, uno::UNO_QUERY);
+        xThread = new TestThread(xComponentLoader, getComponent());
+        xThread->launch();
+        // If loadComponentFromURL() doesn't lock the solar mutex, the test will abort here.
+        osl::Thread::wait(std::chrono::seconds(1));
+    }
+    {
+        // Now release the solar mutex, so the thread can post the task on the main loop.
+        SolarMutexReleaser releaser;
+        osl::Thread::wait(std::chrono::seconds(1));
+    }
+    {
+        // Spin the main loop.
+        SolarMutexGuard guard;
+        Scheduler::ProcessEventsToIdle();
+    }
+    {
+        // Stop the thread.
+        SolarMutexReleaser releaser;
+        xThread->join();
+    }
+}
+}
+
+CPPUNIT_PLUGIN_IMPLEMENT();
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/framework/source/services/frame.cxx b/framework/source/services/frame.cxx
index 11881c3ebefb..481a425f1ea8 100644
--- a/framework/source/services/frame.cxx
+++ b/framework/source/services/frame.cxx
@@ -583,9 +583,16 @@ css::uno::Reference< css::lang::XComponent > SAL_CALL XFrameImpl::loadComponentF
     bool bOnMainThread = aDescriptor.getUnpackedValueOrDefault("OnMainThread", false);
 
     if (bOnMainThread)
+    {
+        // Make sure that we own the solar mutex, otherwise later
+        // vcl::SolarThreadExecutor::execute() will release the solar mutex, even if it's owned by
+        // an other thread, leading to an std::abort() at the end.
+        SolarMutexGuard g;
+
         return vcl::solarthread::syncExecute(std::bind(&LoadEnv::loadComponentFromURL, xThis,
                                                        m_xContext, sURL, sTargetFrameName,
                                                        nSearchFlags, lArguments));
+    }
     else
         return LoadEnv::loadComponentFromURL(xThis, m_xContext, sURL, sTargetFrameName,
                                              nSearchFlags, lArguments);


More information about the Libreoffice-commits mailing list