Fix XMLHttpRequest when used with QQmlEngine::evaluate

Our XHR implementation insists on a valid QQmlContext when processing
callbacks. This is to protect against callbacks being triggered after
dynamic QML contexts such as delegates have been destroyed.
Unfortunately those checks are too strict and make it impossible to use
XHR from within plain JS scripts (where v4->callingQmlContext() will
return a null pointer).

Dispatching the callbacks in functions that are directly called from
QML/JS is safe and something we can do unconditionally. This applies to
the callbacks triggered from abort() and open() for example.

When we're called from QNetworkAccessManager we should enforce the
continued existence of a QML context only if it was present at send()
time.

Task-number: QTBUG-67337
Change-Id: I8235f6ef407adc3eaeeff4eee72238ba6750afb2
Reviewed-by: Michael Brasser <michael.brasser@live.com>
Reviewed-by: Valery Kotov <vkotov@luxoft.com>
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

3 files changed, 69 insertions, 30 deletions

diff --git a/src/qml/qml/qqmlxmlhttprequest.cpp b/src/qml/qml/qqmlxmlhttprequest.cpp
index 5673acec89..567d83f3ee 100644
--- a/src/qml/qml/qqmlxmlhttprequest.cpp
+++ b/src/qml/qml/qqmlxmlhttprequest.cpp
@@ -1019,7 +1019,7 @@ public:
                  Opened = 1, HeadersReceived = 2,
                  Loading = 3, Done = 4 };
 
-    QQmlXMLHttpRequest(QNetworkAccessManager *manager);
+    QQmlXMLHttpRequest(QNetworkAccessManager *manager, QV4::ExecutionEngine *v4);
     virtual ~QQmlXMLHttpRequest();
 
     bool sendFlag() const;
@@ -1028,9 +1028,9 @@ public:
     int replyStatus() const;
     QString replyStatusText() const;
 
-    ReturnedValue open(Object *thisObject, QQmlContextData *context, const QString &, const QUrl &, LoadType);
+    ReturnedValue open(Object *thisObject, const QString &, const QUrl &, LoadType);
     ReturnedValue send(Object *thisObject, QQmlContextData *context, const QByteArray &);
-    ReturnedValue abort(Object *thisObject, QQmlContextData *context);
+    ReturnedValue abort(Object *thisObject);
 
     void addHeader(const QString &, const QString &);
     QString header(const QString &name) const;
@@ -1078,9 +1078,10 @@ private:
 
     PersistentValue m_thisObject;
     QQmlContextDataRef m_qmlContext;
+    bool m_wasConstructedWithQmlContext = true;
 
-    static void dispatchCallback(Object *thisObj, QQmlContextData *context);
-    void dispatchCallback();
+    static void dispatchCallbackNow(Object *thisObj);
+    void dispatchCallbackSafely();
 
     int m_status;
     QString m_statusText;
@@ -1096,12 +1097,13 @@ private:
     QV4::PersistentValue m_parsedDocument;
 };
 
-QQmlXMLHttpRequest::QQmlXMLHttpRequest(QNetworkAccessManager *manager)
+QQmlXMLHttpRequest::QQmlXMLHttpRequest(QNetworkAccessManager *manager, QV4::ExecutionEngine *v4)
     : m_state(Unsent), m_errorFlag(false), m_sendFlag(false)
     , m_redirectCount(0), m_gotXml(false), m_textCodec(nullptr), m_network(nullptr), m_nam(manager)
     , m_responseType()
     , m_parsedDocument()
 {
+    m_wasConstructedWithQmlContext = v4->callingQmlContext() != nullptr;
 }
 
 QQmlXMLHttpRequest::~QQmlXMLHttpRequest()
@@ -1134,7 +1136,7 @@ QString QQmlXMLHttpRequest::replyStatusText() const
     return m_statusText;
 }
 
-ReturnedValue QQmlXMLHttpRequest::open(Object *thisObject, QQmlContextData *context, const QString &method, const QUrl &url, LoadType loadType)
+ReturnedValue QQmlXMLHttpRequest::open(Object *thisObject, const QString &method, const QUrl &url, LoadType loadType)
 {
     destroyNetwork();
     m_sendFlag = false;
@@ -1145,7 +1147,7 @@ ReturnedValue QQmlXMLHttpRequest::open(Object *thisObject, QQmlContextData *cont
     m_request.setAttribute(QNetworkRequest::SynchronousRequestAttribute, loadType == SynchronousLoad);
     m_state = Opened;
     m_addedHeaders.clear();
-    dispatchCallback(thisObject, context);
+    dispatchCallbackNow(thisObject);
     return Encode::undefined();
 }
 
@@ -1297,7 +1299,7 @@ ReturnedValue QQmlXMLHttpRequest::send(Object *thisObject, QQmlContextData *cont
     return Encode::undefined();
 }
 
-ReturnedValue QQmlXMLHttpRequest::abort(Object *thisObject, QQmlContextData *context)
+ReturnedValue QQmlXMLHttpRequest::abort(Object *thisObject)
 {
     destroyNetwork();
     m_responseEntityBody = QByteArray();
@@ -1310,7 +1312,7 @@ ReturnedValue QQmlXMLHttpRequest::abort(Object *thisObject, QQmlContextData *con
 
         m_state = Done;
         m_sendFlag = false;
-        dispatchCallback(thisObject, context);
+        dispatchCallbackNow(thisObject);
     }
 
     m_state = Unsent;
@@ -1329,7 +1331,7 @@ void QQmlXMLHttpRequest::readyRead()
     if (m_state < HeadersReceived) {
         m_state = HeadersReceived;
         fillHeadersList ();
-        dispatchCallback();
+        dispatchCallbackSafely();
     }
 
     bool wasEmpty = m_responseEntityBody.isEmpty();
@@ -1337,7 +1339,7 @@ void QQmlXMLHttpRequest::readyRead()
     if (wasEmpty && !m_responseEntityBody.isEmpty())
         m_state = Loading;
 
-    dispatchCallback();
+    dispatchCallbackSafely();
 }
 
 static const char *errorToString(QNetworkReply::NetworkError error)
@@ -1380,14 +1382,14 @@ void QQmlXMLHttpRequest::error(QNetworkReply::NetworkError error)
         error == QNetworkReply::ServiceUnavailableError ||
         error == QNetworkReply::UnknownServerError) {
         m_state = Loading;
-        dispatchCallback();
+        dispatchCallbackSafely();
     } else {
         m_errorFlag = true;
         m_responseEntityBody = QByteArray();
     }
 
     m_state = Done;
-    dispatchCallback();
+    dispatchCallbackSafely();
 }
 
 #define XMLHTTPREQUEST_MAXIMUM_REDIRECT_RECURSION 15
@@ -1419,7 +1421,7 @@ void QQmlXMLHttpRequest::finished()
     if (m_state < HeadersReceived) {
         m_state = HeadersReceived;
         fillHeadersList ();
-        dispatchCallback();
+        dispatchCallbackSafely();
     }
     m_responseEntityBody.append(m_network->readAll());
     readEncoding();
@@ -1436,11 +1438,11 @@ void QQmlXMLHttpRequest::finished()
     destroyNetwork();
     if (m_state < Loading) {
         m_state = Loading;
-        dispatchCallback();
+        dispatchCallbackSafely();
     }
     m_state = Done;
 
-    dispatchCallback();
+    dispatchCallbackSafely();
 
     m_thisObject.clear();
     m_qmlContext.setContextData(nullptr);
@@ -1557,17 +1559,10 @@ const QByteArray &QQmlXMLHttpRequest::rawResponseBody() const
     return m_responseEntityBody;
 }
 
-void QQmlXMLHttpRequest::dispatchCallback(Object *thisObj, QQmlContextData *context)
+void QQmlXMLHttpRequest::dispatchCallbackNow(Object *thisObj)
 {
     Q_ASSERT(thisObj);
 
-    if (!context)
-        // if the calling context object is no longer valid, then it has been
-        // deleted explicitly (e.g., by a Loader deleting the itemContext when
-        // the source is changed).  We do nothing in this case, as the evaluation
-        // cannot succeed.
-        return;
-
     QV4::Scope scope(thisObj->engine());
     ScopedString s(scope, scope.engine->newString(QStringLiteral("onreadystatechange")));
     ScopedFunctionObject callback(scope, thisObj->get(s));
@@ -1585,9 +1580,16 @@ void QQmlXMLHttpRequest::dispatchCallback(Object *thisObj, QQmlContextData *cont
     }
 }
 
-void QQmlXMLHttpRequest::dispatchCallback()
+void QQmlXMLHttpRequest::dispatchCallbackSafely()
 {
-    dispatchCallback(m_thisObject.as<Object>(), m_qmlContext.contextData());
+    if (m_wasConstructedWithQmlContext && !m_qmlContext.contextData())
+        // if the calling context object is no longer valid, then it has been
+        // deleted explicitly (e.g., by a Loader deleting the itemContext when
+        // the source is changed).  We do nothing in this case, as the evaluation
+        // cannot succeed.
+        return;
+
+    dispatchCallbackNow(m_thisObject.as<Object>());
 }
 
 void QQmlXMLHttpRequest::destroyNetwork()
@@ -1640,7 +1642,7 @@ struct QQmlXMLHttpRequestCtor : public FunctionObject
         Scope scope(f->engine());
         const QQmlXMLHttpRequestCtor *ctor = static_cast<const QQmlXMLHttpRequestCtor *>(f);
 
-        QQmlXMLHttpRequest *r = new QQmlXMLHttpRequest(scope.engine->v8Engine->networkAccessManager());
+        QQmlXMLHttpRequest *r = new QQmlXMLHttpRequest(scope.engine->v8Engine->networkAccessManager(), scope.engine);
         Scoped<QQmlXMLHttpRequestWrapper> w(scope, scope.engine->memoryManager->allocObject<QQmlXMLHttpRequestWrapper>(r));
         ScopedObject proto(scope, ctor->d()->proto);
         w->setPrototype(proto);
@@ -1778,7 +1780,7 @@ ReturnedValue QQmlXMLHttpRequestCtor::method_open(const FunctionObject *b, const
     if (!username.isNull()) url.setUserName(username);
     if (!password.isNull()) url.setPassword(password);
 
-    return r->open(w, scope.engine->callingQmlContext(), method, url, async ? QQmlXMLHttpRequest::AsynchronousLoad : QQmlXMLHttpRequest::SynchronousLoad);
+    return r->open(w, method, url, async ? QQmlXMLHttpRequest::AsynchronousLoad : QQmlXMLHttpRequest::SynchronousLoad);
 }
 
 ReturnedValue QQmlXMLHttpRequestCtor::method_setRequestHeader(const FunctionObject *b, const Value *thisObject, const Value *argv, int argc)
@@ -1860,7 +1862,7 @@ ReturnedValue QQmlXMLHttpRequestCtor::method_abort(const FunctionObject *b, cons
         V4THROW_REFERENCE("Not an XMLHttpRequest object");
     QQmlXMLHttpRequest *r = w->d()->request;
 
-    return r->abort(w, scope.engine->callingQmlContext());
+    return r->abort(w);
 }
 
 ReturnedValue QQmlXMLHttpRequestCtor::method_getResponseHeader(const FunctionObject *b, const Value *thisObject, const Value *argv, int argc)
diff --git a/tests/auto/qml/qqmlxmlhttprequest/data/noqmlcontext.js b/tests/auto/qml/qqmlxmlhttprequest/data/noqmlcontext.js
new file mode 100644
index 0000000000..adb7269310
--- /dev/null
+++ b/tests/auto/qml/qqmlxmlhttprequest/data/noqmlcontext.js
@@ -0,0 +1,11 @@
+(function(url, resultCollector) {
+        var x = new XMLHttpRequest;
+        x.open("GET", url);
+        x.setRequestHeader("Accept-Language","en-US");
+        x.onreadystatechange = function() {
+            if (x.readyState == XMLHttpRequest.DONE) {
+                resultCollector.responseText = x.responseText
+            }
+        }
+        x.send()
+})
diff --git a/tests/auto/qml/qqmlxmlhttprequest/tst_qqmlxmlhttprequest.cpp b/tests/auto/qml/qqmlxmlhttprequest/tst_qqmlxmlhttprequest.cpp
index 59716acc0d..ecce6515ed 100644
--- a/tests/auto/qml/qqmlxmlhttprequest/tst_qqmlxmlhttprequest.cpp
+++ b/tests/auto/qml/qqmlxmlhttprequest/tst_qqmlxmlhttprequest.cpp
@@ -108,6 +108,8 @@ private slots:
     void text();
     void cdata();
 
+    void noQmlContext();
+
     // Crashes
     // void outstanding_request_at_shutdown();
 
@@ -1243,6 +1245,30 @@ void tst_qqmlxmlhttprequest::cdata()
     QCOMPARE(object->property("status").toInt(), 200);
 }
 
+void tst_qqmlxmlhttprequest::noQmlContext()
+{
+    TestHTTPServer server;
+    QVERIFY2(server.listen(), qPrintable(server.errorString()));
+    QVERIFY(server.wait(testFileUrl("open_network.expect"),
+                        testFileUrl("open_network.reply"),
+                        testFileUrl("testdocument.html")));
+    QUrl url = server.urlString(QStringLiteral("/testdocument.html"));
+
+    QQmlEngine engine;
+
+    QFile f(testFile("noqmlcontext.js"));
+    QVERIFY(f.open(QIODevice::ReadOnly));
+    QString script = QString::fromUtf8(f.readAll());
+    QJSValue testFunction = engine.evaluate(script);
+    QVERIFY(testFunction.isCallable());
+
+    QJSValue resultCollector = engine.newObject();
+
+    testFunction.call(QJSValueList() << url.toString() << resultCollector);
+
+    QTRY_COMPARE(resultCollector.property("responseText").toString(), "QML Rocks!\n");
+ }
+
 void tst_qqmlxmlhttprequest::stateChangeCallingContext()
 {
 #ifdef Q_OS_WIN