Skip to content

Commit

Permalink
V8: Remove extra V8::Context allocated for expressing strong references
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
Simon Hausmann authored and Qt by Nokia committed Dec 15, 2011
1 parent 9ae39c9 commit d70cca8
Show file tree
Hide file tree
Showing 6 changed files with 104 additions and 116 deletions.
7 changes: 4 additions & 3 deletions src/declarative/qml/qdeclarativevmemetaobject.cpp
View file
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion src/declarative/qml/qdeclarativevmemetaobject_p.h
View file
Expand Up @@ -179,7 +179,7 @@ class Q_AUTOTEST_EXPORT QDeclarativeVMEMetaObject : public QAbstractDynamicMetaO
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();

Expand Down
153 changes: 62 additions & 91 deletions src/declarative/qml/v8/qv8engine.cpp
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -177,6 +178,8 @@ QV8Engine::~QV8Engine()
invalidateAllValues();
clearExceptions();

qPersistentDispose(m_strongReferencer);

m_sequenceWrapper.destroy();
m_valueTypeWrapper.destroy();
m_variantWrapper.destroy();
Expand Down Expand Up @@ -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()
Expand All @@ -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()
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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
Expand All @@ -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);
}
}
Expand All @@ -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();
Expand All @@ -1606,8 +1579,6 @@ QV8Engine::ThreadData::ThreadData()

QV8Engine::ThreadData::~ThreadData()
{
delete referencer;
referencer = 0;
if (isolate) {
isolate->Exit();
isolate->Dispose();
Expand Down
24 changes: 8 additions & 16 deletions src/declarative/qml/v8/qv8engine_p.h
View file
Expand Up @@ -231,23 +231,9 @@ class Q_AUTOTEST_EXPORT QV8GCCallback
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();

Expand Down Expand Up @@ -451,11 +437,13 @@ class Q_DECLARATIVE_EXPORT QV8Engine

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;
};
Expand All @@ -464,6 +452,8 @@ class Q_DECLARATIVE_EXPORT QV8Engine
static ThreadData* threadData();
static void ensurePerThreadIsolate();

v8::Persistent<v8::Object> m_strongReferencer;

protected:
QJSEngine* q;
QDeclarativeEngine *m_engine;
Expand Down Expand Up @@ -503,6 +493,8 @@ class Q_DECLARATIVE_EXPORT QV8Engine
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;
Expand Down
24 changes: 19 additions & 5 deletions tests/auto/declarative/qdeclarativeecmascript/testtypes.h
View file
Expand Up @@ -1031,6 +1031,7 @@ class CircularReferenceObject : public QObject,
{
CircularReferenceObject *retn = new CircularReferenceObject(parent);
retn->m_dtorCount = m_dtorCount;
retn->m_engine = m_engine;
return retn;
}

Expand All @@ -1039,17 +1040,23 @@ class CircularReferenceObject : public QObject,
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*)

Expand All @@ -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);
}
Expand All @@ -1079,6 +1086,7 @@ class CircularReferenceHandle : public QObject,
{
CircularReferenceHandle *retn = new CircularReferenceHandle(parent);
retn->m_dtorCount = m_dtorCount;
retn->m_engine = m_engine;
return retn;
}

Expand All @@ -1095,15 +1103,21 @@ class CircularReferenceHandle : public QObject,
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*)

Expand Down

0 comments on commit d70cca8

Please sign in to comment.