[Libreoffice-commits] core.git: Branch 'libreoffice-5-3' - framework/source

[Michael Stahl]

 framework/source/layoutmanager/toolbarlayoutmanager.cxx |   11 +++++++++++
 framework/source/uielement/toolbarmanager.cxx           |    2 +-
 2 files changed, 12 insertions(+), 1 deletion(-)

New commits:
commit 962e70501e4351761a9e521faed9506d865bcc63
Author: Michael Stahl <mstahl at redhat.com>
Date:   Fri Dec 9 23:42:14 2016 +0100

    framework: fix race in ToolBarManager creation
    
    ToolbarLayoutManager::createToolbar() may be called concurrently on
    different threads, and then it can happen that both threads want to
    create the same toolbar URL, see that it does not exist in line 457,
    then both release the SolarMutex and create a new ToolBarManager
    and the first inserts it and then the second overwrites it on line 514
    without disposing the first one.
    
    The non-disposed extra ToolBarManager is kept alive because it is
    registered as a listener on the Frame.  When the Frame::close() is
    called, the ToolbarLayoutManager is disposed, and that disposes all the
    ToolBarManagers it knows about, but not the extra one, which is
    then un-ref'd and then has a live VclPtr m_pToolBar, which asserts
    because the SolarMutex is not locked since commit
    e794ce1eef6730e5a46d5fb0aa6db2895ede85e7.
    
    (This commit is thanks to rr, which recorded the
    JunitTest_framework_complex execution and allowed debugging this.)
    
    (cherry picked from commit 84f2ff67a7e404febf710b1dc7f66d06745c503f)
    
    tdf#104621 framework: Redo commit 84f2ff67a7e404febf710b1dc7f66d06745c503f
    
    The fix was silly and wrong, need to check m_xUIElement, not m_aName,
    which may be set independently, see the confusing code in
    ToolbarLayoutManager::requestToolbar().
    
    Change-Id: I279088cb2516b0a19619b5647f15f738a2624edf
    (cherry picked from commit d266cb32c3c982a60cd68650dd7ae8983744134e)
    Reviewed-on: https://gerrit.libreoffice.org/32186
    Reviewed-by: Michael Stahl <mstahl at redhat.com>
    Tested-by: Michael Stahl <mstahl at redhat.com>

diff --git a/framework/source/layoutmanager/toolbarlayoutmanager.cxx b/framework/source/layoutmanager/toolbarlayoutmanager.cxx
index b770fd8..2e28756 100644
--- a/framework/source/layoutmanager/toolbarlayoutmanager.cxx
+++ b/framework/source/layoutmanager/toolbarlayoutmanager.cxx
@@ -506,6 +506,17 @@ bool ToolbarLayoutManager::createToolbar( const OUString& rResourceURL )
             SolarMutexClearableGuard aWriteLock;
 
             UIElement& rElement = impl_findToolbar( rResourceURL );
+            if (rElement.m_xUIElement.is())
+            {
+                // somebody else must have created it while we released
+                // the SolarMutex - just dispose our new instance and
+                // do nothing. (We have to dispose either the new or the
+                // existing m_xUIElement.)
+                aWriteLock.clear();
+                uno::Reference<lang::XComponent> const xC(xUIElement, uno::UNO_QUERY);
+                xC->dispose();
+                return false;
+            }
             if ( !rElement.m_aName.isEmpty() )
             {
                 // Reuse a local entry so we are able to use the latest
diff --git a/framework/source/uielement/toolbarmanager.cxx b/framework/source/uielement/toolbarmanager.cxx
index 4c330b3..489ad87 100644
--- a/framework/source/uielement/toolbarmanager.cxx
+++ b/framework/source/uielement/toolbarmanager.cxx
@@ -200,7 +200,7 @@ ToolBarManager::ToolBarManager( const Reference< XComponentContext >& rxContext,
 ToolBarManager::~ToolBarManager()
 {
     assert(!m_aAsyncUpdateControllersTimer.IsActive());
-    OSL_ASSERT( !m_pToolBar );
+    assert(!m_pToolBar); // must be disposed by ToolbarLayoutManager
     OSL_ASSERT( !m_bAddedToTaskPaneList );
 }