Avoid calling gc in QQmlEngine destructor.

GC may be expensive. GC call in QQmlEngine doesn't have much sense
because V8 will destroy all objects living in a current context.
The only problem is that V8 may decide to not invoke weak callbacks
which may cause a memory leak. To avoid that we track all QObjects that
have JavaScript ownership set and we delete them explicitly.

The change reduce time of destroying QQmlEngine by 75%, which is really
visible in qquicklistmodel test.

Change-Id: I2a3668fd23630669114baee8c241a7ecc4100e33
Reviewed-by: Chris Adams <christopher.adams@nokia.com>

9 files changed, 213 insertions, 63 deletions

diff --git a/src/qml/qml/qqmlengine.cpp b/src/qml/qml/qqmlengine.cpp
index cef878a073..626a4b03e4 100644
--- a/src/qml/qml/qqmlengine.cpp
+++ b/src/qml/qml/qqmlengine.cpp
@@ -592,9 +592,6 @@ QQmlEngine::~QQmlEngine()
         }
     }
 
-    // ensure we clean up QObjects with JS ownership
-    d->v8engine()->gc();
-
     if (d->incubationController)
         d->incubationController->d = 0;
 }
@@ -899,10 +896,13 @@ void QQmlEngine::setContextForObject(QObject *object, QQmlContext *context)
 
   \value JavaScriptOwnership The object is owned by JavaScript.
   When the object is returned to QML as the return value of a method
-  call or property access, QML will delete the object if there are no
-  remaining JavaScript references to it and it has no
-  QObject::parent().  This option is similar to
-  QScriptEngine::ScriptOwnership.
+  call or property access, QML will track it, and delete the object
+  if there are no remaining JavaScript references to it and it has no
+  QObject::parent().  An object tracked by one QQmlEngine
+  will be deleted during that QQmlEngine's destructor, and thus
+  JavaScript references between objects with JavaScriptOwnership from
+  two different engines will not be valid after the deletion of one of
+  those engines.  This option is similar to QScriptEngine::ScriptOwnership.
 
   Generally an application doesn't need to set an object's ownership
   explicitly.  QML uses a heuristic to set the default object
diff --git a/src/qml/qml/v8/qv8engine_p.h b/src/qml/qml/v8/qv8engine_p.h
index 1fc03d82e5..09e1ae537e 100644
--- a/src/qml/qml/v8/qv8engine_p.h
+++ b/src/qml/qml/v8/qv8engine_p.h
@@ -72,6 +72,7 @@
 
 #include <private/qqmlpropertycache_p.h>
 
+#include "qv8objectresource_p.h"
 #include "qv8contextwrapper_p.h"
 #include "qv8qobjectwrapper_p.h"
 #include "qv8stringwrapper_p.h"
@@ -91,12 +92,6 @@ QT_BEGIN_NAMESPACE
 // a handle, qFatal() is called.
 // #define QML_GLOBAL_HANDLE_DEBUGGING
 
-#define V8_RESOURCE_TYPE(resourcetype) \
-public: \
-    enum { V8ResourceType = QV8ObjectResource:: resourcetype }; \
-    virtual QV8ObjectResource::ResourceType resourceType() const { return QV8ObjectResource:: resourcetype; } \
-private: 
-
 #define V8ENGINE() ((QV8Engine *)v8::External::Unwrap(args.Data()))
 #define V8FUNCTION(function, engine) v8::FunctionTemplate::New(function, v8::External::Wrap((QV8Engine*)engine))->GetFunction()
 #define V8THROW_ERROR(string) { \
@@ -132,21 +127,6 @@ private:
     } \
 
 
-class QV8Engine;
-class QV8ObjectResource : public v8::Object::ExternalResource
-{
-public:
-    QV8ObjectResource(QV8Engine *engine) : engine(engine) { Q_ASSERT(engine); }
-    enum ResourceType { ContextType, QObjectType, TypeType, ListType, VariantType, 
-                        ValueTypeType, XMLHttpRequestType, DOMNodeType, SQLDatabaseType,
-                        ListModelType, Context2DType, Context2DStyleType, Context2DPixelArrayType, 
-                        ParticleDataType, SignalHandlerType, IncubatorType, VisualDataItemType,
-                        SequenceType, LocaleDataType };
-    virtual ResourceType resourceType() const = 0;
-
-    QV8Engine *engine;
-};
-
 template<class T>
 inline T *v8_resource_cast(v8::Handle<v8::Object> object) {
     QV8ObjectResource *resource = static_cast<QV8ObjectResource *>(object->GetExternalResource());
diff --git a/src/qml/qml/v8/qv8objectresource_p.h b/src/qml/qml/v8/qv8objectresource_p.h
new file mode 100644
index 0000000000..c37905a0b8
--- /dev/null
+++ b/src/qml/qml/v8/qv8objectresource_p.h
@@ -0,0 +1,84 @@
+/****************************************************************************
+**
+** Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies).
+** Contact: http://www.qt-project.org/
+**
+** This file is part of the QtQml module of the Qt Toolkit.
+**
+** $QT_BEGIN_LICENSE:LGPL$
+** GNU Lesser General Public License Usage
+** This file may be used under the terms of the GNU Lesser General Public
+** License version 2.1 as published by the Free Software Foundation and
+** appearing in the file LICENSE.LGPL included in the packaging of this
+** file. Please review the following information to ensure the GNU Lesser
+** General Public License version 2.1 requirements will be met:
+** http://www.gnu.org/licenses/old-licenses/lgpl-2.1.html.
+**
+** In addition, as a special exception, Nokia gives you certain additional
+** rights. These rights are described in the Nokia Qt LGPL Exception
+** version 1.1, included in the file LGPL_EXCEPTION.txt in this package.
+**
+** GNU General Public License Usage
+** Alternatively, this file may be used under the terms of the GNU General
+** Public License version 3.0 as published by the Free Software Foundation
+** and appearing in the file LICENSE.GPL included in the packaging of this
+** file. Please review the following information to ensure the GNU General
+** Public License version 3.0 requirements will be met:
+** http://www.gnu.org/copyleft/gpl.html.
+**
+** Other Usage
+** Alternatively, this file may be used in accordance with the terms and
+** conditions contained in a signed written agreement between you and Nokia.
+**
+**
+**
+**
+**
+**
+** $QT_END_LICENSE$
+**
+****************************************************************************/
+
+#ifndef QV8OBJECTRESOURCE_P_H
+#define QV8OBJECTRESOURCE_P_H
+
+//
+//  W A R N I N G
+//  -------------
+//
+// This file is not part of the Qt API.  It exists purely as an
+// implementation detail.  This header file may change from version to
+// version without notice, or even be removed.
+//
+// We mean it.
+//
+
+#include <QtCore/qglobal.h>
+#include <private/qv8_p.h>
+
+QT_BEGIN_NAMESPACE
+
+#define V8_RESOURCE_TYPE(resourcetype) \
+public: \
+    enum { V8ResourceType = QV8ObjectResource:: resourcetype }; \
+    virtual QV8ObjectResource::ResourceType resourceType() const { return QV8ObjectResource:: resourcetype; } \
+private:
+
+class QV8Engine;
+class QV8ObjectResource : public v8::Object::ExternalResource
+{
+public:
+    QV8ObjectResource(QV8Engine *engine) : engine(engine) { Q_ASSERT(engine); }
+    enum ResourceType { ContextType, QObjectType, TypeType, ListType, VariantType,
+                        ValueTypeType, XMLHttpRequestType, DOMNodeType, SQLDatabaseType,
+                        ListModelType, Context2DType, Context2DStyleType, Context2DPixelArrayType,
+                        ParticleDataType, SignalHandlerType, IncubatorType, VisualDataItemType,
+                        SequenceType, LocaleDataType };
+    virtual ResourceType resourceType() const = 0;
+
+    QV8Engine *engine;
+};
+
+QT_END_NAMESPACE
+
+#endif // QV8OBJECTRESOURCE_P_H
diff --git a/src/qml/qml/v8/qv8qobjectwrapper.cpp b/src/qml/qml/v8/qv8qobjectwrapper.cpp
index 70f10b2346..2d6c5ec24a 100644
--- a/src/qml/qml/v8/qv8qobjectwrapper.cpp
+++ b/src/qml/qml/v8/qv8qobjectwrapper.cpp
@@ -80,16 +80,6 @@ QT_BEGIN_NAMESPACE
 // XXX TODO: Need to review all calls to QQmlEngine *engine() to confirm QObjects work
 // correctly in a worker thread
 
-class QV8QObjectResource : public QV8ObjectResource
-{
-    V8_RESOURCE_TYPE(QObjectType);
-
-public:
-    QV8QObjectResource(QV8Engine *engine, QObject *object);
-
-    QQmlGuard<QObject> object;
-};
-
 class QV8QObjectInstance : public QQmlGuard<QObject>
 {
 public:
@@ -223,6 +213,13 @@ void QV8QObjectWrapper::destroy()
     qPersistentDispose(m_signalHandlerConstructor);
     qPersistentDispose(m_methodConstructor);
     qPersistentDispose(m_constructor);
+
+    QIntrusiveList<QV8QObjectResource, &QV8QObjectResource::weakResource>::iterator i = m_javaScriptOwnedWeakQObjects.begin();
+    for (; i != m_javaScriptOwnedWeakQObjects.end(); ++i) {
+        QV8QObjectResource *resource = *i;
+        Q_ASSERT(resource);
+        deleteWeakQObject(resource);
+    }
 }
 
 struct ReadAccessor {
@@ -930,25 +927,14 @@ static void FastValueSetterReadOnly(v8::Local<v8::String> property, v8::Local<v8
     v8::ThrowException(v8::Exception::Error(v8engine->toString(error)));
 }
 
-static void WeakQObjectReferenceCallback(v8::Persistent<v8::Value> handle, void *)
+void QV8QObjectWrapper::WeakQObjectReferenceCallback(v8::Persistent<v8::Value> handle, void *wrapper)
 {
     Q_ASSERT(handle->IsObject());
-    
     QV8QObjectResource *resource = v8_resource_check<QV8QObjectResource>(handle->ToObject());
-
     Q_ASSERT(resource);
 
-    QObject *object = resource->object;
-    if (object) {
-        QQmlData *ddata = QQmlData::get(object, false);
-        if (ddata) {
-            ddata->v8object.Clear();
-            if (!object->parent() && !ddata->indestructible) {
-                ddata->isQueuedForDeletion = true;
-                object->deleteLater();
-            }
-        }
-    }
+    static_cast<QV8QObjectWrapper*>(wrapper)->unregisterWeakQObjectReference(resource);
+    static_cast<QV8QObjectWrapper*>(wrapper)->deleteWeakQObject(resource);
 
     qPersistentDispose(handle);
 }
@@ -1117,8 +1103,10 @@ v8::Handle<v8::Value> QV8QObjectWrapper::newQObject(QObject *object)
 
         v8::Local<v8::Object> rv = newQObject(object, ddata, m_engine);
         ddata->v8object = qPersistentNew<v8::Object>(rv);
-        ddata->v8object.MakeWeak(0, WeakQObjectReferenceCallback);
+        ddata->v8object.MakeWeak(this, WeakQObjectReferenceCallback);
         ddata->v8objectid = m_id;
+        QV8QObjectResource *resource = v8_resource_check<QV8QObjectResource>(rv);
+        registerWeakQObjectReference(resource);
         return rv;
 
     } else {
@@ -1133,8 +1121,10 @@ v8::Handle<v8::Value> QV8QObjectWrapper::newQObject(QObject *object)
         if ((!found || (*iter)->v8object.IsEmpty()) && ddata->v8object.IsEmpty()) {
             v8::Local<v8::Object> rv = newQObject(object, ddata, m_engine);
             ddata->v8object = qPersistentNew<v8::Object>(rv);
-            ddata->v8object.MakeWeak(0, WeakQObjectReferenceCallback);
+            ddata->v8object.MakeWeak(this, WeakQObjectReferenceCallback);
             ddata->v8objectid = m_id;
+            QV8QObjectResource *resource = v8_resource_check<QV8QObjectResource>(rv);
+            registerWeakQObjectReference(resource);
 
             if (found) {
                 delete (*iter);
@@ -1157,6 +1147,20 @@ v8::Handle<v8::Value> QV8QObjectWrapper::newQObject(QObject *object)
         return v8::Local<v8::Object>::New((*iter)->v8object);
     }
 }
+void QV8QObjectWrapper::deleteWeakQObject(QV8QObjectResource *resource)
+{
+    QObject *object = resource->object;
+    if (object) {
+        QQmlData *ddata = QQmlData::get(object, false);
+        if (ddata) {
+            ddata->v8object.Clear();
+            if (!object->parent() && !ddata->indestructible) {
+                ddata->isQueuedForDeletion = true;
+                object->deleteLater();
+            }
+        }
+    }
+}
 
 QPair<QObject *, int> QV8QObjectWrapper::ExtractQtSignal(QV8Engine *engine, v8::Handle<v8::Object> object)
 {
diff --git a/src/qml/qml/v8/qv8qobjectwrapper_p.h b/src/qml/qml/v8/qv8qobjectwrapper_p.h
index f7b965690b..ab037eee14 100644
--- a/src/qml/qml/v8/qv8qobjectwrapper_p.h
+++ b/src/qml/qml/v8/qv8qobjectwrapper_p.h
@@ -61,6 +61,8 @@
 #include <private/qhashedstring_p.h>
 #include <private/qqmldata_p.h>
 #include <private/qqmlpropertycache_p.h>
+#include <private/qintrusivelist_p.h>
+#include "qv8objectresource_p.h"
 
 QT_BEGIN_NAMESPACE
 
@@ -71,6 +73,18 @@ class QV8ObjectResource;
 class QV8QObjectInstance;
 class QV8QObjectConnectionList;
 class QQmlPropertyCache;
+
+class QV8QObjectResource : public QV8ObjectResource
+{
+    V8_RESOURCE_TYPE(QObjectType);
+
+public:
+    QV8QObjectResource(QV8Engine *engine, QObject *object);
+
+    QQmlGuard<QObject> object;
+    QIntrusiveListNode weakResource;
+};
+
 class Q_QML_EXPORT QV8QObjectWrapper
 {
 public:
@@ -89,12 +103,23 @@ public:
     inline v8::Handle<v8::Value> getProperty(QObject *, const QHashedV8String &, RevisionMode);
     inline bool setProperty(QObject *, const QHashedV8String &, v8::Handle<v8::Value>, RevisionMode);
 
+    void registerWeakQObjectReference(QV8QObjectResource *resource)
+    {
+        m_javaScriptOwnedWeakQObjects.insert(resource);
+    }
+
+    void unregisterWeakQObjectReference(QV8QObjectResource *resource)
+    {
+        m_javaScriptOwnedWeakQObjects.remove(resource);
+    }
+
 private:
     friend class QQmlPropertyCache;
     friend class QV8QObjectConnectionList;
     friend class QV8QObjectInstance;
 
     v8::Local<v8::Object> newQObject(QObject *, QQmlData *, QV8Engine *);
+    void deleteWeakQObject(QV8QObjectResource *resource);
     static v8::Handle<v8::Value> GetProperty(QV8Engine *, QObject *, v8::Handle<v8::Value> *, 
                                              const QHashedV8String &, QV8QObjectWrapper::RevisionMode);
     static bool SetProperty(QV8Engine *, QObject *, const QHashedV8String &,
@@ -112,6 +137,7 @@ private:
     static v8::Handle<v8::Value> Invoke(const v8::Arguments &args);
     static QPair<QObject *, int> ExtractQtMethod(QV8Engine *, v8::Handle<v8::Function>);
     static QPair<QObject *, int> ExtractQtSignal(QV8Engine *, v8::Handle<v8::Object>);
+    static void WeakQObjectReferenceCallback(v8::Persistent<v8::Value> handle, void *wrapper);
 
     QV8Engine *m_engine;
     quint32 m_id;
@@ -126,6 +152,7 @@ private:
     QHash<QObject *, QV8QObjectConnectionList *> m_connections;
     typedef QHash<QObject *, QV8QObjectInstance *> TaintedHash;
     TaintedHash m_taintedObjects;
+    QIntrusiveList<QV8QObjectResource, &QV8QObjectResource::weakResource> m_javaScriptOwnedWeakQObjects;
 };
 
 v8::Handle<v8::Value> QV8QObjectWrapper::getProperty(QObject *object, const QHashedV8String &string,  
diff --git a/src/qml/qml/v8/v8.pri b/src/qml/qml/v8/v8.pri
index 52b6bf480a..33a0ad10a1 100644
--- a/src/qml/qml/v8/v8.pri
+++ b/src/qml/qml/v8/v8.pri
@@ -22,7 +22,8 @@ HEADERS += \
     $$PWD/qv8engine_impl_p.h \
     $$PWD/qv8domerrors_p.h \
     $$PWD/qv8sqlerrors_p.h \
-    $$PWD/qqmlbuiltinfunctions_p.h
+    $$PWD/qqmlbuiltinfunctions_p.h \
+    $$PWD/qv8objectresource_p.h
 
 SOURCES += \
     $$PWD/qv8stringwrapper.cpp \
diff --git a/tests/auto/qml/qjsengine/tst_qjsengine.cpp b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
index e34304b258..5ebc42a453 100644
--- a/tests/auto/qml/qjsengine/tst_qjsengine.cpp
+++ b/tests/auto/qml/qjsengine/tst_qjsengine.cpp
@@ -120,6 +120,7 @@ private slots:
 #endif
     void newQObject_promoteNonObject();
     void newQObject_promoteNonQScriptObject();
+    void newQObject_deletedEngine();
 #if 0 // ### FIXME: No QScript Metaobject support right now
     void newQMetaObject();
     void newActivationObject();
@@ -1120,6 +1121,19 @@ void tst_QJSEngine::newQObject_promoteNonQScriptObject()
 #endif
 }
 
+void tst_QJSEngine::newQObject_deletedEngine()
+{
+    QJSValue object;
+    QObject *ptr = new QObject();
+    QSignalSpy spy(ptr, SIGNAL(destroyed()));
+    {
+        QJSEngine engine;
+        object = engine.newQObject(ptr);
+        engine.globalObject().setProperty("obj", object);
+    }
+    QTRY_VERIFY(spy.count());
+}
+
 #if 0 // ### FIXME: No QScript Metaobject support right now
 QT_BEGIN_NAMESPACE
 Q_SCRIPT_DECLARE_QMETAOBJECT(QObject, QObject*)
diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index 4b913ef697..cb56948b6e 100644
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -4904,18 +4904,20 @@ void tst_qqmlecmascript::handleReferenceManagement()
         QQmlEngine::setObjectOwnership(second1, QQmlEngine::JavaScriptOwnership);
         QQmlEngine::setObjectOwnership(first2, QQmlEngine::JavaScriptOwnership);
         QQmlEngine::setObjectOwnership(second2, QQmlEngine::JavaScriptOwnership);
-        gc(engine);
-        QCOMPARE(dtorCount, 0);
-        delete hrmEngine2;
-        gc(engine);
+        gc(*hrmEngine1);
+        gc(*hrmEngine2);
         QCOMPARE(dtorCount, 0);
+        delete hrmEngine2; // should trigger deletion of objects with JS ownership tracked by this engine
+        gc(*hrmEngine1);
+        QCOMPARE(dtorCount, 2); // first2 and second2 should have been deleted.
         delete object1;
         delete object2;
-        hrmEngine1->collectGarbage();
+        gc(*hrmEngine1);
+        QCOMPARE(dtorCount, 6); // deleting object1 and object2 should trigger deletion of first1 and first2.
+        delete hrmEngine1;
         QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
         QCoreApplication::processEvents();
-        QCOMPARE(dtorCount, 6);
-        delete hrmEngine1;
+        QCOMPARE(dtorCount, 6); // all objects should have been cleaned up prior to deleting hrmEngine1.
     }
 }
 
diff --git a/tests/auto/qml/qqmlengine/tst_qqmlengine.cpp b/tests/auto/qml/qqmlengine/tst_qqmlengine.cpp
index c039429f48..ab18cdd050 100644
--- a/tests/auto/qml/qqmlengine/tst_qqmlengine.cpp
+++ b/tests/auto/qml/qqmlengine/tst_qqmlengine.cpp
@@ -46,6 +46,7 @@
 #include <QPointer>
 #include <QDir>
 #include <QStandardPaths>
+#include <QSignalSpy>
 #include <QDebug>
 #include <QQmlComponent>
 #include <QQmlNetworkAccessManagerFactory>
@@ -67,6 +68,13 @@ private slots:
     void outputWarningsToStandardError();
     void objectOwnership();
     void multipleEngines();
+
+public slots:
+    QObject *createAQObjectForOwnershipTest ()
+    {
+        static QObject *ptr = new QObject();
+        return ptr;
+    }
 };
 
 void tst_qqmlengine::rootContext()
@@ -326,7 +334,37 @@ void tst_qqmlengine::objectOwnership()
 
     delete o;
     }
-
+    {
+        QObject *ptr = createAQObjectForOwnershipTest();
+        QSignalSpy spy(ptr, SIGNAL(destroyed()));
+        {
+            QQmlEngine engine;
+            QQmlComponent c(&engine);
+            engine.rootContext()->setContextProperty("test", this);
+            QQmlEngine::setObjectOwnership(ptr, QQmlEngine::JavaScriptOwnership);
+            c.setData("import QtQuick 2.0; Item { property int data: test.createAQObjectForOwnershipTest() ? 0 : 1 }", QUrl());
+            QVERIFY(c.isReady());
+            QObject *o = c.create();
+            QVERIFY(o != 0);
+        }
+        QTRY_VERIFY(spy.count());
+    }
+    {
+        QObject *ptr = new QObject();
+        QSignalSpy spy(ptr, SIGNAL(destroyed()));
+        {
+            QQmlEngine engine;
+            QQmlComponent c(&engine);
+            engine.rootContext()->setContextProperty("test", ptr);
+            QQmlEngine::setObjectOwnership(ptr, QQmlEngine::JavaScriptOwnership);
+            c.setData("import QtQuick 2.0; QtObject { property var object: { var i = test; test ? 0 : 1 }  }", QUrl());
+            QVERIFY(c.isReady());
+            QObject *o = c.create();
+            QVERIFY(o != 0);
+            engine.rootContext()->setContextProperty("test", 0);
+        }
+        QTRY_VERIFY(spy.count());
+    }
 }
 
 // Test an object can be accessed by multiple engines