WIN run main thread redirects ignoring SolarMutex

This way we can drop all the special nReleased handling. Instead
we use the same mechanism as on Mac, where we keep the lock, but
disable it for the main thread. As a security measure we assert on
duplicate redirects, which should not happen.

As a result we can't use SendMessage on the main thread itself,
which would normally just call the WinProc directly. This could be
accomplished by converting the redirect bool into a counter, which
should be safe, as no other thread could acquire the SolarMutex,
as we don't release it.

Change-Id: Icd87b3da37a2489f3cad2bc80215bf93fc41d388
Reviewed-on: https://gerrit.libreoffice.org/42583Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Jan-Marek Glogowski <glogow@fbihome.de>

include/vcl/svapp.hxx

@@ -495,12 +495,6 @@ public:
     */
     static void                 EndYield();
 
-    /** Acquire SolarMutex after it has been temporarily dropped completely.
-
-        This will Reschedule() on WNT and just acquire on other platforms.
-    */
-    static void                 ReAcquireSolarMutex(sal_uLong nReleased);
-
     /** @brief Get the Solar Mutex for this thread.
 
      Get the Solar Mutex that prevents other threads from accessing VCL
@@ -1481,7 +1475,7 @@ class SolarMutexReleaser
     const sal_uInt32 mnReleased;
 public:
     SolarMutexReleaser(): mnReleased(Application::ReleaseSolarMutex()) {}
-    ~SolarMutexReleaser() { Application::ReAcquireSolarMutex(mnReleased); }
+    ~SolarMutexReleaser() { Application::AcquireSolarMutex( mnReleased ); }
 };
 
 VCL_DLLPUBLIC Application* GetpApp();

vcl/README.scheduler

@@ -89,6 +89,21 @@ can be added to the scheduler reasonably.
 
 = Implementation details =
 
+== General: main thread deferral ==
+
+Currently for Mac and Windows, we run main thread deferrals by disabling the
+SolarMutex using a boolean. In the case of the redirect, this makes
+tryToaAcquire and doAcquire return true or 1, while a release is ignored.
+Also the IsCurrentThread() mutex check function will act accordingly, so all
+the DBG_TESTSOLARMUTEX won't fail.
+
+Since we just disable the locks when we start running the deferred code in the
+main thread, we won't let the main thread run into stuff, where it would
+normally wait for the SolarMutex.
+
+Eventually this will move into the GenericSolarMutex. KDE / Qt also does main
+thread redirects using Qt::BlockingQueuedConnection.
+
 == MacOS implementation details ==
 
 Generally the Scheduler is handled as expected, except on resize, which is
@@ -100,9 +115,9 @@ Like the Windows backend, all Cocoa / GUI handling also has to be run in
 the main thread. We're emulating Windows out-of-order PeekMessage processing,
 via a YieldWakeupEvent and two conditionals. When in a RUNINMAIN call, all
 the DBG_TESTSOLARMUTEX calls are disabled, as we can't release the SolarMutex,
-but we can prevent running any other SolarMutex based code. Same for all the
-SolarMutex acquire and release calls, so the calling and the main thread
-don't deadlock. Those wakeup events must be ignored to prevent busy-locks.
+but we can prevent running any other SolarMutex based code. Those wakeup
+events must be ignored to prevent busy-locks. For more info read the "General:
+main thread deferral" section.
 
 We can neither rely on MacOS dispatch_sync code block execution nor the
 message handling, as both can't be prioritized or filtered and the first
@@ -136,6 +151,10 @@ the timer callback message, which is checked before starting the Scheduler.
 This way we can end with multiple timer callback message in the queue, which
 we were asserting.
 
+To run the required GUI code in the main thread without unlocking the
+SolarMutex, we "disable" it. For more infos read the "General: main thread
+deferral" section.
+
 == KDE implementation details ==
 
 This implementation also works as intended. But there is a different Yield

vcl/headless/svpinst.cxx

@@ -304,12 +304,9 @@ SalBitmap* SvpSalInstance::CreateSalBitmap()
 #endif
 }
 
-bool SvpSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents, sal_uLong const nReleased)
+bool SvpSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents)
 {
-    (void) nReleased;
-    assert(nReleased == 0); // not implemented
     // first, check for already queued events.
-
     std::list< SalUserEvent > aEvents;
     {
         osl::MutexGuard g(m_aEventGuard);

vcl/inc/headless/svpinst.hxx

@@ -155,7 +155,7 @@ public:
     // wait next event and dispatch
     // must returned by UserEvent (SalFrame::PostEvent)
     // and timer
-    virtual bool            DoYield(bool bWait, bool bHandleAllCurrentEvents, sal_uLong nReleased) override;
+    virtual bool            DoYield(bool bWait, bool bHandleAllCurrentEvents) override;
     virtual bool            AnyInput( VclInputFlags nType ) override;
     virtual bool            IsMainThread() const override { return true; }
 

vcl/inc/osx/salinst.h

@@ -114,8 +114,7 @@ public:
     virtual comphelper::SolarMutex* GetYieldMutex() override;
     virtual sal_uInt32      ReleaseYieldMutex( bool bUnlockAll = false ) override;
     virtual void            AcquireYieldMutex( sal_uInt32 nCount = 1 ) override;
-    virtual bool            DoYield(bool bWait, bool bHandleAllCurrentEvents,
-                                    sal_uLong nReleased) override;
+    virtual bool            DoYield(bool bWait, bool bHandleAllCurrentEvents) override;
     virtual bool            AnyInput( VclInputFlags nType ) override;
     virtual SalMenu*        CreateMenu( bool bMenuBar, Menu* pVCLMenu ) override;
     virtual void            DestroyMenu( SalMenu* ) override;

vcl/inc/salinst.hxx

@@ -133,7 +133,7 @@ public:
      * If bHandleAllCurrentEvents - dispatch multiple posted
      * user events. Returns true if events were processed.
      */
-    virtual bool           DoYield(bool bWait, bool bHandleAllCurrentEvents, sal_uLong nReleased) = 0;
+    virtual bool           DoYield(bool bWait, bool bHandleAllCurrentEvents) = 0;
     virtual bool           AnyInput( VclInputFlags nType ) = 0;
 
     // menus

vcl/inc/unx/gtk/gtkinst.hxx

@@ -208,7 +208,7 @@ public:
                                                      const SystemGraphicsData* = nullptr ) override;
     virtual SalBitmap*          CreateSalBitmap() override;
 
-    virtual bool                DoYield(bool bWait, bool bHandleAllCurrentEvents, sal_uLong nReleased) override;
+    virtual bool                DoYield(bool bWait, bool bHandleAllCurrentEvents) override;
     virtual bool                AnyInput( VclInputFlags nType ) override;
     // impossible to handle correctly, as "main thread" depends on the dispatch mutex
     virtual bool                IsMainThread() const override { return false; }

vcl/inc/unx/salinst.h

@@ -74,7 +74,7 @@ public:
     virtual SalSession*         CreateSalSession() override;
     virtual OpenGLContext*      CreateOpenGLContext() override;
 
-    virtual bool                DoYield(bool bWait, bool bHandleAllCurrentEvents, sal_uLong nReleased) override;
+    virtual bool                DoYield(bool bWait, bool bHandleAllCurrentEvents) override;
     virtual bool                AnyInput( VclInputFlags nType ) override;
     virtual bool                IsMainThread() const override { return true; }
 

vcl/inc/win/salinst.h

@@ -34,6 +34,9 @@ public:
     /// The Yield mutex ensures that only one thread calls into VCL
     SalYieldMutex*      mpSalYieldMutex;
 
+    osl::Condition      maWaitingYieldCond;
+    bool                mbNoYieldLock;
+
 public:
     WinSalInstance();
     virtual ~WinSalInstance() override;
@@ -63,7 +66,7 @@ public:
     virtual void                AcquireYieldMutex( sal_uInt32 nCount = 1 ) override;
     virtual bool                IsMainThread() const override;
 
-    virtual bool                DoYield(bool bWait, bool bHandleAllCurrentEvents, sal_uLong nReleased) override;
+    virtual bool                DoYield(bool bWait, bool bHandleAllCurrentEvents) override;
     virtual bool                AnyInput( VclInputFlags nType ) override;
     virtual SalMenu*            CreateMenu( bool bMenuBar, Menu* ) override;
     virtual void                DestroyMenu( SalMenu* ) override;

vcl/osx/salinst.cxx

@@ -533,10 +533,8 @@ SAL_WNODEPRECATED_DECLARATIONS_PUSH
 SAL_WNODEPRECATED_DECLARATIONS_POP
 }
 
-bool AquaSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents, sal_uLong const nReleased)
+bool AquaSalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents)
 {
-    (void) nReleased;
-    assert(nReleased == 0); // not implemented
     bool bHadEvent = false;
 
     // ensure that the per thread autorelease pool is top level and

vcl/qa/cppunit/timer.cxx

@@ -148,7 +148,7 @@ void TimerTest::testIdleMainloop()
         // can't test this via Application::Yield since this
         // also processes all tasks directly via the scheduler.
         pSVData->maAppData.mnDispatchLevel++;
-        pSVData->mpDefInst->DoYield(true, false, 0);
+        pSVData->mpDefInst->DoYield(true, false);
         pSVData->maAppData.mnDispatchLevel--;
     }
     CPPUNIT_ASSERT_MESSAGE("mainloop idle triggered", bTriggered);

vcl/source/app/svapp.cxx

@@ -451,12 +451,12 @@ void Application::Execute()
     pSVData->maAppData.mbInAppExecute = false;
 }
 
-inline bool ImplYield(bool i_bWait, bool i_bAllEvents, sal_uLong const nReleased)
+inline bool ImplYield(bool i_bWait, bool i_bAllEvents)
 {
     ImplSVData* pSVData = ImplGetSVData();
 
     SAL_INFO("vcl.schedule", "Enter ImplYield: " << (i_bWait ? "wait" : "no wait") <<
-             ": " << (i_bAllEvents ? "all events" : "one event") << ": " << nReleased);
+             ": " << (i_bAllEvents ? "all events" : "one event"));
 
     // TODO: there's a data race here on WNT only because ImplYield may be
     // called without SolarMutex; if we can get rid of LazyDelete (with VclPtr)
@@ -466,10 +466,8 @@ inline bool ImplYield(bool i_bWait, bool i_bAllEvents, sal_uLong const nReleased
 
     // do not wait for events if application was already quit; in that
     // case only dispatch events already available
-    bool bProcessedEvent =
-        pSVData->mpDefInst->DoYield(
-            i_bWait && !pSVData->maAppData.mbAppQuit,
-            i_bAllEvents, nReleased);
+    bool bProcessedEvent = pSVData->mpDefInst->DoYield(
+            i_bWait && !pSVData->maAppData.mbAppQuit, i_bAllEvents );
 
     pSVData->maAppData.mnDispatchLevel--;
 
@@ -485,7 +483,7 @@ inline bool ImplYield(bool i_bWait, bool i_bAllEvents, sal_uLong const nReleased
 
 bool Application::Reschedule( bool i_bAllEvents )
 {
-    return ImplYield(false, i_bAllEvents, 0);
+    return ImplYield(false, i_bAllEvents);
 }
 
 void Scheduler::ProcessEventsToSignal(bool& bSignal)
@@ -537,27 +535,7 @@ SAL_DLLPUBLIC_EXPORT void unit_lok_process_events_to_idle()
 
 void Application::Yield()
 {
-    ImplYield(true, false, 0);
-}
-
-void Application::ReAcquireSolarMutex(sal_uLong const nReleased)
-{
-    // 0 would mean that events/timers will be handled without locking
-    // SolarMutex (racy)
-    SAL_WARN_IF(nReleased == 0, "vcl", "SolarMutexReleaser without SolarMutex");
-#ifdef _WIN32
-    if (nReleased == 0 || ImplGetSVData()->mbDeInit) //do not Yield in DeInitVCL
-        AcquireSolarMutex(nReleased);
-    else
-        ImplYield(false, false, nReleased);
-#else
-    // a) Yield is not needed on non-WNT platforms
-    // b) some Yield implementations for X11 (e.g. kde4) make it non-obvious
-    //    how to use nReleased
-    // c) would require a review of what all Yield implementations do
-    //    currently _before_ releasing SolarMutex that would run without lock
-    AcquireSolarMutex(nReleased);
-#endif
+    ImplYield(true, false);
 }
 
 IMPL_STATIC_LINK_NOARG( ImplSVAppData, ImplQuitMsg, void*, void )

vcl/unx/generic/app/salinst.cxx

@@ -166,10 +166,8 @@ bool X11SalInstance::AnyInput(VclInputFlags nType)
     return bRet;
 }
 
-bool X11SalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents, sal_uLong const nReleased)
+bool X11SalInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents)
 {
-    (void) nReleased;
-    assert(nReleased == 0); // not implemented
     return mpXLib->Yield( bWait, bHandleAllCurrentEvents );
 }
 

vcl/unx/gtk/gtkinst.cxx

@@ -411,10 +411,8 @@ void GtkInstance::RemoveTimer ()
     m_pTimer = nullptr;
 }
 
-bool GtkInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents, sal_uLong const nReleased)
+bool GtkInstance::DoYield(bool bWait, bool bHandleAllCurrentEvents)
 {
-    (void) nReleased;
-    assert(nReleased == 0); // not implemented
     EnsureInit();
     return GetGtkSalData()->Yield( bWait, bHandleAllCurrentEvents );
 }

vcl/win/window/salframe.cxx

@@ -1054,10 +1054,15 @@ void WinSalFrame::ReleaseGraphics( SalGraphics* pGraphics )
             if ( mpGraphics2->getDefPal() )
                 SelectPalette( mpGraphics2->getHDC(), mpGraphics2->getDefPal(), TRUE );
             mpGraphics2->DeInitGraphics();
-            SendMessageW( pSalData->mpFirstInstance->mhComWnd,
-                             SAL_MSG_RELEASEDC,
-                             reinterpret_cast<WPARAM>(mhWnd),
-                             reinterpret_cast<LPARAM>(mpGraphics2->getHDC()) );
+            // we don't want to run the WinProc in the main thread directly
+            // so we don't hit the mbNoYieldLock assert
+            if ( !pSalData->mpFirstInstance->IsMainThread() )
+                SendMessageW( pSalData->mpFirstInstance->mhComWnd,
+                              SAL_MSG_RELEASEDC,
+                              reinterpret_cast<WPARAM>(mhWnd),
+                              reinterpret_cast<LPARAM>(mpGraphics2->getHDC()) );
+            else
+                ReleaseDC( mhWnd, mpGraphics2->getHDC() );
             mpGraphics2->setHDC(nullptr);
             pSalData->mnCacheDCInUse--;
         }