V8: Remove extra V8::Context allocated for expressing strong references

Moved the Referencer code into QV8Engine and re-used the available v8
context there. That also makes things a bit cleaner in the sense that now
references from one object to another are guaranteed to be within the same
context. Previously some strong references would be across contexts that
do not actually share a security token.

Change-Id: I717b27a4d96323feb570023d4d84f2b2176d1a84
Reviewed-by: Aaron Kennedy <aaron.kennedy@nokia.com>
Reviewed-by: Chris Adams <christopher.adams@nokia.com>

diff --git a/src/declarative/qml/qdeclarativevmemetaobject.cpp b/src/declarative/qml/qdeclarativevmemetaobject.cpp
index 0ff6ad5..a37be92 100644 (file)
--- a/src/declarative/qml/qdeclarativevmemetaobject.cpp
+++ b/src/declarative/qml/qdeclarativevmemetaobject.cpp
@@ -1027,13 +1027,14 @@ void QDeclarativeVMEMetaObject::VarPropertiesWeakReferenceCallback(v8::Persisten
     vmemo->varProperties.Clear();
 }
 
-void QDeclarativeVMEMetaObject::GcPrologueCallback(QV8GCCallback::Referencer *r, QV8GCCallback::Node *node)
+void QDeclarativeVMEMetaObject::GcPrologueCallback(QV8GCCallback::Node *node)
 {
     QDeclarativeVMEMetaObject *vmemo = static_cast<QDeclarativeVMEMetaObject*>(node);
     Q_ASSERT(vmemo);
-    if (!vmemo->varPropertiesInitialized || vmemo->varProperties.IsEmpty())
+    if (!vmemo->varPropertiesInitialized || vmemo->varProperties.IsEmpty() || !vmemo->ctxt || !vmemo->ctxt->engine)
         return;
-    r->addRelationship(vmemo->object, vmemo->varProperties);
+    QDeclarativeEnginePrivate *ep = QDeclarativeEnginePrivate::get(vmemo->ctxt->engine);
+    ep->v8engine()->addRelationshipForGC(vmemo->object, vmemo->varProperties);
 }
 
 bool QDeclarativeVMEMetaObject::aliasTarget(int index, QObject **target, int *coreIndex, int *valueTypeIndex) const

diff --git a/src/declarative/qml/qdeclarativevmemetaobject_p.h b/src/declarative/qml/qdeclarativevmemetaobject_p.h
index 22cfbb1..2a4fd08 100644 (file)
--- a/src/declarative/qml/qdeclarativevmemetaobject_p.h
+++ b/src/declarative/qml/qdeclarativevmemetaobject_p.h
@@ -179,7 +179,7 @@ private:
     int firstVarPropertyIndex;
     bool varPropertiesInitialized;
     static void VarPropertiesWeakReferenceCallback(v8::Persistent<v8::Value> object, void* parameter);
-    static void GcPrologueCallback(QV8GCCallback::Referencer *r, QV8GCCallback::Node *node);
+    static void GcPrologueCallback(QV8GCCallback::Node *node);
     inline void allocateVarPropertiesArray();
     inline void ensureVarPropertiesAllocated();
 

diff --git a/src/declarative/qml/v8/qv8engine.cpp b/src/declarative/qml/v8/qv8engine.cpp
index 7c3dc1d..d89326a 100644 (file)
--- a/src/declarative/qml/v8/qv8engine.cpp
+++ b/src/declarative/qml/v8/qv8engine.cpp
@@ -141,6 +141,7 @@ QV8Engine::QV8Engine(QJSEngine* qq, QJSEngine::ContextOwnership ownership)
 
     v8::V8::SetUserObjectComparisonCallbackFunction(ObjectComparisonCallback);
     QV8GCCallback::registerGcPrologueCallback();
+    m_strongReferencer = qPersistentNew(v8::Object::New());
 
     m_stringWrapper.init();
     m_contextWrapper.init(this);
@@ -177,6 +178,8 @@ QV8Engine::~QV8Engine()
     invalidateAllValues();
     clearExceptions();
 
+    qPersistentDispose(m_strongReferencer);
+
     m_sequenceWrapper.destroy();
     m_valueTypeWrapper.destroy();
     m_variantWrapper.destroy();
@@ -751,6 +754,37 @@ double QV8Engine::qtDateTimeToJsDate(const QDateTime &dt)
     return QV8DateConverter::JSC::gregorianDateTimeToMS(tm, time.msec());
 }
 
+v8::Persistent<v8::Object> *QV8Engine::findOwnerAndStrength(QObject *object, bool *shouldBeStrong)
+{
+    QObject *parent = object->parent();
+    if (!parent) {
+        // if the object has JS ownership, the object's v8object owns the lifetime of the persistent value.
+        if (QDeclarativeEngine::objectOwnership(object) == QDeclarativeEngine::JavaScriptOwnership) {
+            *shouldBeStrong = false;
+            return &(QDeclarativeData::get(object)->v8object);
+        }
+
+        // no parent, and has CPP ownership - doesn't have an implicit parent.
+        *shouldBeStrong = true;
+        return 0;
+    }
+
+    // if it is owned by CPP, it's root parent may still be owned by JS.
+    // in that case, the owner of the persistent handle is the root parent's v8object.
+    while (parent->parent())
+        parent = parent->parent();
+
+    if (QDeclarativeEngine::objectOwnership(parent) == QDeclarativeEngine::JavaScriptOwnership) {
+        // root parent is owned by JS.  It's v8object owns the persistent value in question.
+        *shouldBeStrong = false;
+        return &(QDeclarativeData::get(parent)->v8object);
+    } else {
+        // root parent has CPP ownership.  The persistent value should not be made weak.
+        *shouldBeStrong = true;
+        return 0;
+    }
+}
+
 QDateTime QV8Engine::qtDateTimeFromJsDate(double jsDate)
 {
     // from QScriptEngine::MsToDateTime()
@@ -769,6 +803,32 @@ QDateTime QV8Engine::qtDateTimeFromJsDate(double jsDate)
     return convertedUTC.toLocalTime();
 }
 
+void QV8Engine::addRelationshipForGC(QObject *object, v8::Persistent<v8::Value> handle)
+{
+    if (handle.IsEmpty())
+        return;
+
+    bool handleShouldBeStrong = false;
+    v8::Persistent<v8::Object> *implicitOwner = findOwnerAndStrength(object, &handleShouldBeStrong);
+    if (handleShouldBeStrong) {
+        v8::V8::AddImplicitReferences(m_strongReferencer, &handle, 1);
+    } else if (!implicitOwner->IsEmpty()) {
+        v8::V8::AddImplicitReferences(*implicitOwner, &handle, 1);
+    }
+}
+
+void QV8Engine::addRelationshipForGC(QObject *object, QObject *other)
+{
+    bool handleShouldBeStrong = false;
+    v8::Persistent<v8::Object> *implicitOwner = findOwnerAndStrength(object, &handleShouldBeStrong);
+    v8::Persistent<v8::Value> handle = QDeclarativeData::get(other, true)->v8object;
+    if (handleShouldBeStrong) {
+        v8::V8::AddImplicitReferences(m_strongReferencer, &handle, 1);
+    } else if (!implicitOwner->IsEmpty()) {
+        v8::V8::AddImplicitReferences(*implicitOwner, &handle, 1);
+    }
+}
+
 static QThreadStorage<QV8Engine::ThreadData*> perThreadEngineData;
 
 bool QV8Engine::hasThreadData()
@@ -1456,8 +1516,6 @@ void QV8GCCallback::registerGcPrologueCallback()
     QV8Engine::ThreadData *td = QV8Engine::threadData();
     if (!td->gcPrologueCallbackRegistered) {
         td->gcPrologueCallbackRegistered = true;
-        if (!td->referencer)
-            td->referencer = new QV8GCCallback::Referencer;
         v8::V8::AddGCPrologueCallback(QV8GCCallback::garbageCollectorPrologueCallback, v8::kGCTypeMarkSweepCompact);
     }
 }
@@ -1472,89 +1530,6 @@ QV8GCCallback::Node::~Node()
     node.remove();
 }
 
-QV8GCCallback::Referencer::~Referencer()
-{
-    dispose();
-}
-
-QV8GCCallback::Referencer::Referencer()
-{
-    v8::HandleScope handleScope;
-    context = v8::Context::New();
-    qPersistentRegister(context);
-    v8::Context::Scope contextScope(context);
-    strongReferencer = qPersistentNew(v8::Object::New());
-}
-
-void QV8GCCallback::Referencer::addRelationship(QObject *object, QObject *other)
-{
-    bool handleShouldBeStrong = false;
-    v8::Persistent<v8::Object> *implicitOwner = findOwnerAndStrength(object, &handleShouldBeStrong);
-    v8::Persistent<v8::Value> handle = QDeclarativeData::get(other, true)->v8object;
-    if (handleShouldBeStrong) {
-        v8::V8::AddImplicitReferences(strongReferencer, &handle, 1);
-    } else if (!implicitOwner->IsEmpty()) {
-        v8::V8::AddImplicitReferences(*implicitOwner, &handle, 1);
-    }
-}
-
-void QV8GCCallback::Referencer::dispose()
-{
-    if (!strongReferencer.IsEmpty()) {
-        Q_ASSERT_X(v8::Isolate::GetCurrent(), "QV8GCCallback::Referencer::~Referencer()", "called after v8::Isolate has exited");
-        // automatically release the strongReferencer if it hasn't
-        // been explicitly released already.
-        qPersistentDispose(strongReferencer);
-    }
-    if (!context.IsEmpty())
-        qPersistentDispose(context);
-}
-
-void QV8GCCallback::Referencer::addRelationship(QObject *object, v8::Persistent<v8::Value> handle)
-{
-    if (handle.IsEmpty())
-        return;
-
-    bool handleShouldBeStrong = false;
-    v8::Persistent<v8::Object> *implicitOwner = findOwnerAndStrength(object, &handleShouldBeStrong);
-    if (handleShouldBeStrong) {
-        v8::V8::AddImplicitReferences(strongReferencer, &handle, 1);
-    } else if (!implicitOwner->IsEmpty()) {
-        v8::V8::AddImplicitReferences(*implicitOwner, &handle, 1);
-    }
-}
-
-v8::Persistent<v8::Object> *QV8GCCallback::Referencer::findOwnerAndStrength(QObject *object, bool *shouldBeStrong)
-{
-    QObject *parent = object->parent();
-    if (!parent) {
-        // if the object has JS ownership, the object's v8object owns the lifetime of the persistent value.
-        if (QDeclarativeEngine::objectOwnership(object) == QDeclarativeEngine::JavaScriptOwnership) {
-            *shouldBeStrong = false;
-            return &(QDeclarativeData::get(object)->v8object);
-        }
-
-        // no parent, and has CPP ownership - doesn't have an implicit parent.
-        *shouldBeStrong = true;
-        return 0;
-    }
-
-    // if it is owned by CPP, it's root parent may still be owned by JS.
-    // in that case, the owner of the persistent handle is the root parent's v8object.
-    while (parent->parent())
-        parent = parent->parent();
-
-    if (QDeclarativeEngine::objectOwnership(parent) == QDeclarativeEngine::JavaScriptOwnership) {
-        // root parent is owned by JS.  It's v8object owns the persistent value in question.
-        *shouldBeStrong = false;
-        return &(QDeclarativeData::get(parent)->v8object);
-    } else {
-        // root parent has CPP ownership.  The persistent value should not be made weak.
-        *shouldBeStrong = true;
-        return 0;
-    }
-}
-
 /*
    Ensure that each persistent handle is strong if it has CPP ownership
    and has no implicitly JS owned object owner in its parent chain, and
@@ -1574,14 +1549,13 @@ void QV8GCCallback::garbageCollectorPrologueCallback(v8::GCType, v8::GCCallbackF
         return;
 
     QV8Engine::ThreadData *td = QV8Engine::threadData();
-    Q_ASSERT(td->referencer);
     QV8GCCallback::Node *currNode = td->gcCallbackNodes.first();
 
     while (currNode) {
         // The client which adds itself to the list is responsible
         // for maintaining the correct implicit references in the
         // specified callback.
-        currNode->prologueCallback(td->referencer, currNode);
+        currNode->prologueCallback(currNode);
         currNode = td->gcCallbackNodes.next(currNode);
     }
 }
@@ -1593,8 +1567,7 @@ void QV8GCCallback::addGcCallbackNode(QV8GCCallback::Node *node)
 }
 
 QV8Engine::ThreadData::ThreadData()
-    : referencer(0)
-    , gcPrologueCallbackRegistered(false)
+    : gcPrologueCallbackRegistered(false)
 {
     if (!v8::Isolate::GetCurrent()) {
         isolate = v8::Isolate::New();
@@ -1606,8 +1579,6 @@ QV8Engine::ThreadData::ThreadData()
 
 QV8Engine::ThreadData::~ThreadData()
 {
-    delete referencer;
-    referencer = 0;
     if (isolate) {
         isolate->Exit();
         isolate->Dispose();

diff --git a/src/declarative/qml/v8/qv8engine_p.h b/src/declarative/qml/v8/qv8engine_p.h
index 4e6d3de..7564a69 100644 (file)
--- a/src/declarative/qml/v8/qv8engine_p.h
+++ b/src/declarative/qml/v8/qv8engine_p.h
@@ -231,23 +231,9 @@ public:
     static void garbageCollectorPrologueCallback(v8::GCType, v8::GCCallbackFlags);
     static void registerGcPrologueCallback();
 
-    class Q_AUTOTEST_EXPORT Referencer {
-    public:
-        ~Referencer();
-        void addRelationship(QObject *object, v8::Persistent<v8::Value> handle);
-        void addRelationship(QObject *object, QObject *other);
-        void dispose();
-    private:
-        Referencer();
-        static v8::Persistent<v8::Object> *findOwnerAndStrength(QObject *qobjectOwner, bool *shouldBeStrong);
-        v8::Persistent<v8::Object> strongReferencer;
-        v8::Persistent<v8::Context> context;
-        friend class QV8GCCallback;
-    };
-
     class Q_AUTOTEST_EXPORT Node {
     public:
-        typedef void (*PrologueCallback)(Referencer *r, Node *node);
+        typedef void (*PrologueCallback)(Node *node);
         Node(PrologueCallback callback);
         ~Node();
 
@@ -451,11 +437,13 @@ public:
 
     static QDateTime qtDateTimeFromJsDate(double jsDate);
 
+    void addRelationshipForGC(QObject *object, v8::Persistent<v8::Value> handle);
+    void addRelationshipForGC(QObject *object, QObject *other);
+
     struct ThreadData {
         ThreadData();
         ~ThreadData();
         v8::Isolate* isolate;
-        QV8GCCallback::Referencer* referencer;
         bool gcPrologueCallbackRegistered;
         QIntrusiveList<QV8GCCallback::Node, &QV8GCCallback::Node::node> gcCallbackNodes;
     };
@@ -464,6 +452,8 @@ public:
     static ThreadData* threadData();
     static void ensurePerThreadIsolate();
 
+    v8::Persistent<v8::Object> m_strongReferencer;
+
 protected:
     QJSEngine* q;
     QDeclarativeEngine *m_engine;
@@ -503,6 +493,8 @@ protected:
     double qtDateTimeToJsDate(const QDateTime &dt);
 
 private:
+    static v8::Persistent<v8::Object> *findOwnerAndStrength(QObject *object, bool *shouldBeStrong);
+
     typedef QScriptIntrusiveList<QJSValuePrivate, &QJSValuePrivate::m_node> ValueList;
     ValueList m_values;
     typedef QScriptIntrusiveList<QJSValueIteratorPrivate, &QJSValueIteratorPrivate::m_node> ValueIteratorList;

diff --git a/tests/auto/declarative/qdeclarativeecmascript/testtypes.h b/tests/auto/declarative/qdeclarativeecmascript/testtypes.h
index 2f1d3f9..0a5fc22 100644 (file)
--- a/tests/auto/declarative/qdeclarativeecmascript/testtypes.h
+++ b/tests/auto/declarative/qdeclarativeecmascript/testtypes.h
@@ -1031,6 +1031,7 @@ public:
     {
         CircularReferenceObject *retn = new CircularReferenceObject(parent);
         retn->m_dtorCount = m_dtorCount;
+        retn->m_engine = m_engine;
         return retn;
     }
 
@@ -1039,17 +1040,23 @@ public:
         m_referenced = other;
     }
 
-    static void callback(QV8GCCallback::Referencer *r, QV8GCCallback::Node *n)
+    static void callback(QV8GCCallback::Node *n)
     {
         CircularReferenceObject *cro = static_cast<CircularReferenceObject*>(n);
         if (cro->m_referenced) {
-            r->addRelationship(cro, cro->m_referenced);
+            cro->m_engine->addRelationshipForGC(cro, cro->m_referenced);
         }
     }
 
+    void setEngine(QDeclarativeEngine* declarativeEngine)
+    {
+        m_engine = QDeclarativeEnginePrivate::get(declarativeEngine)->v8engine();
+    }
+
 private:
     QObject *m_referenced;
     int *m_dtorCount;
+    QV8Engine* m_engine;
 };
 Q_DECLARE_METATYPE(CircularReferenceObject*)
 
@@ -1060,7 +1067,7 @@ class CircularReferenceHandle : public QObject,
 
 public:
     CircularReferenceHandle(QObject *parent = 0)
-        : QObject(parent), QV8GCCallback::Node(gccallback), m_dtorCount(0)
+        : QObject(parent), QV8GCCallback::Node(gccallback), m_dtorCount(0), m_engine(0)
     {
         QV8GCCallback::addGcCallbackNode(this);
     }
@@ -1079,6 +1086,7 @@ public:
     {
         CircularReferenceHandle *retn = new CircularReferenceHandle(parent);
         retn->m_dtorCount = m_dtorCount;
+        retn->m_engine = m_engine;
         return retn;
     }
 
@@ -1095,15 +1103,21 @@ public:
         crh->m_referenced.Clear();
     }
 
-    static void gccallback(QV8GCCallback::Referencer *r, QV8GCCallback::Node *n)
+    static void gccallback(QV8GCCallback::Node *n)
     {
         CircularReferenceHandle *crh = static_cast<CircularReferenceHandle*>(n);
-        r->addRelationship(crh, crh->m_referenced);
+        crh->m_engine->addRelationshipForGC(crh, crh->m_referenced);
+    }
+
+    void setEngine(QDeclarativeEngine* declarativeEngine)
+    {
+        m_engine = QDeclarativeEnginePrivate::get(declarativeEngine)->v8engine();
     }
 
 private:
     v8::Persistent<v8::Value> m_referenced;
     int *m_dtorCount;
+    QV8Engine* m_engine;
 };
 Q_DECLARE_METATYPE(CircularReferenceHandle*)
 

diff --git a/tests/auto/declarative/qdeclarativeecmascript/tst_qdeclarativeecmascript.cpp b/tests/auto/declarative/qdeclarativeecmascript/tst_qdeclarativeecmascript.cpp
index f3d810e..9bb763c 100644 (file)
--- a/tests/auto/declarative/qdeclarativeecmascript/tst_qdeclarativeecmascript.cpp
+++ b/tests/auto/declarative/qdeclarativeecmascript/tst_qdeclarativeecmascript.cpp
@@ -4129,6 +4129,7 @@ void tst_qdeclarativeecmascript::handleReferenceManagement()
         QObject *object = component.create();
         QVERIFY(object != 0);
         CircularReferenceObject *cro = object->findChild<CircularReferenceObject*>("cro");
+        cro->setEngine(&hrmEngine);
         cro->setDtorCount(&dtorCount);
         QMetaObject::invokeMethod(object, "createReference");
         gc(engine);
@@ -4147,6 +4148,7 @@ void tst_qdeclarativeecmascript::handleReferenceManagement()
         QObject *object = component.create();
         QVERIFY(object != 0);
         CircularReferenceObject *cro = object->findChild<CircularReferenceObject*>("cro");
+        cro->setEngine(&hrmEngine);
         cro->setDtorCount(&dtorCount);
         QMetaObject::invokeMethod(object, "circularReference");
         gc(engine);
@@ -4166,6 +4168,7 @@ void tst_qdeclarativeecmascript::handleReferenceManagement()
         QVERIFY(object != 0);
         CircularReferenceHandle *crh = object->findChild<CircularReferenceHandle*>("crh");
         QVERIFY(crh != 0);
+        crh->setEngine(&hrmEngine);
         crh->setDtorCount(&dtorCount);
         QMetaObject::invokeMethod(object, "createReference");
         CircularReferenceHandle *first = object->property("first").value<CircularReferenceHandle*>();
@@ -4193,6 +4196,7 @@ void tst_qdeclarativeecmascript::handleReferenceManagement()
         QVERIFY(object != 0);
         CircularReferenceHandle *crh = object->findChild<CircularReferenceHandle*>("crh");
         QVERIFY(crh != 0);
+        crh->setEngine(&hrmEngine);
         crh->setDtorCount(&dtorCount);
         QMetaObject::invokeMethod(object, "circularReference");
         CircularReferenceHandle *first = object->property("first").value<CircularReferenceHandle*>();
@@ -4229,6 +4233,8 @@ void tst_qdeclarativeecmascript::handleReferenceManagement()
         CircularReferenceHandle *crh2 = object2->findChild<CircularReferenceHandle*>("crh");
         QVERIFY(crh1 != 0);
         QVERIFY(crh2 != 0);
+        crh1->setEngine(&hrmEngine1);
+        crh2->setEngine(&hrmEngine2);
         crh1->setDtorCount(&dtorCount);
         crh2->setDtorCount(&dtorCount);
         QMetaObject::invokeMethod(object1, "createReference");
@@ -4271,6 +4277,8 @@ void tst_qdeclarativeecmascript::handleReferenceManagement()
         CircularReferenceHandle *crh2 = object2->findChild<CircularReferenceHandle*>("crh");
         QVERIFY(crh1 != 0);
         QVERIFY(crh2 != 0);
+        crh1->setEngine(&hrmEngine1);
+        crh2->setEngine(&hrmEngine2);
         crh1->setDtorCount(&dtorCount);
         crh2->setDtorCount(&dtorCount);
         QMetaObject::invokeMethod(object1, "createReference");
@@ -4322,6 +4330,8 @@ void tst_qdeclarativeecmascript::handleReferenceManagement()
         CircularReferenceHandle *crh2 = object2->findChild<CircularReferenceHandle*>("crh");
         QVERIFY(crh1 != 0);
         QVERIFY(crh2 != 0);
+        crh1->setEngine(hrmEngine1);
+        crh2->setEngine(hrmEngine2);
         crh1->setDtorCount(&dtorCount);
         crh2->setDtorCount(&dtorCount);
         QMetaObject::invokeMethod(object1, "createReference");