Fix calling Qt.binding() on bound functions

Calling Qt.binding() on a bound function object is a valid use
case and used to work until Qt 5.8.

The problem was that we optimized the code in QQmlBinding and
QQmlJavascriptExpression to directly work on a QV4::Function,
so this wouldn't work anymore.

To fix this make sure recursive calls to Function.bind() are
unrolled (so that the BoundFunction's target is never a bound
function itself), then add the bound function as an optional
member to the QQmlBinding and use it's bound arguments if
present.

Task-number: QTBUG-61927
Change-Id: I472214ddd82fc2a1212efd9b769861fc43d2ddaf
Reviewed-by: Simon Hausmann <simon.hausmann@qt.io>

7 files changed, 76 insertions, 26 deletions

diff --git a/src/qml/qml/qqmlbinding.cpp b/src/qml/qml/qqmlbinding.cpp
index c314833304..30a18440a8 100644
--- a/src/qml/qml/qqmlbinding.cpp
+++ b/src/qml/qml/qqmlbinding.cpp
@@ -51,6 +51,7 @@
 #include <private/qqmlvaluetypewrapper_p.h>
 #include <private/qv4qobjectwrapper_p.h>
 #include <private/qv4variantobject_p.h>
+#include <private/qv4jscall_p.h>
 
 #include <QVariant>
 #include <QtCore/qdebug.h>
@@ -97,6 +98,21 @@ QQmlBinding *QQmlBinding::create(const QQmlPropertyData *property, const QQmlScr
     return b;
 }
 
+QQmlSourceLocation QQmlBinding::sourceLocation() const
+{
+    if (m_sourceLocation)
+        return *m_sourceLocation;
+    return QQmlJavaScriptExpression::sourceLocation();
+}
+
+void QQmlBinding::setSourceLocation(const QQmlSourceLocation &location)
+{
+    if (m_sourceLocation)
+        delete m_sourceLocation;
+    m_sourceLocation = new QQmlSourceLocation(location);
+}
+
+
 QQmlBinding *QQmlBinding::create(const QQmlPropertyData *property, const QString &str, QObject *obj,
                                  QQmlContextData *ctxt, const QString &url, quint16 lineNumber)
 {
@@ -128,6 +144,7 @@ QQmlBinding *QQmlBinding::create(const QQmlPropertyData *property, QV4::Function
 
 QQmlBinding::~QQmlBinding()
 {
+    delete m_sourceLocation;
 }
 
 void QQmlBinding::setNotifyOnValueChanged(bool v)
@@ -171,6 +188,28 @@ void QQmlBinding::update(QQmlPropertyData::WriteFlags flags)
         setUpdatingFlag(false);
 }
 
+QV4::ReturnedValue QQmlBinding::evaluate(bool *isUndefined)
+{
+    QV4::ExecutionEngine *v4 = context()->engine->handle();
+    int argc = 0;
+    const QV4::Value *argv = nullptr;
+    const QV4::Value *thisObject = nullptr;
+    QV4::BoundFunction *b = nullptr;
+    if ((b = static_cast<QV4::BoundFunction *>(m_boundFunction.valueRef()))) {
+        QV4::Heap::MemberData *args = b->boundArgs();
+        if (args) {
+            argc = args->values.size;
+            argv = args->values.data();
+        }
+        thisObject = &b->d()->boundThis;
+    }
+    QV4::Scope scope(v4);
+    QV4::JSCallData jsCall(scope, argc, argv, thisObject);
+
+    return QQmlJavaScriptExpression::evaluate(jsCall.callData(), isUndefined);
+}
+
+
 // QQmlBindingBinding is for target properties which are of type "binding" (instead of, say, int or
 // double). The reason for being is that GenericBinding::fastWrite needs a compile-time constant
 // expression for the switch for the compiler to generate the optimal code, but
@@ -203,7 +242,7 @@ protected:
 
         bool isUndefined = false;
 
-        QV4::ScopedValue result(scope, QQmlJavaScriptExpression::evaluate(&isUndefined));
+        QV4::ScopedValue result(scope, evaluate(&isUndefined));
 
         bool error = false;
         if (!watcher.wasDeleted() && isAddedToObject() && !hasError())
@@ -302,9 +341,14 @@ public:
     {
         setCompilationUnit(compilationUnit);
         m_binding = binding;
-        setSourceLocation(QQmlSourceLocation(compilationUnit->fileName(), binding->valueLocation.line, binding->valueLocation.column));
     }
 
+    QQmlSourceLocation sourceLocation() const override final
+    {
+        return QQmlSourceLocation(m_compilationUnit->fileName(), m_binding->valueLocation.line, m_binding->valueLocation.column);
+    }
+
+
     void doUpdate(const DeleteWatcher &watcher,
                   QQmlPropertyData::WriteFlags flags, QV4::Scope &scope) override final
     {
diff --git a/src/qml/qml/qqmlbinding_p.h b/src/qml/qml/qqmlbinding_p.h
index b67b02975f..a1295bd0ac 100644
--- a/src/qml/qml/qqmlbinding_p.h
+++ b/src/qml/qml/qqmlbinding_p.h
@@ -104,6 +104,12 @@ public:
     QString expressionIdentifier() const override;
     void expressionChanged() override;
 
+    QQmlSourceLocation sourceLocation() const override;
+    void setSourceLocation(const QQmlSourceLocation &location);
+    void setBoundFunction(QV4::BoundFunction *boundFunction) {
+        m_boundFunction.set(boundFunction->engine(), *boundFunction);
+    }
+
     /**
      * This method returns a snapshot of the currently tracked dependencies of
      * this binding. The dependencies can change upon reevaluation. This method is
@@ -123,6 +129,8 @@ protected:
     bool slowWrite(const QQmlPropertyData &core, const QQmlPropertyData &valueTypeData,
                    const QV4::Value &result, bool isUndefined, QQmlPropertyData::WriteFlags flags);
 
+    QV4::ReturnedValue evaluate(bool *isUndefined);
+
 private:
     inline bool updatingFlag() const;
     inline void setUpdatingFlag(bool);
@@ -130,6 +138,9 @@ private:
     inline void setEnabledFlag(bool);
 
     static QQmlBinding *newBinding(QQmlEnginePrivate *engine, const QQmlPropertyData *property);
+
+    QQmlSourceLocation *m_sourceLocation = nullptr; // used for Qt.binding() created functions
+    QV4::PersistentValue m_boundFunction; // used for Qt.binding() that are created from a bound function object
 };
 
 bool QQmlBinding::updatingFlag() const
diff --git a/src/qml/qml/qqmljavascriptexpression.cpp b/src/qml/qml/qqmljavascriptexpression.cpp
index 40cf1417d0..93ec9421ed 100644
--- a/src/qml/qml/qqmljavascriptexpression.cpp
+++ b/src/qml/qml/qqmljavascriptexpression.cpp
@@ -97,8 +97,7 @@ QQmlJavaScriptExpression::QQmlJavaScriptExpression()
       m_context(nullptr),
       m_prevExpression(nullptr),
       m_nextExpression(nullptr),
-      m_v4Function(nullptr),
-      m_sourceLocation(nullptr)
+      m_v4Function(nullptr)
 {
 }
 
@@ -115,8 +114,6 @@ QQmlJavaScriptExpression::~QQmlJavaScriptExpression()
     clearError();
     if (m_scopeObject.isT2()) // notify DeleteWatcher of our deletion.
         m_scopeObject.asT2()->_s = nullptr;
-
-    delete m_sourceLocation;
 }
 
 void QQmlJavaScriptExpression::setNotifyOnValueChanged(bool v)
@@ -137,20 +134,11 @@ void QQmlJavaScriptExpression::resetNotifyOnValueChanged()
 
 QQmlSourceLocation QQmlJavaScriptExpression::sourceLocation() const
 {
-    if (m_sourceLocation)
-        return *m_sourceLocation;
     if (m_v4Function)
         return m_v4Function->sourceLocation();
     return QQmlSourceLocation();
 }
 
-void QQmlJavaScriptExpression::setSourceLocation(const QQmlSourceLocation &location)
-{
-    if (m_sourceLocation)
-        delete m_sourceLocation;
-    m_sourceLocation = new QQmlSourceLocation(location);
-}
-
 void QQmlJavaScriptExpression::setContext(QQmlContextData *context)
 {
     if (m_prevExpression) {
diff --git a/src/qml/qml/qqmljavascriptexpression_p.h b/src/qml/qml/qqmljavascriptexpression_p.h
index bff8866011..01af3b89ca 100644
--- a/src/qml/qml/qqmljavascriptexpression_p.h
+++ b/src/qml/qml/qqmljavascriptexpression_p.h
@@ -116,8 +116,7 @@ public:
     inline QObject *scopeObject() const;
     inline void setScopeObject(QObject *v);
 
-    QQmlSourceLocation sourceLocation() const;
-    void setSourceLocation(const QQmlSourceLocation &location);
+    virtual QQmlSourceLocation sourceLocation() const;
 
     bool isValid() const { return context() != nullptr; }
 
@@ -188,7 +187,6 @@ private:
     QV4::PersistentValue m_qmlScope;
     QQmlRefPointer<QV4::CompiledData::CompilationUnit> m_compilationUnit;
     QV4::Function *m_v4Function;
-    QQmlSourceLocation *m_sourceLocation; // used for Qt.binding() created functions
 };
 
 class QQmlPropertyCapture
diff --git a/src/qml/qml/qqmlvaluetypewrapper.cpp b/src/qml/qml/qqmlvaluetypewrapper.cpp
index a28115d192..6196a09d94 100644
--- a/src/qml/qml/qqmlvaluetypewrapper.cpp
+++ b/src/qml/qml/qqmlvaluetypewrapper.cpp
@@ -463,8 +463,12 @@ bool QQmlValueTypeWrapper::put(Managed *m, String *name, const Value &value)
 
             QV4::Scoped<QQmlBindingFunction> bindingFunction(scope, (const Value &)f);
 
-            QV4::ScopedContext ctx(scope, bindingFunction->scope());
-            QQmlBinding *newBinding = QQmlBinding::create(&cacheData, bindingFunction->function(), referenceObject, context, ctx);
+            QV4::ScopedFunctionObject f(scope, bindingFunction->bindingFunction());
+            QV4::ScopedContext ctx(scope, f->scope());
+            QQmlBinding *newBinding = QQmlBinding::create(&cacheData, f->function(), referenceObject, context, ctx);
+            newBinding->setSourceLocation(bindingFunction->currentLocation());
+            if (f->isBoundFunction())
+                newBinding->setBoundFunction(static_cast<QV4::BoundFunction *>(f.getPointer()));
             newBinding->setSourceLocation(bindingFunction->currentLocation());
             newBinding->setTarget(referenceObject, cacheData, pd);
             QQmlPropertyPrivate::setBinding(newBinding);
diff --git a/src/qml/qml/v8/qqmlbuiltinfunctions.cpp b/src/qml/qml/v8/qqmlbuiltinfunctions.cpp
index 1371f1f041..9e7c84011b 100644
--- a/src/qml/qml/v8/qqmlbuiltinfunctions.cpp
+++ b/src/qml/qml/v8/qqmlbuiltinfunctions.cpp
@@ -1344,11 +1344,12 @@ ReturnedValue QtObject::method_locale(const FunctionObject *b, const Value *, co
     return QQmlLocale::locale(scope.engine, code);
 }
 
-void Heap::QQmlBindingFunction::init(const QV4::FunctionObject *originalFunction)
+void Heap::QQmlBindingFunction::init(const QV4::FunctionObject *bindingFunction)
 {
-    Scope scope(originalFunction->engine());
-    ScopedContext context(scope, originalFunction->scope());
-    FunctionObject::init(context, originalFunction->function());
+    Scope scope(bindingFunction->engine());
+    ScopedContext context(scope, bindingFunction->scope());
+    FunctionObject::init(context, bindingFunction->function());
+    this->bindingFunction.set(internalClass->engine, bindingFunction->d());
 }
 
 QQmlSourceLocation QQmlBindingFunction::currentLocation() const
diff --git a/src/qml/qml/v8/qqmlbuiltinfunctions_p.h b/src/qml/qml/v8/qqmlbuiltinfunctions_p.h
index 104dae5d79..ee3b5f7d6e 100644
--- a/src/qml/qml/v8/qqmlbuiltinfunctions_p.h
+++ b/src/qml/qml/v8/qqmlbuiltinfunctions_p.h
@@ -80,8 +80,11 @@ struct ConsoleObject : Object {
     void init();
 };
 
-struct QQmlBindingFunction : FunctionObject {
-    void init(const QV4::FunctionObject *originalFunction);
+#define QQmlBindingFunctionMembers(class, Member) \
+    Member(class, Pointer, FunctionObject *, bindingFunction)
+DECLARE_HEAP_OBJECT(QQmlBindingFunction, FunctionObject) {
+    DECLARE_MARKOBJECTS(QQmlBindingFunction)
+    void init(const QV4::FunctionObject *bindingFunction);
 };
 
 }
@@ -179,6 +182,7 @@ struct QQmlBindingFunction : public QV4::FunctionObject
 {
     V4_OBJECT2(QQmlBindingFunction, FunctionObject)
 
+    Heap::FunctionObject *bindingFunction() const { return d()->bindingFunction; }
     QQmlSourceLocation currentLocation() const; // from caller stack trace
 };