[Libreoffice-commits] core.git: ucb/source

Stephan Bergmann (via logerrit) logerrit at kemper.freedesktop.org
Thu May 9 11:15:19 UTC 2019


 ucb/source/ucp/gio/gio_content.cxx |   13 +++++++++++--
 ucb/source/ucp/gio/gio_mount.cxx   |   26 +++++++++++++++++++++++++-
 ucb/source/ucp/gio/gio_mount.hxx   |   30 +++++++++++++++++++++++++++++-
 3 files changed, 65 insertions(+), 4 deletions(-)

New commits:
commit 8a443fe0f4ab50e2156e2c7e0cf713f2949e3164
Author:     Stephan Bergmann <sbergman at redhat.com>
AuthorDate: Thu May 9 09:21:24 2019 +0200
Commit:     Stephan Bergmann <sbergman at redhat.com>
CommitDate: Thu May 9 13:14:00 2019 +0200

    tdf#124962: Reduce risk of g_main_loop_run from within gio MountOperation
    
    Using g_main_loop_run here appears to be inherently necessary for the
    g_file_mount_enclosing_volume/g_file_mount_enclosing_volume_finish protocol, but
    has at least two problems:  For one, it temporarily drops the SolarMutex (if it
    was held by the current thread), causing the usual integrity issues caused by an
    inner stack frame temporarily releasing the SolarMutex that is held by some
    unsuspecting caller.  This is an inherent problem of our broken SolarMutex
    design, and this change can't do much about it.
    
    But for another, at least with GTK-based VCL backends, it also means that the
    current thread can start to execute VCL events at "unexpected" times from within
    this g_main_loop_run (e.g., paint events, as in the backtraces linked from
    tdf#124962).  While handling of VCL events is necessary when a callback to
    ooo_mount_operation_ask_password happens and it actually pops up a dialog
    prompting the user for credentials, such handling of VCL events is completely
    unwanted when no such dialog is popped up (e.g., when the given server is
    unreachable anyway, as is the case in tdf#124962).
    
    So, to shrink the problematic window of time in which VCL events may get handled
    from within the gio MountOperation, use a dedicated GMainContext for the gio
    GMainLoop (so that it only handles events related to the mount operation), and
    only temporarily put back in place the original GMainContext during
    ooo_mount_operation_ask_password (so that VCL events will get handled as
    necessary when a dialog is actually popped up).
    
    Change-Id: Ie410f23778045b1adf98579bb34ce38d0f8f3320
    Reviewed-on: https://gerrit.libreoffice.org/72026
    Tested-by: Jenkins
    Reviewed-by: Stephan Bergmann <sbergman at redhat.com>

diff --git a/ucb/source/ucp/gio/gio_content.cxx b/ucb/source/ucp/gio/gio_content.cxx
index 6127412d58fc..841a19980824 100644
--- a/ucb/source/ucp/gio/gio_content.cxx
+++ b/ucb/source/ucp/gio/gio_content.cxx
@@ -17,6 +17,10 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include <sal/config.h>
+
+#include <utility>
+
 #include <string.h>
 #include <unistd.h>
 #include <sys/types.h>
@@ -315,6 +319,7 @@ css::uno::Any Content::getBadArgExcept()
 
 class MountOperation
 {
+    ucb::ucp::gio::glib::MainContextRef mContext;
     GMainLoop *mpLoop;
     GMountOperation *mpAuthentication;
     GError *mpError;
@@ -327,8 +332,11 @@ public:
 
 MountOperation::MountOperation(const css::uno::Reference< css::ucb::XCommandEnvironment >& xEnv) : mpError(nullptr)
 {
-    mpLoop = g_main_loop_new(nullptr, FALSE);
-    mpAuthentication = ooo_mount_operation_new(xEnv);
+    ucb::ucp::gio::glib::MainContextRef oldContext(g_main_context_ref_thread_default());
+    mContext.reset(g_main_context_new());
+    mpLoop = g_main_loop_new(mContext.get(), FALSE);
+    g_main_context_push_thread_default(mContext.get());
+    mpAuthentication = ooo_mount_operation_new(std::move(oldContext), xEnv);
 }
 
 void MountOperation::Completed(GObject *source, GAsyncResult *res, gpointer user_data)
@@ -363,6 +371,7 @@ GError *MountOperation::Mount(GFile *pFile)
 MountOperation::~MountOperation()
 {
     g_object_unref(mpAuthentication);
+    g_main_context_pop_thread_default(mContext.get());
     g_main_loop_unref(mpLoop);
 }
 
diff --git a/ucb/source/ucp/gio/gio_mount.cxx b/ucb/source/ucp/gio/gio_mount.cxx
index d791fb3579be..be22b6d9bd3f 100644
--- a/ucb/source/ucp/gio/gio_mount.cxx
+++ b/ucb/source/ucp/gio/gio_mount.cxx
@@ -17,6 +17,10 @@
  *   the License at http://www.apache.org/licenses/LICENSE-2.0 .
  */
 
+#include <sal/config.h>
+
+#include <utility>
+
 #include "gio_mount.hxx"
 #include <ucbhelper/simpleauthenticationrequest.hxx>
 #include <string.h>
@@ -47,6 +51,7 @@ static void ooo_mount_operation_finalize (GObject *object)
         free(mount_op->m_pPrevUsername);
     if (mount_op->m_pPrevPassword)
         free(mount_op->m_pPrevPassword);
+    mount_op->context.reset();
 
     G_OBJECT_CLASS (ooo_mount_operation_parent_class)->finalize (object);
 }
@@ -60,6 +65,23 @@ static void ooo_mount_operation_class_init (OOoMountOperationClass *klass)
     mount_op_class->ask_password = ooo_mount_operation_ask_password;
 }
 
+namespace {
+
+// Temporarily undo the g_main_context_push_thread_default done in the surrounding MountOperation
+// ctor (in ucb/source/ucp/gio/gio_content.cxx):
+struct GlibThreadDefaultMainContextScope {
+public:
+    GlibThreadDefaultMainContextScope(GMainContext * context): context_(context)
+    { g_main_context_push_thread_default(context_); }
+
+    ~GlibThreadDefaultMainContextScope() { g_main_context_pop_thread_default(context_); }
+
+private:
+    GMainContext * context_;
+};
+
+}
+
 static void ooo_mount_operation_ask_password (GMountOperation *op,
     const char * /*message*/, const char *default_user,
     const char *default_domain, GAskPasswordFlags flags)
@@ -67,6 +89,7 @@ static void ooo_mount_operation_ask_password (GMountOperation *op,
     css::uno::Reference< css::task::XInteractionHandler > xIH;
 
     OOoMountOperation *pThis = reinterpret_cast<OOoMountOperation*>(op);
+    GlibThreadDefaultMainContextScope scope(pThis->context.get());
 
     const css::uno::Reference< css::ucb::XCommandEnvironment > &xEnv = *(pThis->pEnv);
 
@@ -171,9 +194,10 @@ static void ooo_mount_operation_ask_password (GMountOperation *op,
     g_mount_operation_reply (op, G_MOUNT_OPERATION_HANDLED);
 }
 
-GMountOperation *ooo_mount_operation_new(const css::uno::Reference< css::ucb::XCommandEnvironment >& rEnv)
+GMountOperation *ooo_mount_operation_new(ucb::ucp::gio::glib::MainContextRef && context, const css::uno::Reference< css::ucb::XCommandEnvironment >& rEnv)
 {
     OOoMountOperation *pRet = static_cast<OOoMountOperation*>(g_object_new (OOO_TYPE_MOUNT_OPERATION, nullptr));
+    pRet->context = std::move(context);
     pRet->pEnv = &rEnv;
     return &pRet->parent_instance;
 }
diff --git a/ucb/source/ucp/gio/gio_mount.hxx b/ucb/source/ucp/gio/gio_mount.hxx
index 3b6c057499d6..421f7b7d3fd5 100644
--- a/ucb/source/ucp/gio/gio_mount.hxx
+++ b/ucb/source/ucp/gio/gio_mount.hxx
@@ -20,6 +20,10 @@
 #ifndef INCLUDED_UCB_SOURCE_UCP_GIO_GIO_MOUNT_HXX
 #define INCLUDED_UCB_SOURCE_UCP_GIO_GIO_MOUNT_HXX
 
+#include <sal/config.h>
+
+#include <memory>
+
 #include <com/sun/star/ucb/XCommandEnvironment.hpp>
 #include <gio/gio.h>
 
@@ -32,13 +36,37 @@ G_BEGIN_DECLS
 #define OOO_IS_MOUNT_OPERATION_CLASS(k)  (G_TYPE_CHECK_CLASS_TYPE ((k), OOO_TYPE_MOUNT_OPERATION))
 #define OOO_MOUNT_OPERATION_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), OOO_TYPE_MOUNT_OPERATION, OOoMountOperationClass))
 
+namespace ucb::ucp::gio::glib {
+
+namespace detail {
+
+struct MainContextUnref {
+    void operator ()(GMainContext * context) {
+        if (context != nullptr) {
+            g_main_context_unref(context);
+        }
+    }
+};
+
+}
+
+using MainContextRef = std::unique_ptr<GMainContext, detail::MainContextUnref>;
+
+}
+
 struct OOoMountOperation
 {
     GMountOperation parent_instance;
 
+    ucb::ucp::gio::glib::MainContextRef context;
     const css::uno::Reference< css::ucb::XCommandEnvironment > *pEnv;
     char *m_pPrevUsername;
     char *m_pPrevPassword;
+
+private:
+    // Managed via ooo_mount_operation_new and ooo_mount_operation_finalize:
+    OOoMountOperation() = delete;
+    ~OOoMountOperation() = delete;
 };
 
 struct OOoMountOperationClass
@@ -54,7 +82,7 @@ struct OOoMountOperationClass
 
 
 GType            ooo_mount_operation_get_type();
-GMountOperation *ooo_mount_operation_new(const css::uno::Reference< css::ucb::XCommandEnvironment >& rEnv);
+GMountOperation *ooo_mount_operation_new(ucb::ucp::gio::glib::MainContextRef && context, const css::uno::Reference< css::ucb::XCommandEnvironment >& rEnv);
 
 G_END_DECLS
 #endif


More information about the Libreoffice-commits mailing list