tdf#137356 framework: fix opening the same document twice for long loads

If the document loading is long enough that the statusbar is updated,
then we can have this situation that we start loading the document, then
spin the main loop during load and do a second load of the same document
as part of the main loop spinning:

	#0  SwDoc::SwDoc() (this=0x1c6a180) at sw/source/core/doc/docnew.cxx:194
	...
	#6  0x00007ffff359a6dd in SfxBaseModel::load(com::sun::star::uno::Sequence<com::sun::star::beans::PropertyValue> const&) (this=0x1c5c260, seqArguments=uno::Sequence of length 15 = {...})
	...
	#33 0x00007fffeeb81ecd in Application::Reschedule(bool) (i_bAllEvents=true) at vcl/source/app/svapp.cxx:460
	...
	#36 0x00007ffff4265251 in framework::StatusIndicator::start(rtl::OUString const&, int) (this=0x1aace80, sText="Loading document...", nRange=1000000) at framework/source/helper/statusindicator.cxx:51
	#37 0x00007fffd026dfd3 in XMLReader::Read(SwDoc&, rtl::OUString const&, SwPaM&, rtl::OUString const&) (this=0x1bb7d20, rDoc=..., rBaseURL="file:///.../test.odt", rPaM=SwPaM = {...}, rName="") at sw/source/filter/xml/swxml.cxx:630
	...
	#42 0x00007ffff359a6dd in SfxBaseModel::load(com::sun::star::uno::Sequence<com::sun::star::beans::PropertyValue> const&) (this=0x1bce4d0, seqArguments=uno::Sequence of length 15 = {...}) at sfx2/source/doc/sfxbasemodel.cxx:1883

The reason for this is is that by the time
LoadEnv::impl_searchAlreadyLoaded() searches for frames which already
have this doc open, the first load is still in progress, and we
assiciate the frame with its controller (which has the URL) only once
the load finishes.

Fix the problem by setting the URL on the frame directly for the
duration of the load: this way an in-progress load also counts as a
duplicate and we'll have just one document open at the end.

Regression from commit 74ac65c49cc1d53b1aa93c2b7c720255867aace2
(#i114963# Enable IPC before OpenClients to allow client connections
when printing., 2016-09-06), we just didn't process incoming requests on
the socket before, so the problem was less visible.

Change-Id: Ib138c4c264e2508c20104ab268501bcca31e2790
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/104310
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
Tested-by: Jenkins

7 files changed, 195 insertions, 16 deletions

diff --git a/framework/CppunitTest_framework_loadenv.mk b/framework/CppunitTest_framework_loadenv.mk
new file mode 100644
index 000000000000..b1e01e474e66
--- /dev/null
+++ b/framework/CppunitTest_framework_loadenv.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_loadenv))
+
+$(eval $(call gb_CppunitTest_add_exception_objects,framework_loadenv, \
+    framework/qa/cppunit/loadenv \
+))
+
+$(eval $(call gb_CppunitTest_use_sdk_api,framework_loadenv))
+
+$(eval $(call gb_CppunitTest_use_libraries,framework_loadenv, \
+	comphelper \
+	cppu \
+	cppuhelper \
+	sal \
+	salhelper \
+	test \
+	unotest \
+	vcl \
+))
+
+$(eval $(call gb_CppunitTest_use_external,framework_loadenv,boost_headers))
+
+$(eval $(call gb_CppunitTest_use_sdk_api,framework_loadenv))
+
+$(eval $(call gb_CppunitTest_use_ure,framework_loadenv))
+$(eval $(call gb_CppunitTest_use_vcl,framework_loadenv))
+
+$(eval $(call gb_CppunitTest_use_rdb,framework_loadenv,services))
+
+$(eval $(call gb_CppunitTest_use_configuration,framework_loadenv))
+
+# vim: set noet sw=4 ts=4:
diff --git a/framework/Module_framework.mk b/framework/Module_framework.mk
index 2d59a8183456..924989d0672a 100644
--- a/framework/Module_framework.mk
+++ b/framework/Module_framework.mk
@@ -27,6 +27,7 @@ $(eval $(call gb_Module_add_targets,framework,\
 
 $(eval $(call gb_Module_add_slowcheck_targets,framework,\
     CppunitTest_framework_dispatch \
+    CppunitTest_framework_loadenv \
 ))
 
 # Not sure why this is not stable on macOS.
diff --git a/framework/inc/properties.h b/framework/inc/properties.h
index 5977b7b760bb..b5bcd412f5c6 100644
--- a/framework/inc/properties.h
+++ b/framework/inc/properties.h
@@ -31,6 +31,7 @@ namespace framework{
 #define FRAME_PROPNAME_ASCII_LAYOUTMANAGER              "LayoutManager"
 #define FRAME_PROPNAME_ASCII_TITLE                      "Title"
 #define FRAME_PROPNAME_ASCII_INDICATORINTERCEPTION      "IndicatorInterception"
+#define FRAME_PROPNAME_ASCII_URL "URL"
 
 // Please add new entries alphabetical sorted and correct all other handles!
 // Start counting with 0, so it can be used as direct index into an array too.
@@ -40,8 +41,9 @@ namespace framework{
 #define FRAME_PROPHANDLE_LAYOUTMANAGER                  2
 #define FRAME_PROPHANDLE_TITLE                          3
 #define FRAME_PROPHANDLE_INDICATORINTERCEPTION          4
+#define FRAME_PROPHANDLE_URL 5
 
-#define FRAME_PROPCOUNT                                 5
+#define FRAME_PROPCOUNT 6
 
 /** properties for "PathSettings" class */
 
diff --git a/framework/qa/cppunit/data/double-loading.odt b/framework/qa/cppunit/data/double-loading.odt
new file mode 100644
index 000000000000..ce990fc5c1de
--- /dev/null
+++ b/framework/qa/cppunit/data/double-loading.odt
diff --git a/framework/qa/cppunit/loadenv.cxx b/framework/qa/cppunit/loadenv.cxx
new file mode 100644
index 000000000000..96d28cd47396
--- /dev/null
+++ b/framework/qa/cppunit/loadenv.cxx
@@ -0,0 +1,86 @@
+/* -*- 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 <comphelper/processfactory.hxx>
+#include <vcl/scheduler.hxx>
+#include <vcl/svapp.hxx>
+
+using namespace ::com::sun::star;
+
+namespace
+{
+/// Covers framework/source/loadenv/ fixes.
+class Test : public test::BootstrapFixture, public unotest::MacrosTest
+{
+public:
+    void setUp() override;
+};
+
+void Test::setUp()
+{
+    test::BootstrapFixture::setUp();
+
+    mxDesktop.set(frame::Desktop::create(mxComponentContext));
+}
+
+char const DATA_DIRECTORY[] = "/framework/qa/cppunit/data/";
+
+class DocumentOpener
+{
+public:
+    DECL_STATIC_LINK(DocumentOpener, OpenDocument, void*, void);
+};
+
+IMPL_STATIC_LINK(DocumentOpener, OpenDocument, void*, pArg, void)
+{
+    CPPUNIT_ASSERT(pArg);
+    auto pURL = static_cast<OUString*>(pArg);
+    uno::Reference<uno::XComponentContext> xComponentContext
+        = comphelper::getProcessComponentContext();
+    uno::Reference<frame::XDesktop2> xDesktop = frame::Desktop::create(xComponentContext);
+    xDesktop->loadComponentFromURL(*pURL, "_default", 0, {});
+    delete pURL;
+}
+
+CPPUNIT_TEST_FIXTURE(Test, testDoubleLoading)
+{
+    // Try to load the same document twice. This is similar to trying to execute the soffice process
+    // twice: in that case the 2nd instance forwards to the 1st instance and then uses the same code
+    // path.
+    for (int i = 0; i < 2; ++i)
+    {
+        auto pURL = std::make_unique<OUString>(m_directories.getURLFromSrc(DATA_DIRECTORY)
+                                               + "double-loading.odt");
+        Application::PostUserEvent(LINK(nullptr, DocumentOpener, OpenDocument), pURL.release());
+    }
+    Scheduler::ProcessEventsToIdle();
+
+    // Verify that the 2nd load didn't happen, since it's the same document.
+    uno::Reference<frame::XFrames> xFrames = mxDesktop->getFrames();
+    // Without the accompanying fix in place, this failed with:
+    // - Expected: 1
+    // - Actual  : 2
+    // i.e. the document was loaded twice, into two separate frames/windows.
+    CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(1), xFrames->getCount());
+
+    // Close the document, now that we know we have a single one.
+    uno::Reference<frame::XFrame> xFrame(xFrames->getByIndex(0), uno::UNO_QUERY);
+    xFrame->getController()->getModel()->dispose();
+}
+}
+
+CPPUNIT_PLUGIN_IMPLEMENT();
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/framework/source/loadenv/loadenv.cxx b/framework/source/loadenv/loadenv.cxx
index b25b0fea8f3a..dbe80f802c25 100644
--- a/framework/source/loadenv/loadenv.cxx
+++ b/framework/source/loadenv/loadenv.cxx
@@ -1154,6 +1154,13 @@ bool LoadEnv::impl_loadContent()
     }
     else if (xSyncLoader.is())
     {
+        uno::Reference<beans::XPropertySet> xTargetFrameProps(xTargetFrame, uno::UNO_QUERY);
+        if (xTargetFrameProps.is())
+        {
+            // Set the URL on the frame itself, for the duration of the load, when it has no
+            // controller.
+            xTargetFrameProps->setPropertyValue("URL", uno::makeAny(sURL));
+        }
         bool bResult = xSyncLoader->load(lDescriptor, xTargetFrame);
         // react for the result here, so the outside waiting
         // code can ask for it later.
@@ -1319,23 +1326,38 @@ css::uno::Reference< css::frame::XFrame > LoadEnv::impl_searchAlreadyLoaded()
             if (!xTask.is())
                 continue;
 
+            OUString sURL;
             css::uno::Reference< css::frame::XController > xController = xTask->getController();
             if (!xController.is())
             {
-                xTask.clear ();
-                continue;
+                // If we have no controller, then perhaps there is a load in progress. The frame
+                // itself has the URL in this case.
+                uno::Reference<beans::XPropertySet> xTaskProps(xTask, uno::UNO_QUERY);
+                if (xTaskProps.is())
+                {
+                    xTaskProps->getPropertyValue("URL") >>= sURL;
+                }
+                if (sURL.isEmpty())
+                {
+                    xTask.clear();
+                    continue;
+                }
             }
 
-            css::uno::Reference< css::frame::XModel > xModel = xController->getModel();
-            if (!xModel.is())
+            uno::Reference<frame::XModel> xModel;
+            if (sURL.isEmpty())
             {
-                xTask.clear ();
-                continue;
+                xModel = xController->getModel();
+                if (!xModel.is())
+                {
+                    xTask.clear();
+                    continue;
+                }
+
+                // don't check the complete URL here.
+                // use its main part - ignore optional jumpmarks!
+                sURL = xModel->getURL();
             }
-
-            // don't check the complete URL here.
-            // use its main part - ignore optional jumpmarks!
-            const OUString sURL = xModel->getURL();
             if (!::utl::UCBContentHelper::EqualURLs( m_aURL.Main, sURL ))
             {
                 xTask.clear ();
@@ -1346,12 +1368,18 @@ css::uno::Reference< css::frame::XFrame > LoadEnv::impl_searchAlreadyLoaded()
             // and decide if it's really the same then the one will be.
             // It must be visible and must use the same file revision ...
             // or must not have any file revision set (-1 == -1!)
-            utl::MediaDescriptor lOldDocDescriptor(xModel->getArgs());
-
-            if (lOldDocDescriptor.getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_VERSION(), sal_Int32(-1)) != nNewVersion)
+            utl::MediaDescriptor lOldDocDescriptor;
+            if (xModel.is())
             {
-                xTask.clear ();
-                continue;
+                lOldDocDescriptor = xModel->getArgs();
+
+                if (lOldDocDescriptor.getUnpackedValueOrDefault(
+                        utl::MediaDescriptor::PROP_VERSION(), sal_Int32(-1))
+                    != nNewVersion)
+                {
+                    xTask.clear();
+                    continue;
+                }
             }
 
             // Hidden frames are special.
diff --git a/framework/source/services/frame.cxx b/framework/source/services/frame.cxx
index 562afb695e35..c6930eeed96b 100644
--- a/framework/source/services/frame.cxx
+++ b/framework/source/services/frame.cxx
@@ -424,6 +424,11 @@ private:
     css::uno::WeakReference< css::uno::XInterface > m_xBroadcaster;
 
     FrameContainer                                                          m_aChildFrameContainer;   /// array of child frames
+    /**
+     * URL of the file that is being loaded, when the load already started by we have no controller
+     * yet.
+     */
+    OUString m_aURL;
 };
 
 
@@ -551,6 +556,9 @@ void XFrameImpl::initListeners()
             FRAME_PROPHANDLE_TITLE,
             cppu::UnoType<OUString>::get(),
             css::beans::PropertyAttribute::TRANSIENT));
+    impl_addPropertyInfo(css::beans::Property(FRAME_PROPNAME_ASCII_URL, FRAME_PROPHANDLE_URL,
+                                              cppu::UnoType<OUString>::get(),
+                                              css::beans::PropertyAttribute::TRANSIENT));
 }
 
 /*-************************************************************************************************************
@@ -1533,6 +1541,10 @@ sal_Bool SAL_CALL XFrameImpl::setComponent(const css::uno::Reference< css::awt::
     SolarMutexResettableGuard aWriteLock;
     m_xComponentWindow = xComponentWindow;
     m_xController      = xController;
+
+    // Clear the URL on the frame itself, now that the controller has it.
+    m_aURL.clear();
+
     m_bConnected       = (m_xComponentWindow.is() || m_xController.is());
     bool bIsConnected       = m_bConnected;
     aWriteLock.clear();
@@ -2769,6 +2781,11 @@ void XFrameImpl::impl_setPropertyValue(sal_Int32 nHandle,
                 }
                 break;
 
+        case FRAME_PROPHANDLE_URL:
+        {
+            aValue >>= m_aURL;
+        }
+        break;
         default :
                 SAL_INFO("fwk.frame",  "XFrameImpl::setFastPropertyValue_NoBroadcast(): Invalid handle detected!" );
                 break;
@@ -2812,6 +2829,11 @@ css::uno::Any XFrameImpl::impl_getPropertyValue(sal_Int32 nHandle)
                 }
                 break;
 
+        case FRAME_PROPHANDLE_URL:
+        {
+            aValue <<= m_aURL;
+        }
+        break;
         default :
                 SAL_INFO("fwk.frame", "XFrameImpl::getFastPropertyValue(): Invalid handle detected!" );
                 break;