From c18e04b2e61f174a4883f6884cf9a0712c5725e3 Mon Sep 17 00:00:00 2001 From: Philip Lorenz Date: Sat, 1 Dec 2012 12:58:39 +0100 Subject: Only free context if the owning QV8ContextResource gets destroyed Since fdeee38b781376012c4f086276c3c376726c8839 QQmlXMLHttpRequest stores the calling context for later use. This leads to issues after the first request completes and the wrapping QV8ContextResource gets freed by garbage collection and therefore removes the associated QQmlDataContext which may still be required for later calls (e.g. if the calling context is part of a stateless library). This patch introduces an ownership flag for QV8ContextResource which indicates if the associated context should be cleared when the object is destroyed. Task-number: QTBUG-28351 Change-Id: I552ebb5c55b889eb33f3884283c8fdf037ac33be Reviewed-by: Alan Alpert --- src/qml/qml/qqmlvme.cpp | 1 + src/qml/qml/v8/qv8contextwrapper.cpp | 21 ++++++++---- src/qml/qml/v8/qv8contextwrapper_p.h | 5 ++- .../auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp | 40 ++++++++++++++++++++++ 4 files changed, 59 insertions(+), 8 deletions(-) diff --git a/src/qml/qml/qqmlvme.cpp b/src/qml/qml/qqmlvme.cpp index da918c3c71..9db3ee5a4d 100644 --- a/src/qml/qml/qqmlvme.cpp +++ b/src/qml/qml/qqmlvme.cpp @@ -1244,6 +1244,7 @@ v8::Persistent QQmlVME::run(QQmlContextData *parentCtxt, QQmlScriptD script->initialize(parentCtxt->engine); v8::Local qmlglobal = v8engine->qmlScope(ctxt, 0); + v8engine->contextWrapper()->takeContextOwnership(qmlglobal); if (!script->m_program.IsEmpty()) { script->m_program->Run(qmlglobal); diff --git a/src/qml/qml/v8/qv8contextwrapper.cpp b/src/qml/qml/v8/qv8contextwrapper.cpp index 9f18afc5cb..bc64189cbd 100644 --- a/src/qml/qml/v8/qv8contextwrapper.cpp +++ b/src/qml/qml/v8/qv8contextwrapper.cpp @@ -55,7 +55,7 @@ class QV8ContextResource : public QV8ObjectResource V8_RESOURCE_TYPE(ContextType); public: - QV8ContextResource(QV8Engine *engine, QQmlContextData *context, QObject *scopeObject); + QV8ContextResource(QV8Engine *engine, QQmlContextData *context, QObject *scopeObject, bool ownsContext = false); ~QV8ContextResource(); inline QQmlContextData *getContext() const; @@ -64,7 +64,8 @@ public: quint32 isSharedContext:1; quint32 hasSubContexts:1; quint32 readOnly:1; - quint32 dummy:29; + quint32 ownsContext:1; + quint32 dummy:28; // This is a pretty horrible hack, and an abuse of external strings. When we create a // sub-context (a context created by a Qt.include() in an external javascript file), @@ -86,15 +87,15 @@ private: }; -QV8ContextResource::QV8ContextResource(QV8Engine *engine, QQmlContextData *context, QObject *scopeObject) -: QV8ObjectResource(engine), isSharedContext(false), hasSubContexts(false), readOnly(true), - context(context), scopeObject(scopeObject) +QV8ContextResource::QV8ContextResource(QV8Engine *engine, QQmlContextData *context, QObject *scopeObject, bool ownsContext) +: QV8ObjectResource(engine), isSharedContext(false), hasSubContexts(false), readOnly(true), + ownsContext(ownsContext), context(context), scopeObject(scopeObject) { } QV8ContextResource::~QV8ContextResource() { - if (context && context->isJSContext) + if (context && ownsContext) context->destroy(); } @@ -186,7 +187,7 @@ v8::Local QV8ContextWrapper::urlScope(const QUrl &url) // XXX NewInstance() should be optimized v8::Local rv = m_urlConstructor->NewInstance(); - QV8ContextResource *r = new QV8ContextResource(m_engine, context, 0); + QV8ContextResource *r = new QV8ContextResource(m_engine, context, 0, true); rv->SetExternalResource(r); return rv; } @@ -226,6 +227,12 @@ QQmlContextData *QV8ContextWrapper::context(v8::Handle value) return r?r->getContext():0; } +void QV8ContextWrapper::takeContextOwnership(v8::Handle qmlglobal) +{ + QV8ContextResource *r = v8_resource_cast(qmlglobal); + r->ownsContext = true; +} + v8::Handle QV8ContextWrapper::NullGetter(v8::Local, const v8::AccessorInfo &) { diff --git a/src/qml/qml/v8/qv8contextwrapper_p.h b/src/qml/qml/v8/qv8contextwrapper_p.h index 1e62ea6480..43eeee05d3 100644 --- a/src/qml/qml/v8/qv8contextwrapper_p.h +++ b/src/qml/qml/v8/qv8contextwrapper_p.h @@ -54,6 +54,7 @@ // #include +#include #include QT_BEGIN_NAMESPACE @@ -62,7 +63,7 @@ class QUrl; class QObject; class QV8Engine; class QQmlContextData; -class QV8ContextWrapper +class Q_QML_PRIVATE_EXPORT QV8ContextWrapper { public: QV8ContextWrapper(); @@ -84,6 +85,8 @@ public: inline v8::Handle sharedContext() const; + void takeContextOwnership(v8::Handle qmlglobal); + private: static v8::Handle NullGetter(v8::Local property, const v8::AccessorInfo &info); diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp index a99c5fa8be..048fdb1e16 100644 --- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp +++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp @@ -288,6 +288,7 @@ private slots: private: static void propertyVarWeakRefCallback(v8::Persistent object, void* parameter); + static void verifyContextLifetime(QQmlContextData *ctxt); QQmlEngine engine; }; @@ -3783,6 +3784,41 @@ void tst_qqmlecmascript::singletonTypeResolution() delete object; } +void tst_qqmlecmascript::verifyContextLifetime(QQmlContextData *ctxt) { + QQmlContextData *childCtxt = ctxt->childContexts; + + if (!ctxt->importedScripts.isEmpty()) { + QV8Engine *engine = QV8Engine::get(ctxt->engine); + foreach (v8::Persistent qmlglobal, ctxt->importedScripts) { + QQmlContextData *scriptContext, *newContext; + + if (qmlglobal.IsEmpty()) + continue; + + scriptContext = engine->contextWrapper()->context(qmlglobal); + + { + v8::HandleScope handle_scope; + v8::Persistent context = v8::Context::New(); + v8::Context::Scope context_scope(context); + v8::Local temporaryScope = engine->qmlScope(scriptContext, NULL); + + context.Dispose(); + } + + QV8Engine::gc(); + newContext = engine->contextWrapper()->context(qmlglobal); + QVERIFY(scriptContext == newContext); + } + } + + while (childCtxt) { + verifyContextLifetime(childCtxt); + + childCtxt = childCtxt->nextChild; + } +} + void tst_qqmlecmascript::importScripts_data() { QTest::addColumn("testfile"); @@ -4014,6 +4050,10 @@ void tst_qqmlecmascript::importScripts() QVERIFY(object == 0); } else { QVERIFY(object != 0); + + QQmlContextData *ctxt = QQmlContextData::get(engine.rootContext()); + tst_qqmlecmascript::verifyContextLifetime(ctxt); + for (int i = 0; i < propertyNames.size(); ++i) QCOMPARE(object->property(propertyNames.at(i).toLatin1().constData()), propertyValues.at(i)); delete object; -- cgit v1.2.3