Added script value and script value proxy counters

libraries/script-engine/src/ScriptEngine.h

@@ -26,6 +26,9 @@
 #include "ScriptValue.h"
 #include "ScriptException.h"
 
+// These are used for debugging memory leaks caused by persistent handles
+#define OVERTE_V8_HANDLE_COUNTERS
+
 class QByteArray;
 class QLatin1String;
 class QString;
@@ -53,6 +56,10 @@ public:
     size_t totalAvailableSize;
     size_t totalGlobalHandlesSize;
     size_t usedGlobalHandlesSize;
+#ifdef OVERTE_V8_HANDLE_COUNTERS
+    size_t scriptValueCount;
+    size_t scriptValueProxyCount;
+#endif
 };
 
 /**

libraries/script-engine/src/ScriptManagerScriptingInterface.cpp

@@ -61,6 +61,10 @@ QVariantMap ScriptManagerScriptingInterface::getMemoryUsageStatistics() {
     map.insert("totalAvailableSize", QVariant((qulonglong)(statistics.totalAvailableSize)));
     map.insert("totalGlobalHandlesSize", QVariant((qulonglong)(statistics.totalGlobalHandlesSize)));
     map.insert("usedGlobalHandlesSize", QVariant((qulonglong)(statistics.usedGlobalHandlesSize)));
+#ifdef OVERTE_V8_HANDLE_COUNTERS
+    map.insert("scriptValueCount", QVariant((qulonglong)(statistics.scriptValueCount)));
+    map.insert("scriptValueProxyCount", QVariant((qulonglong)(statistics.scriptValueProxyCount)));
+#endif
     return map;
 }
 

libraries/script-engine/src/ScriptValue.h

@@ -179,7 +179,7 @@ public:
     virtual QObject* toQObject() const = 0;
 
 protected:
-    ~ScriptValueProxy() {}  // prevent explicit deletion of base class
+    virtual ~ScriptValueProxy() {}  // prevent explicit deletion of base class
 };
 
 // the second template parameter is used to defer evaluation of calls to the engine until ScriptEngine isn't forward-declared

libraries/script-engine/src/v8/ScriptEngineV8.cpp

@@ -1720,6 +1720,10 @@ ScriptEngineMemoryStatistics ScriptEngineV8::getMemoryUsageStatistics() {
     statistics.totalAvailableSize = heapStatistics.total_available_size();
     statistics.totalGlobalHandlesSize = heapStatistics.total_global_handles_size();
     statistics.usedGlobalHandlesSize = heapStatistics.used_global_handles_size();
+#ifdef OVERTE_V8_HANDLE_COUNTERS
+    statistics.scriptValueCount = scriptValueCount;
+    statistics.scriptValueProxyCount = scriptValueProxyCount;
+#endif
 
     return statistics;
 }

libraries/script-engine/src/v8/ScriptEngineV8.h

@@ -205,6 +205,13 @@ public: // not for public use, but I don't like how Qt strings this along with p
     void popContext();
     // V8TODO: call this after initializing global object
     void storeGlobalObjectContents();
+#ifdef OVERTE_V8_HANDLE_COUNTERS
+    void incrementScriptValueCounter() { scriptValueCount++; };
+    void decrementScriptValueCounter() { scriptValueCount--; };
+    void incrementScriptValueProxyCounter() { scriptValueProxyCount++; };
+    void decrementScriptValueProxyCounter() { scriptValueProxyCount--; };
+    //V8TODO: do the same for other proxy objects
+#endif
 
 protected:
     // like `newFunction`, but allows mapping inline C++ lambdas with captures as callable V8ScriptValues
@@ -257,6 +264,10 @@ protected:
     //ArrayBufferClass* _arrayBufferClass;
     // Counts how many nested evaluate calls are there at a given point
     int _evaluatingCounter;
+#ifdef OVERTE_V8_HANDLE_COUNTERS
+    std::atomic<size_t> scriptValueCount{0};
+    std::atomic<size_t> scriptValueProxyCount{0};
+#endif
 };
 
 #include "V8Types.h"

libraries/script-engine/src/v8/ScriptObjectV8Proxy.cpp

@@ -376,7 +376,6 @@ void ScriptObjectV8Proxy::investigate() {
     v8Object->SetInternalField(2, propertiesObject);
     _v8Object.Reset(_engine->getIsolate(), v8Object);
     if (_ownsObject) {
-        qDebug(scriptengine_v8) << "ScriptObjectV8Proxy::investigate SetWeak";
         _v8Object.SetWeak(this, weakHandleCallback, v8::WeakCallbackType::kParameter);
     }
 

libraries/script-engine/src/v8/ScriptValueV8Wrapper.cpp

@@ -21,7 +21,7 @@
 
 void ScriptValueV8Wrapper::release() {
     // V8TODO: maybe add an assert to check if it happens on script engine thread?
-    // With v8::Locker in V8ScriptValue such requirement shouldn't be necessary but deleting on different threadwww can cause deadlocks sometimes
+    // With v8::Locker in V8ScriptValue such requirement shouldn't be necessary but deleting on different thread can cause deadlocks sometimes
     delete this;
 }
 
@@ -31,8 +31,8 @@ ScriptValueProxy* ScriptValueV8Wrapper::copy() const {
     v8::Locker locker(isolate);
     v8::Isolate::Scope isolateScope(isolate);
     v8::HandleScope handleScope(isolate);
-    // V8TODO: I'm not sure if this part is right:
     v8::Context::Scope contextScope(_engine->getContext());
+    // V8TODO: I'm not sure if this part is right:
     ScriptValueV8Wrapper *copiedWrapper = new ScriptValueV8Wrapper(_engine, _value);
     return copiedWrapper;
 }

libraries/script-engine/src/v8/ScriptValueV8Wrapper.h

@@ -29,14 +29,16 @@
 /// [V8] Implements ScriptValue for V8 and translates calls for V8ScriptValue
 class ScriptValueV8Wrapper final : public ScriptValueProxy {
 public: // construction
+    ScriptValueV8Wrapper() = delete;
+    //ScriptValueV8Wrapper(ScriptValueV8Wrapper &) = delete;
     inline ScriptValueV8Wrapper(ScriptEngineV8* engine, const V8ScriptValue& value) :
-        _engine(engine), _value(value) {}
+        _engine(engine), _value(value) { engine->incrementScriptValueProxyCounter(); }
     inline ScriptValueV8Wrapper(ScriptEngineV8* engine, V8ScriptValue&& value) :
-        _engine(engine), _value(std::move(value)) {}
+        _engine(engine), _value(std::move(value)) { engine->incrementScriptValueProxyCounter(); }
     static ScriptValueV8Wrapper* unwrap(const ScriptValue& val);
     inline const V8ScriptValue& toV8Value() const { return _value; }
     static V8ScriptValue fullUnwrap(ScriptEngineV8* engine, const ScriptValue& value);
-    ScriptEngineV8* getV8Engine() {return _engine;}
+    ScriptEngineV8* getV8Engine() {return _engine;};
 
 public:
     virtual void release() override;
@@ -91,12 +93,18 @@ public:  // ScriptValue implementation
     virtual QVariant toVariant() const override;
     virtual QObject* toQObject() const override;
 
+protected:
+    virtual ~ScriptValueV8Wrapper() { _engine->decrementScriptValueProxyCounter(); };
+
 private: // helper functions
     V8ScriptValue fullUnwrap(const ScriptValue& value) const;
 
 private: // storage
     ScriptEngineV8 *_engine;
+    //V8TODO: this needs a persistent handle instead, maybe with set weak?
     V8ScriptValue _value;
+
+    Q_DISABLE_COPY(ScriptValueV8Wrapper)
 };
 
 #endif  // hifi_ScriptValueV8Wrapper_h

libraries/script-engine/src/v8/V8Types.h

@@ -47,6 +47,9 @@ public:
             return;
         }
         Q_ASSERT(false);*/
+#ifdef OVERTE_V8_HANDLE_COUNTERS
+        _engine->incrementScriptValueCounter();
+#endif
         _value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), value));
     };
 
@@ -67,6 +70,9 @@ public:
         v8::HandleScope handleScope(_engine->getIsolate());
         v8::Context::Scope(_engine->getContext());
         //_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), v8::Local<T>()));
+#ifdef OVERTE_V8_HANDLE_COUNTERS
+        _engine->incrementScriptValueCounter();
+#endif
         _value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), v8::Local<T>()));
     };
 
@@ -76,6 +82,9 @@ public:
         v8::HandleScope handleScope(_engine->getIsolate());
         v8::Context::Scope(_engine->getContext());
         //_value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), copied.constGet()));
+#ifdef OVERTE_V8_HANDLE_COUNTERS
+        _engine->incrementScriptValueCounter();
+#endif
         _value.reset(new v8::UniquePersistent<T>(_engine->getIsolate(), copied.constGet()));
     }
 
@@ -127,6 +136,9 @@ public:
         v8::Isolate::Scope isolateScope(_engine->getIsolate());
         v8::HandleScope handleScope(_engine->getIsolate());
         //v8::Context::Scope(_engine->getContext());
+#ifdef OVERTE_V8_HANDLE_COUNTERS
+        _engine->decrementScriptValueCounter();
+#endif
         _value->Reset();
     }
 

scripts/developer/debugging/QObjectWrapperGCTest.js

@@ -13,6 +13,24 @@
 // A simple script that prints memory usage statistics for a given script engine every 5 seconds
 // It can be included for example as a part of default scripts or controller scripts
 
+var memoryStatisticsIntervalHandle = Script.setInterval(function () {
+    statistics = Script.getMemoryUsageStatistics();
+    if (statistics.scriptValueCount != null) {
+        print("GC test script memory usage: Total heap size: " + statistics.totalHeapSize
+            + " usedHeapSize: " + statistics.usedHeapSize
+            + " totalAvailableSize: " + statistics.totalAvailableSize
+            + " totalGlobalHandlesSize: " + statistics.totalGlobalHandlesSize
+            + " usedGlobalHandlesSize: " + statistics.usedGlobalHandlesSize
+            + " scriptValueCount: " + statistics.scriptValueCount
+            + " scriptValueProxyCount: " + statistics.scriptValueProxyCount);
+    } else {
+        print("GC test script memory usage: Total heap size: " + statistics.totalHeapSize
+            + " usedHeapSize: " + statistics.usedHeapSize
+            + " totalAvailableSize: " + statistics.totalAvailableSize
+            + " totalGlobalHandlesSize: " + statistics.totalGlobalHandlesSize
+            + " usedGlobalHandlesSize: " + statistics.usedGlobalHandlesSize);
+    }
+}, 5000);
 Script.setInterval(function () {
     for (let i = 0; i < 50000; i++) {
         let dbgobj = Script.createGarbageCollectorDebuggingObject();

scripts/developer/debugging/scriptMemoryReport.js

@@ -15,11 +15,21 @@
 
 var memoryStatisticsIntervalHandle = Script.setInterval(function () {
     statistics = Script.getMemoryUsageStatistics();
-    print("Script memory usage: Total heap size: " + statistics.totalHeapSize
-        + " usedHeapSize: " + statistics.usedHeapSize
-        + " totalAvailableSize: " + statistics.totalAvailableSize
-        + " totalGlobalHandlesSize: " + statistics.totalGlobalHandlesSize
-        + " usedGlobalHandlesSize: " + statistics.usedGlobalHandlesSize);
+    if (statistics.scriptValueCount != null) {
+        print("Script memory usage: Total heap size: " + statistics.totalHeapSize
+            + " usedHeapSize: " + statistics.usedHeapSize
+            + " totalAvailableSize: " + statistics.totalAvailableSize
+            + " totalGlobalHandlesSize: " + statistics.totalGlobalHandlesSize
+            + " usedGlobalHandlesSize: " + statistics.usedGlobalHandlesSize
+            + " scriptValueCount: " + statistics.scriptValueCount
+            + " scriptValueProxyCount: " + statistics.scriptValueProxyCount);
+    } else {
+        print("Script memory usage: Total heap size: " + statistics.totalHeapSize
+            + " usedHeapSize: " + statistics.usedHeapSize
+            + " totalAvailableSize: " + statistics.totalAvailableSize
+            + " totalGlobalHandlesSize: " + statistics.totalGlobalHandlesSize
+            + " usedGlobalHandlesSize: " + statistics.usedGlobalHandlesSize);
+    }
 }, 5000);
 
 Script.scriptEnding.connect(function () {