From fe8290468d53f6d3632044a5d85b8880403d2616 Mon Sep 17 00:00:00 2001
From: Karol Suprynowicz <ksuprynowicz@post.pl>
Date: Thu, 14 Nov 2024 15:23:52 +0100
Subject: [PATCH] Fix access-after-delete during enbtity script engine cleanup

---
 .../script-engine/src/v8/ScriptEngineDebugFlags.h     | 10 ++++++++++
 libraries/script-engine/src/v8/ScriptEngineV8.cpp     | 11 +++++++++--
 libraries/script-engine/src/v8/ScriptEngineV8.h       |  3 +--
 .../script-engine/src/v8/ScriptObjectV8Proxy.cpp      |  2 ++
 libraries/script-engine/src/v8/ScriptObjectV8Proxy.h  |  4 ++++
 5 files changed, 26 insertions(+), 4 deletions(-)
 create mode 100644 libraries/script-engine/src/v8/ScriptEngineDebugFlags.h

diff --git a/libraries/script-engine/src/v8/ScriptEngineDebugFlags.h b/libraries/script-engine/src/v8/ScriptEngineDebugFlags.h
new file mode 100644
index 00000000000..702c7735d22
--- /dev/null
+++ b/libraries/script-engine/src/v8/ScriptEngineDebugFlags.h
@@ -0,0 +1,10 @@
+//
+// Created by ksuprynowicz on 14.11.24.
+//
+
+#ifndef OVERTE_SCRIPTENGINEDEBUGFLAGS_H
+#define OVERTE_SCRIPTENGINEDEBUGFLAGS_H
+
+#define OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+
+#endif  //OVERTE_SCRIPTENGINEDEBUGFLAGS_H
diff --git a/libraries/script-engine/src/v8/ScriptEngineV8.cpp b/libraries/script-engine/src/v8/ScriptEngineV8.cpp
index 5a9dead93f7..5e233d3a90b 100644
--- a/libraries/script-engine/src/v8/ScriptEngineV8.cpp
+++ b/libraries/script-engine/src/v8/ScriptEngineV8.cpp
@@ -258,6 +258,13 @@ ScriptEngineV8::ScriptEngineV8(ScriptManager *manager) : ScriptEngine(manager),
 }
 
 ScriptEngineV8::~ScriptEngineV8() {
+    // Process remaining events to avoid problems with `deleteLater` calling destructor of script proxies after script engine has been deleted:
+    {
+        QEventLoop loop;
+        loop.processEvents();
+    }
+    // This is necessary for script engines that don't run in ScriptManager::run(), for example entity scripts:
+    disconnectSignalProxies();
     deleteUnusedValueWrappers();
 #ifdef OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
     _wasDestroyed = true;
@@ -272,14 +279,14 @@ void ScriptEngineV8::perManagerLoopIterationCleanup() {
 void ScriptEngineV8::disconnectSignalProxies() {
     _signalProxySetLock.lockForRead();
     while (!_signalProxySet.empty()) {
+        auto proxy = *_signalProxySet.begin();
         _signalProxySetLock.unlock();
-        delete *_signalProxySet.begin();
+        delete proxy;
         _signalProxySetLock.lockForRead();
     }
     _signalProxySetLock.unlock();
 }
 
-
 void ScriptEngineV8::deleteUnusedValueWrappers() {
     while (!_scriptValueWrappersToDelete.empty()) {
         auto wrapper = _scriptValueWrappersToDelete.dequeue();
diff --git a/libraries/script-engine/src/v8/ScriptEngineV8.h b/libraries/script-engine/src/v8/ScriptEngineV8.h
index 02352c9ed78..dd2edca41b5 100644
--- a/libraries/script-engine/src/v8/ScriptEngineV8.h
+++ b/libraries/script-engine/src/v8/ScriptEngineV8.h
@@ -35,6 +35,7 @@
 #include "libplatform/libplatform.h"
 #include "v8.h"
 
+#include "ScriptEngineDebugFlags.h"
 #include "../ScriptEngine.h"
 #include "../ScriptManager.h"
 #include "../ScriptException.h"
@@ -58,8 +59,6 @@ using ScriptContextV8Pointer = std::shared_ptr<ScriptContextV8Wrapper>;
 
 const double GARBAGE_COLLECTION_TIME_LIMIT_S = 1.0;
 
-#define OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
-
 Q_DECLARE_METATYPE(ScriptEngine::FunctionSignature)
 
 /// [V8] Implements ScriptEngine for V8 and translates calls for QScriptEngine
diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp
index 7c3fd07e9ba..eeae1e2e943 100644
--- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp
+++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp
@@ -1217,7 +1217,9 @@ ScriptSignalV8Proxy::~ScriptSignalV8Proxy() {
     _objectLifetime.Reset();
     _v8Context.Reset();
 #ifdef OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+    Q_ASSERT(!_wasDeleted);
     Q_ASSERT(!_engine->_wasDestroyed);
+    _wasDeleted = true;
 #endif
     _engine->_signalProxySetLock.lockForWrite();
     _engine->_signalProxySet.remove(this);
diff --git a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h
index 9e3445ca989..14b8f1900eb 100644
--- a/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h
+++ b/libraries/script-engine/src/v8/ScriptObjectV8Proxy.h
@@ -23,6 +23,7 @@
 #include <QtCore/QPointer>
 #include <QtCore/QString>
 
+#include "ScriptEngineDebugFlags.h"
 #include "../ScriptEngine.h"
 #include "../Scriptable.h"
 #include "ScriptEngineV8.h"
@@ -295,6 +296,9 @@ class ScriptSignalV8Proxy final : public ScriptSignalV8ProxyBase, public ReadWri
     // Call counter for debugging purposes. It can be used to determine which signals are overwhelming script engine.
     int _callCounter{0};
     float _totalCallTime_s{ 0.0 };
+#ifdef OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+    std::atomic<bool> _wasDeleted{false};
+#endif
 
     Q_DISABLE_COPY(ScriptSignalV8Proxy)
 };