From 46781ad95418cb94baef3da5e5fcce4c2fae7560 Mon Sep 17 00:00:00 2001
From: Greg <greg@primesoft.co.nz>
Date: Fri, 20 Mar 2026 20:40:57 +1300
Subject: [PATCH] Fix data race on nativeWindowsToRedraw across threads
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

nativeWindowsToRedraw (std::set<RenderNativeWindow*>) is accessed from
multiple threads without synchronisation:

- requestRedraw() inserts from any thread (via NativeSurface callback)
- ~RenderNativeWindow() erases during model operations
- drawNativeWindows() iterates and clears on the minsky thread
- macOSXLaunchDrawNativeWindows() reads .size() on the minsky thread

This is a data race on std::set that can corrupt the container's
internal tree, potentially leading to crashes or infinite loops during
iteration.

Whether this contributes to the freeze reported in tickets:#1906 is
unclear — the SetWindowPos removal in 3.25.1 likely addresses the
primary mechanism. This fix closes a correctness issue that could
cause problems under load (complex models with many native windows),
but may not be the root cause of Steve's reported freeze.

Fix: add a dedicated mutex (nativeWindowsToRedrawMutex) to protect all
accesses. The draw functions swap the set into a local copy under the
new mutex, then release it before acquiring minskyCmdMutex — preserving
existing lock scope and avoiding lock ordering issues.

Also fixes a minor existing bug where macOSXDrawNativeWindows() cleared
nativeWindowsToRedraw AFTER swapping it out, discarding any redraw
requests made during the draw cycle.

Related to tickets:#1906
---
 RESTService/addon.cc        | 19 +++++++++++++------
 model/minsky.h              |  2 ++
 model/renderNativeWindow.cc |  2 ++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/RESTService/addon.cc b/RESTService/addon.cc
index 5e4eb2f0a..4463aa444 100644
--- a/RESTService/addon.cc
+++ b/RESTService/addon.cc
@@ -254,9 +254,14 @@ namespace minsky
 
       void drawNativeWindows()
       {
+        decltype(nativeWindowsToRedraw) windows;
+        {
+          const lock_guard<mutex> lock(nativeWindowsToRedrawMutex);
+          windows.swap(nativeWindowsToRedraw);
+        }
         const lock_guard<recursive_mutex> lock(minskyCmdMutex);
         const Timer timer(timers["draw"]);
-        for (auto i: nativeWindowsToRedraw)
+        for (auto i: windows)
           try
             {
               i->draw();
@@ -268,7 +273,6 @@ namespace minsky
               break;
             }
           catch (...) {break;}
-        nativeWindowsToRedraw.clear();
       }
 
       // arrange for native window drawing to happen on node's main thread, required for MacOSX.
@@ -284,12 +288,11 @@ namespace minsky
         const Timer timer(timers["draw"]);
         decltype(nativeWindowsToRedraw) windows;
         {
-          const lock_guard<recursive_mutex> lock(minskyCmdMutex);
+          const lock_guard<mutex> lock(nativeWindowsToRedrawMutex);
           windows.swap(nativeWindowsToRedraw);
         }
         for (auto i: windows)
           macOSXRedraw(*i,minskyCmdMutex);
-        nativeWindowsToRedraw.clear();
         drawLaunched=false;
        }
       
@@ -336,8 +339,12 @@ namespace minsky
                   catch (...)
                     {flags&=~reset_needed;}
 #ifdef MAC_OSX_TK
-                if (!drawLaunched && nativeWindowsToRedraw.size())
-                  macOSXLaunchDrawNativeWindows();
+                if (!drawLaunched)
+                  {
+                    const lock_guard<mutex> lock(nativeWindowsToRedrawMutex);
+                    if (nativeWindowsToRedraw.size())
+                      macOSXLaunchDrawNativeWindows();
+                  }
 #else
                 drawNativeWindows();
 #endif
diff --git a/model/minsky.h b/model/minsky.h
index 89aaea4c9..4ae0297b9 100644
--- a/model/minsky.h
+++ b/model/minsky.h
@@ -55,6 +55,7 @@
 #include <vector>
 #include <string>
 #include <set>
+#include <mutex>
 #include <deque>
 #include <cstdio>
 
@@ -102,6 +103,7 @@ namespace minsky
     MinskyExclude& operator=(const MinskyExclude&) {return *this;}
 
     /// record nativeWindows that have requested redrawing
+    std::mutex nativeWindowsToRedrawMutex;
     std::set<RenderNativeWindow*> nativeWindowsToRedraw;
     
   protected:
diff --git a/model/renderNativeWindow.cc b/model/renderNativeWindow.cc
index a7ad783c9..00c4059c6 100644
--- a/model/renderNativeWindow.cc
+++ b/model/renderNativeWindow.cc
@@ -73,6 +73,7 @@ namespace minsky
 
   RenderNativeWindow::~RenderNativeWindow()
   {
+    const lock_guard<mutex> lock(minsky().nativeWindowsToRedrawMutex);
     minsky().nativeWindowsToRedraw.erase(this);
   }
   
@@ -106,6 +107,7 @@ void macOSXRedraw(RenderNativeWindow& window,std::recursive_mutex& cmdMutex)
   void RenderNativeWindow::requestRedraw()
   {
     if (!winInfoPtr.get()) return;
+    const lock_guard<mutex> lock(minsky().nativeWindowsToRedrawMutex);
     minsky().nativeWindowsToRedraw.insert(this);
   }