Skip to content

Commit

Permalink
Fix crashes with closures created in QML components
Browse files Browse the repository at this point in the history 
When closures created inside QML components are called after the
surrounding component (and consequently QML context) has been destroyed,
we are in a somewhat limited environment. Initially we would just crash
as the calling QML context is not valid anymore. We can alleviate that
by introducing reference counting on the context and letting the QML
context wrapper keep a strong reference. This avoids the crashes and
also ensures that at least imports continue to be accessible within
these contexts (as the singleton test case demonstrates).

Task-number: QTBUG-61781
Change-Id: I893f171842d01b0863d95a02ea738adc2620e236
Reviewed-by: Lars Knoll <lars.knoll@qt.io>
  • Loading branch information
@laknoll @tronical
laknoll authored and tronical committed Sep 6, 2017
1 parent a88ca87 commit e22b624
Show file tree
Hide file tree
Showing 16 changed files with 257 additions and 106 deletions.
9 changes: 3 additions & 6 deletions src/qml/jsruntime/qv4qmlcontext.cpp
View file
Expand Up @@ -62,20 +62,17 @@ using namespace QV4;
DEFINE_OBJECT_VTABLE(QmlContextWrapper);
DEFINE_MANAGED_VTABLE(QmlContext);

void Heap::QmlContextWrapper::init(QQmlContextData *context, QObject *scopeObject, bool ownsContext)
void Heap::QmlContextWrapper::init(QQmlContextData *context, QObject *scopeObject)
{
Object::init();
readOnly = true;
this->ownsContext = ownsContext;
isNullWrapper = false;
this->context = new QQmlGuardedContextData(context);
this->context = new QQmlContextDataRef(context);
this->scopeObject.init(scopeObject);
}

void Heap::QmlContextWrapper::destroy()
{
if (*context && ownsContext)
(*context)->destroy();
delete context;
scopeObject.destroy();
Object::destroy();
Expand Down Expand Up @@ -321,7 +318,7 @@ Heap::QmlContext *QmlContext::createWorkerContext(ExecutionContext *parent, cons
context->isInternal = true;
context->isJSContext = true;

Scoped<QmlContextWrapper> qml(scope, scope.engine->memoryManager->allocObject<QmlContextWrapper>(context, (QObject*)0, true));
Scoped<QmlContextWrapper> qml(scope, scope.engine->memoryManager->allocObject<QmlContextWrapper>(context, (QObject*)0));
qml->d()->isNullWrapper = true;

qml->setReadOnly(false);
Expand Down
13 changes: 2 additions & 11 deletions src/qml/jsruntime/qv4qmlcontext_p.h
View file
Expand Up @@ -67,13 +67,12 @@ struct QmlContextWrapper;
namespace Heap {

struct QmlContextWrapper : Object {
void init(QQmlContextData *context, QObject *scopeObject, bool ownsContext = false);
void init(QQmlContextData *context, QObject *scopeObject);
void destroy();
bool readOnly;
bool ownsContext;
bool isNullWrapper;

QQmlGuardedContextData *context;
QQmlContextDataRef *context;
QQmlQPointer<QObject> scopeObject;
};

Expand All @@ -90,10 +89,6 @@ struct Q_QML_EXPORT QmlContextWrapper : Object
V4_OBJECT2(QmlContextWrapper, Object)
V4_NEEDS_DESTROY

void takeContextOwnership() {
d()->ownsContext = true;
}

inline QObject *getScopeObject() const { return d()->scopeObject; }
inline QQmlContextData *getContext() const { return *d()->context; }

Expand All @@ -116,10 +111,6 @@ struct Q_QML_EXPORT QmlContext : public ExecutionContext
QQmlContextData *qmlContext() const {
return *d()->qml->context;
}

void takeContextOwnership() {
d()->qml->ownsContext = true;
}
};

}
Expand Down
8 changes: 6 additions & 2 deletions src/qml/jsruntime/qv4qobjectwrapper.cpp
View file
Expand Up @@ -977,8 +977,12 @@ void QObjectWrapper::destroyObject(bool lastCall)
QQmlData *ddata = QQmlData::get(h->object(), false);
if (ddata) {
if (!h->object()->parent() && !ddata->indestructible) {
if (ddata && ddata->ownContext && ddata->context)
ddata->context->emitDestruction();
if (ddata && ddata->ownContext) {
Q_ASSERT(ddata->ownContext == ddata->context);
ddata->ownContext->emitDestruction();
ddata->ownContext = 0;
ddata->context = 0;
}
// This object is notionally destroyed now
ddata->isQueuedForDeletion = true;
if (lastCall)
Expand Down
59 changes: 41 additions & 18 deletions src/qml/qml/qqmlcontext.cpp
View file
Expand Up @@ -161,6 +161,7 @@ QQmlContext::QQmlContext(QQmlEngine *e, bool)
{
Q_D(QQmlContext);
d->data = new QQmlContextData(this);
++d->data->refCount;

d->data->engine = e;
}
Expand All @@ -174,6 +175,7 @@ QQmlContext::QQmlContext(QQmlEngine *engine, QObject *parent)
{
Q_D(QQmlContext);
d->data = new QQmlContextData(this);
++d->data->refCount;

d->data->setParent(engine?QQmlContextData::get(engine->rootContext()):0);
}
Expand All @@ -187,6 +189,7 @@ QQmlContext::QQmlContext(QQmlContext *parentContext, QObject *parent)
{
Q_D(QQmlContext);
d->data = new QQmlContextData(this);
++d->data->refCount;

d->data->setParent(parentContext?QQmlContextData::get(parentContext):0);
}
Expand All @@ -199,6 +202,7 @@ QQmlContext::QQmlContext(QQmlContextData *data)
{
Q_D(QQmlContext);
d->data = data;
// don't add a refcount here, as the data owns this context
}

/*!
Expand All @@ -212,7 +216,8 @@ QQmlContext::~QQmlContext()
{
Q_D(QQmlContext);

if (!d->data->isInternal)
d->data->publicContext = 0;
if (!--d->data->refCount)
d->data->destroy();
}

Expand Down Expand Up @@ -521,12 +526,12 @@ QQmlContextData::QQmlContextData()
}

QQmlContextData::QQmlContextData(QQmlContext *ctxt)
: parent(0), engine(0), isInternal(false), ownedByParent(false), isJSContext(false),
isPragmaLibraryContext(false), unresolvedNames(false), hasEmittedDestruction(false), isRootObjectInCreation(false),
publicContext(ctxt), incubator(0), componentObjectIndex(-1),
contextObject(0), childContexts(0), nextChild(0), prevChild(0),
expressions(0), contextObjects(0), contextGuards(0), idValues(0), idValueCount(0), linkedContext(0),
componentAttached(0)
: engine(0), isInternal(false), isJSContext(false),
isPragmaLibraryContext(false), unresolvedNames(false), hasEmittedDestruction(false), isRootObjectInCreation(false),
publicContext(ctxt), incubator(0), componentObjectIndex(-1),
contextObject(0), nextChild(0), prevChild(0),
expressions(0), contextObjects(0), idValues(0), idValueCount(0),
componentAttached(0)
{
}

Expand Down Expand Up @@ -563,11 +568,8 @@ void QQmlContextData::invalidate()
emitDestruction();

while (childContexts) {
if (childContexts->ownedByParent) {
childContexts->destroy();
} else {
childContexts->invalidate();
}
Q_ASSERT(childContexts != this);
childContexts->invalidate();
}

if (prevChild) {
Expand Down Expand Up @@ -601,12 +603,17 @@ void QQmlContextData::clearContext()

void QQmlContextData::destroy()
{
if (linkedContext)
linkedContext->destroy();
Q_ASSERT(refCount == 0);
linkedContext = 0;

if (engine) invalidate();
// avoid recursion
++refCount;
if (engine)
invalidate();

Q_ASSERT(refCount == 1);
clearContext();
Q_ASSERT(refCount == 1);

while (contextObjects) {
QQmlData *co = contextObjects;
Expand All @@ -617,6 +624,7 @@ void QQmlContextData::destroy()
co->nextContextObject = 0;
co->prevContextObject = 0;
}
Q_ASSERT(refCount == 1);

QQmlGuardedContextData *contextGuard = contextGuards;
while (contextGuard) {
Expand All @@ -627,25 +635,36 @@ void QQmlContextData::destroy()
contextGuard = next;
}
contextGuards = 0;
Q_ASSERT(refCount == 1);

delete [] idValues;
idValues = 0;

if (isInternal)
Q_ASSERT(refCount == 1);
if (publicContext) {
// the QQmlContext destructor will remove one ref again
++refCount;
delete publicContext;
}

Q_ASSERT(refCount == 1);
--refCount;
Q_ASSERT(refCount == 0);

delete this;
}

void QQmlContextData::setParent(QQmlContextData *p, bool parentTakesOwnership)
void QQmlContextData::setParent(QQmlContextData *p)
{
if (p == parent)
return;
if (p) {
parent = p;
engine = p->engine;
nextChild = p->childContexts;
if (nextChild) nextChild->prevChild = &nextChild;
prevChild = &p->childContexts;
p->childContexts = this;
ownedByParent = parentTakesOwnership;
}
}

Expand All @@ -660,6 +679,10 @@ void QQmlContextData::refreshExpressionsRecursive(QQmlJavaScriptExpression *expr
expression->refresh();
}

QQmlContextData::~QQmlContextData()
{
}

static inline bool expressions_to_run(QQmlContextData *ctxt, bool isGlobalRefresh)
{
return ctxt->expressions && (!isGlobalRefresh || ctxt->unresolvedNames);
Expand Down

0 comments on commit e22b624

Please sign in to comment.