Merge pull request #1236 from overte-org/fix/signal_proxy_crash

Fix access-after-delete during entity script engine cleanup
overte-org · Nov 21, 2024 · 9503fae · 9503fae
2 parents ad5b6d0 + 3485ab1
commit 9503fae
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 4 deletions.
diff --git a/libraries/script-engine/src/v8/ScriptEngineDebugFlags.h b/libraries/script-engine/src/v8/ScriptEngineDebugFlags.h
@@ -0,0 +1,18 @@
+//
+//  ScriptEngineDebugFlags.h
+//  libraries/script-engine/src/v8/ScriptEngineDebugFlags.h
+//
+//  Created by dr Karol Suprynowicz on 2024/11/14.
+//  Copyright 2024 Overte e.V.
+//
+//  Distributed under the Apache License, Version 2.0.
+//  See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html
+//  SPDX-License-Identifier: Apache-2.0
+//
+
+#ifndef overte_ScriptEngineDebugFlags_h
+#define overte_ScriptEngineDebugFlags_h
+
+#define OVERTE_SCRIPT_USE_AFTER_DELETE_GUARD
+
+#endif
diff --git 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
@@ -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
@@ -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
@@ -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)
 };