[win] Don't add new ref to a System being destroyed (fix aseprite/aseprite#4811)

Regression introduced in 3bea531,
when os::instance() was refactored to os::System::instance() to return
a SystemRef instead of a System*, and ~WindowWin() destructor asks for
the current system() (and WindowWin::system() is trying to create a
new System reference, but the System is being destroyed as it was
completely unreferred, i.e. ref=0.)

Author: dacap

os/common/system.cpp

@@ -25,21 +25,31 @@
   #include "clip/clip.h"
 #endif
 
+#include "base/debug.h"
+
 namespace os {
 
 // Weak reference to the unique system instance. This is destroyed by
 // the user (with the main SystemRef to the system).
 System* g_instance = nullptr;
 
+// Flag to know if the intance is already being destroyed, so we
+// cannot add a ref to it, i.e. calling System::instance() is illegal
+// if this flag is true.
+static bool g_is_being_destroyed = false;
+
 SystemRef System::instance()
 {
+  ASSERT(!g_is_being_destroyed);
   if (g_instance)
     return AddRef(g_instance);
   return nullptr;
 }
 
 SystemRef System::make()
 {
+  ASSERT(!g_instance);
+
   SystemRef ref;
 #if LAF_SKIA
   ref = System::makeSkia();
@@ -72,7 +82,7 @@ CommonSystem::~CommonSystem()
 {
   // destroyInstance() can be called multiple times by derived
   // classes.
-  if (instance() == this)
+  if (g_instance == this)
     destroyInstance();
 }
 
@@ -208,8 +218,12 @@ void CommonSystem::destroyInstance()
 {
   // destroyInstance() can be called multiple times by derived
   // classes.
-  if (g_instance != this)
+  if (g_instance != this) {
+    ASSERT(g_is_being_destroyed);
     return;
+  }
+
+  g_is_being_destroyed = true;
 
   // We have to reset the list of all events to clear all possible
   // living WindowRef (so all window destructors are called at this

os/win/window.cpp

@@ -66,6 +66,9 @@
 
 namespace os {
 
+// Extern raw reference to current system instance.
+extern System* g_instance;
+
 // Converts an os::Hit to a Win32 hit test value
 static int hit2hittest[] = {
   HTNOWHERE,                    // os::Hit::None
@@ -135,7 +138,7 @@ static BOOL CALLBACK log_monitor_info(HMONITOR monitor,
 
 std::wstring get_wnd_class_name()
 {
-  if (auto sys = instance()) {
+  if (auto sys = os::System::instance()) {
     if (!sys->appName().empty())
       return base::from_utf8(sys->appName());
   }
@@ -322,12 +325,17 @@ WindowWin::WindowWin(const WindowSpec& spec)
 
 WindowWin::~WindowWin()
 {
-  auto sys = system();
+  // We cannot use os::System::instance() here because that would add
+  // a new reference to a possible dying System pointer, e.g. when we
+  // come from ~System because the last Ref::unref() was called and
+  // this window is being destroyed because its last reference was in
+  // a os::Event of the os::EventQueue.
+  SystemWin* sys = (SystemWin*)g_instance;
 
   // If this assert fails it's highly probable that an os::WindowRef
   // was kept alive in some kind of memory leak (or just inside an
   // os::Event in the os::EventQueue). Also this can happen when
-  // declaring a os::WindowRef before calling os::make_system(),
+  // declaring a os::WindowRef before calling os::System::system(),
   // because of deletion order when destructors got called.
   ASSERT(sys);