Delete JS-owned QML objects right away in the engine dtor.

This prevents memory leaks when the engine is destroyed after exec()
has already finished. In most cases this happens during application
shutdown, at which point the event loop is never entered again.

Task-number: QTBUG-20377
Change-Id: I65564ed3e56314d656d92fd66f11ae67d4eb932b
Reviewed-by: Jędrzej Nowacki <jedrzej.nowacki@nokia.com>
Reviewed-by: Lars Knoll <lars.knoll@nokia.com>

11 files changed, 119 insertions, 24 deletions

diff --git a/src/declarative/qml/qdeclarativeengine.cpp b/src/declarative/qml/qdeclarativeengine.cpp
index 92a7391..91390cd 100644
--- a/src/declarative/qml/qdeclarativeengine.cpp
+++ b/src/declarative/qml/qdeclarativeengine.cpp
@@ -350,9 +350,9 @@ QDeclarativeEnginePrivate::QDeclarativeEnginePrivate(QDeclarativeEngine *e)
 : captureProperties(false), rootContext(0), isDebugging(false),
   outputWarningsToStdErr(true), contextClass(0), sharedContext(0), sharedScope(0),
   objectClass(0), valueTypeClass(0), globalClass(0), cleanup(0), erroredBindings(0),
-  inProgressCreations(0), scriptEngine(this), workerScriptEngine(0), componentAttached(0),
-  inBeginCreate(false), networkAccessManager(0), networkAccessManagerFactory(0),
-  typeLoader(e), importDatabase(e), uniqueId(1)
+  inProgressCreations(0), scriptEngine(new QDeclarativeScriptEngine(this)),
+  workerScriptEngine(0), componentAttached(0), inBeginCreate(false), networkAccessManager(0),
+  networkAccessManagerFactory(0), typeLoader(e), importDatabase(e), uniqueId(1)
 {
     if (!qt_QmlQtModule_registered) {
         qt_QmlQtModule_registered = true;
@@ -365,7 +365,7 @@ QDeclarativeEnginePrivate::QDeclarativeEnginePrivate(QDeclarativeEngine *e)
         }
         QDeclarativeUtilModule::defineModule(appType);
     }
-    globalClass = new QDeclarativeGlobalScriptClass(&scriptEngine);
+    globalClass = new QDeclarativeGlobalScriptClass(scriptEngine);
 }
 
 /*!
@@ -500,6 +500,12 @@ QDeclarativeEnginePrivate::~QDeclarativeEnginePrivate()
         c->clear();
     }
 
+    // Destroy the script engine now, before object class is destroyed.
+    // This is needed here since destroying the script engine causes the
+    // JSC heap to be cleaned up, and that needs the object class intact
+    // to work.
+    delete scriptEngine;
+
     delete rootContext;
     rootContext = 0;
     delete contextClass;
@@ -589,7 +595,7 @@ void QDeclarativeEnginePrivate::init()
     rootContext = new QDeclarativeContext(q,true);
 
     QScriptValue applicationObject = objectClass->newQObject(new QDeclarativeApplication(q));
-    scriptEngine.globalObject().property(QLatin1String("Qt")).setProperty(QLatin1String("application"), applicationObject);
+    scriptEngine->globalObject().property(QLatin1String("Qt")).setProperty(QLatin1String("application"), applicationObject);
 
     if (QCoreApplication::instance()->thread() == q->thread() &&
         QDeclarativeEngineDebugService::isDebuggingEnabled()) {
@@ -2111,11 +2117,11 @@ QScriptValue QDeclarativeEnginePrivate::scriptValueFromVariant(const QVariant &v
         if (p->object) {
             return listClass->newList(p->property, p->propertyType);
         } else {
-            return scriptEngine.nullValue();
+            return scriptEngine->nullValue();
         }
     } else if (val.userType() == qMetaTypeId<QList<QObject *> >()) {
         const QList<QObject *> &list = *(QList<QObject *>*)val.constData();
-        QScriptValue rv = scriptEngine.newArray(list.count());
+        QScriptValue rv = scriptEngine->newArray(list.count());
         for (int ii = 0; ii < list.count(); ++ii) {
             QObject *object = list.at(ii);
             rv.setProperty(ii, objectClass->newQObject(object));
@@ -2130,7 +2136,7 @@ QScriptValue QDeclarativeEnginePrivate::scriptValueFromVariant(const QVariant &v
     if (objOk) {
         return objectClass->newQObject(obj);
     } else {
-        return scriptEngine.toScriptValue(val);
+        return scriptEngine->toScriptValue(val);
     }
 }
 
@@ -2297,13 +2303,13 @@ bool QDeclarativeEngine::importPlugin(const QString &filePath, const QString &ur
 void QDeclarativeEngine::setOfflineStoragePath(const QString& dir)
 {
     Q_D(QDeclarativeEngine);
-    d->scriptEngine.offlineStoragePath = dir;
+    d->scriptEngine->offlineStoragePath = dir;
 }
 
 QString QDeclarativeEngine::offlineStoragePath() const
 {
     Q_D(const QDeclarativeEngine);
-    return d->scriptEngine.offlineStoragePath;
+    return d->scriptEngine->offlineStoragePath;
 }
 
 static void voidptr_destructor(void *v)
diff --git a/src/declarative/qml/qdeclarativeengine_p.h b/src/declarative/qml/qdeclarativeengine_p.h
index c324f7a..ee95120 100644
--- a/src/declarative/qml/qdeclarativeengine_p.h
+++ b/src/declarative/qml/qdeclarativeengine_p.h
@@ -181,7 +181,7 @@ public:
     QDeclarativeDelayedError *erroredBindings;
     int inProgressCreations;
 
-    QDeclarativeScriptEngine scriptEngine;
+    QDeclarativeScriptEngine *scriptEngine;
 
     QDeclarativeWorkerScriptEngine *getWorkerScriptEngine();
     QDeclarativeWorkerScriptEngine *workerScriptEngine;
@@ -311,7 +311,7 @@ public:
     static QScriptValue formatTime(QScriptContext*, QScriptEngine*);
     static QScriptValue formatDateTime(QScriptContext*, QScriptEngine*);
 #endif
-    static QScriptEngine *getScriptEngine(QDeclarativeEngine *e) { return &e->d_func()->scriptEngine; }
+    static QScriptEngine *getScriptEngine(QDeclarativeEngine *e) { return e->d_func()->scriptEngine; }
     static QDeclarativeEngine *getEngine(QScriptEngine *e) { return static_cast<QDeclarativeScriptEngine*>(e)->p->q_func(); }
     static QDeclarativeEnginePrivate *get(QDeclarativeEngine *e) { return e->d_func(); }
     static QDeclarativeEnginePrivate *get(QDeclarativeContext *c) { return (c && c->engine()) ? QDeclarativeEnginePrivate::get(c->engine()) : 0; }
diff --git a/src/declarative/qml/qdeclarativeexpression.cpp b/src/declarative/qml/qdeclarativeexpression.cpp
index f457286..e3dba93 100644
--- a/src/declarative/qml/qdeclarativeexpression.cpp
+++ b/src/declarative/qml/qdeclarativeexpression.cpp
@@ -176,7 +176,7 @@ QScriptValue QDeclarativeExpressionPrivate::evalInObjectScope(QDeclarativeContex
                                                               int lineNumber, QScriptValue *contextObject)
 {
     QDeclarativeEnginePrivate *ep = QDeclarativeEnginePrivate::get(context->engine);
-    QScriptContext *scriptContext = QScriptDeclarativeClass::pushCleanContext(&ep->scriptEngine);
+    QScriptContext *scriptContext = QScriptDeclarativeClass::pushCleanContext(ep->scriptEngine);
     if (contextObject) {
         *contextObject = ep->contextClass->newContext(context, object);
         scriptContext->pushScope(*contextObject);
@@ -184,8 +184,8 @@ QScriptValue QDeclarativeExpressionPrivate::evalInObjectScope(QDeclarativeContex
         scriptContext->pushScope(ep->contextClass->newContext(context, object));
     }
     scriptContext->pushScope(ep->globalClass->staticGlobalObject());
-    QScriptValue rv = ep->scriptEngine.evaluate(program, fileName, lineNumber);
-    ep->scriptEngine.popContext();
+    QScriptValue rv = ep->scriptEngine->evaluate(program, fileName, lineNumber);
+    ep->scriptEngine->popContext();
     return rv;
 }
 
@@ -194,7 +194,7 @@ QScriptValue QDeclarativeExpressionPrivate::evalInObjectScope(QDeclarativeContex
                                                               QScriptValue *contextObject)
 {
     QDeclarativeEnginePrivate *ep = QDeclarativeEnginePrivate::get(context->engine);
-    QScriptContext *scriptContext = QScriptDeclarativeClass::pushCleanContext(&ep->scriptEngine);
+    QScriptContext *scriptContext = QScriptDeclarativeClass::pushCleanContext(ep->scriptEngine);
     if (contextObject) {
         *contextObject = ep->contextClass->newContext(context, object);
         scriptContext->pushScope(*contextObject);
@@ -202,8 +202,8 @@ QScriptValue QDeclarativeExpressionPrivate::evalInObjectScope(QDeclarativeContex
         scriptContext->pushScope(ep->contextClass->newContext(context, object));
     }
     scriptContext->pushScope(ep->globalClass->staticGlobalObject());
-    QScriptValue rv = ep->scriptEngine.evaluate(program);
-    ep->scriptEngine.popContext();
+    QScriptValue rv = ep->scriptEngine->evaluate(program);
+    ep->scriptEngine->popContext();
     return rv;
 }
 
diff --git a/src/declarative/qml/qdeclarativeobjectscriptclass.cpp b/src/declarative/qml/qdeclarativeobjectscriptclass.cpp
index 8f8557f..f71a64d 100644
--- a/src/declarative/qml/qdeclarativeobjectscriptclass.cpp
+++ b/src/declarative/qml/qdeclarativeobjectscriptclass.cpp
@@ -74,14 +74,28 @@ struct ObjectData : public QScriptDeclarativeClass::Object {
         }
     }
 
-    virtual ~ObjectData() {
+    enum Disposal { Immediate, Deferred };
+
+    inline void disposeObject(Disposal disposal) {
         if (object && !object->parent()) {
             QDeclarativeData *ddata = QDeclarativeData::get(object, false);
-            if (ddata && !ddata->indestructible && 0 == --ddata->objectDataRefCount) 
-                object->deleteLater();
+            if (ddata && !ddata->indestructible && 0 == --ddata->objectDataRefCount) {
+                if (disposal == Immediate)
+                    delete object;
+                else
+                    object->deleteLater();
+            }
         }
     }
 
+    virtual void disposeNow() {
+        disposeObject(Immediate);
+    }
+
+    virtual ~ObjectData() {
+        disposeObject(Deferred);
+    }
+
     QDeclarativeGuard<QObject> object;
     int type;
 };
diff --git a/src/script/api/qscriptengine.cpp b/src/script/api/qscriptengine.cpp
index d821084..cebbe0e 100644
--- a/src/script/api/qscriptengine.cpp
+++ b/src/script/api/qscriptengine.cpp
@@ -962,7 +962,7 @@ QScriptEnginePrivate::QScriptEnginePrivate()
       qobjectPrototype(0), qmetaobjectPrototype(0), variantPrototype(0),
       activeAgent(0), agentLineNumber(-1),
       registeredScriptValues(0), freeScriptValues(0), freeScriptValuesCount(0),
-      registeredScriptStrings(0), processEventsInterval(-1), inEval(false)
+      registeredScriptStrings(0), processEventsInterval(-1), inEval(false), inDestructor(false)
 {
     qMetaTypeId<QScriptValue>();
     qMetaTypeId<QList<int> >();
@@ -1015,6 +1015,7 @@ QScriptEnginePrivate::QScriptEnginePrivate()
 
 QScriptEnginePrivate::~QScriptEnginePrivate()
 {
+    inDestructor = true;
     QScript::APIShim shim(this);
 
     //disconnect all loadedScripts and generate all jsc::debugger::scriptUnload events
diff --git a/src/script/api/qscriptengine_p.h b/src/script/api/qscriptengine_p.h
index 6a023d7..8a7037b 100644
--- a/src/script/api/qscriptengine_p.h
+++ b/src/script/api/qscriptengine_p.h
@@ -382,6 +382,7 @@ public:
     int processEventsInterval;
     QScriptValue abortResult;
     bool inEval;
+    bool inDestructor;
 
     JSC::UString cachedTranslationUrl;
     JSC::UString cachedTranslationContext;
diff --git a/src/script/bridge/qscriptdeclarativeclass_p.h b/src/script/bridge/qscriptdeclarativeclass_p.h
index c8c35bf..c67d23d 100644
--- a/src/script/bridge/qscriptdeclarativeclass_p.h
+++ b/src/script/bridge/qscriptdeclarativeclass_p.h
@@ -77,7 +77,14 @@ public:
 
     typedef void* Identifier;
 
-    struct Object { virtual ~Object() {} };
+    struct Object {
+        // Like the destructor, frees resources associated with this object. The difference is that
+        // the destructor might delay resource freeing with deleteLater(), while disposeNow() always
+        // deletes the resource straigt away.
+        virtual void disposeNow() {}
+
+        virtual ~Object() {}
+    };
 
     static QScriptValue newObject(QScriptEngine *, QScriptDeclarativeClass *, Object *);
     static Value newObjectValue(QScriptEngine *, QScriptDeclarativeClass *, Object *);
diff --git a/src/script/bridge/qscriptdeclarativeobject.cpp b/src/script/bridge/qscriptdeclarativeobject.cpp
index 927309a..a9cd87d 100644
--- a/src/script/bridge/qscriptdeclarativeobject.cpp
+++ b/src/script/bridge/qscriptdeclarativeobject.cpp
@@ -53,6 +53,11 @@ DeclarativeObjectDelegate::DeclarativeObjectDelegate(QScriptDeclarativeClass *c,
 
 DeclarativeObjectDelegate::~DeclarativeObjectDelegate()
 {
+    // When the engine is being destructed, delete the object now, instead of using deleteLater(),
+    // to not have memory leaks on exit.
+    if (m_class->engine() && QScriptEnginePrivate::get(m_class->engine())->inDestructor)
+        m_object->disposeNow();
+
     delete m_object;
 }
 
diff --git a/tests/auto/declarative/qdeclarativeecmascript/data/jsOwnedObjectsDeletedOnEngineDestroy.qml b/tests/auto/declarative/qdeclarativeecmascript/data/jsOwnedObjectsDeletedOnEngineDestroy.qml
new file mode 100644
index 0000000..da1b682
--- /dev/null
+++ b/tests/auto/declarative/qdeclarativeecmascript/data/jsOwnedObjectsDeletedOnEngineDestroy.qml
@@ -0,0 +1,6 @@
+import QtQuick 1.0
+
+Item {
+    property variant jsOwnedObject1: deleteObject.object1()
+    property variant jsOwnedObject2: deleteObject.object2
+}
diff --git a/tests/auto/declarative/qdeclarativeecmascript/testtypes.h b/tests/auto/declarative/qdeclarativeecmascript/testtypes.h
index 19505e1..3d0d37f 100644
--- a/tests/auto/declarative/qdeclarativeecmascript/testtypes.h
+++ b/tests/auto/declarative/qdeclarativeecmascript/testtypes.h
@@ -903,6 +903,27 @@ protected:
     qreal m_p4;
 };
 
+class MyDeleteObject : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(QObject *object2 READ object2 NOTIFY object2Changed)
+
+public:
+    MyDeleteObject() : m_object1(0), m_object2(0) {}
+
+    Q_INVOKABLE QObject *object1() const { return m_object1; }
+    Q_INVOKABLE QObject *object2() const { return m_object2; }
+    void setObject1(QObject *object) { m_object1 = object; }
+    void setObject2(QObject *object) { m_object2 = object; emit object2Changed(); }
+
+signals:
+    void object2Changed();
+
+private:
+    QObject *m_object1;
+    QObject *m_object2;
+};
+
 QML_DECLARE_TYPE(MyRevisionedBaseClassRegistered)
 QML_DECLARE_TYPE(MyRevisionedBaseClassUnregistered)
 QML_DECLARE_TYPE(MyRevisionedClass)
diff --git a/tests/auto/declarative/qdeclarativeecmascript/tst_qdeclarativeecmascript.cpp b/tests/auto/declarative/qdeclarativeecmascript/tst_qdeclarativeecmascript.cpp
index 1c54494..188cf1b 100644
--- a/tests/auto/declarative/qdeclarativeecmascript/tst_qdeclarativeecmascript.cpp
+++ b/tests/auto/declarative/qdeclarativeecmascript/tst_qdeclarativeecmascript.cpp
@@ -48,6 +48,7 @@
 #include <QtDeclarative/private/qdeclarativeguard_p.h>
 #include <QtCore/qdir.h>
 #include <QtCore/qnumeric.h>
+#include <QtTest/qsignalspy.h>
 #include <private/qdeclarativeengine_p.h>
 #include <private/qdeclarativeglobalscriptclass_p.h>
 #include <private/qscriptdeclarativeclass_p.h>
@@ -178,6 +179,7 @@ private slots:
     void pushCleanContext();
     void realToInt();
     void qtbug_20648();
+    void jsOwnedObjectsDeletedOnEngineDestroy();
 
     void include();
 
@@ -1388,7 +1390,7 @@ void tst_qdeclarativeecmascript::callQtInvokables()
 
     QDeclarativeEngine qmlengine;
     QDeclarativeEnginePrivate *ep = QDeclarativeEnginePrivate::get(&qmlengine);
-    QScriptEngine *engine = &ep->scriptEngine;
+    QScriptEngine *engine = ep->scriptEngine;
 
     QStringList names; QList<QScriptValue> values;
     names << QLatin1String("object"); values << ep->objectClass->newQObject(&o);
@@ -3100,6 +3102,38 @@ void tst_qdeclarativeecmascript::qtbug_20648()
     delete o;
 }
 
+void tst_qdeclarativeecmascript::jsOwnedObjectsDeletedOnEngineDestroy()
+{
+    QDeclarativeEngine *myEngine = new QDeclarativeEngine;
+
+    MyDeleteObject deleteObject;
+    deleteObject.setObjectName("deleteObject");
+    QObject * const object1 = new QObject;
+    QObject * const object2 = new QObject;
+    object1->setObjectName("object1");
+    object2->setObjectName("object2");
+    deleteObject.setObject1(object1);
+    deleteObject.setObject2(object2);
+
+    // Objects returned by function calls get marked as destructible, but objects returned by
+    // property getters do not - therefore we explicitly set the object as destructible.
+    QDeclarativeEngine::setObjectOwnership(object2, QDeclarativeEngine::JavaScriptOwnership);
+
+    myEngine->rootContext()->setContextProperty("deleteObject", &deleteObject);
+    QDeclarativeComponent component(myEngine, TEST_FILE("jsOwnedObjectsDeletedOnEngineDestroy.qml"));
+    QObject *object = component.create();
+    QVERIFY(object);
+
+    // Destroying the engine should delete all JS owned QObjects
+    QSignalSpy spy1(object1, SIGNAL(destroyed()));
+    QSignalSpy spy2(object2, SIGNAL(destroyed()));
+    delete myEngine;
+    QCOMPARE(spy1.count(), 1);
+    QCOMPARE(spy2.count(), 1);
+
+    delete object;
+}
+
 QTEST_MAIN(tst_qdeclarativeecmascript)
 
 #include "tst_qdeclarativeecmascript.moc"