Fix string memory leak in JavaScript AST

Commit 02252ae08d introduced a QString member in a JS memory pool class,
which leaks unfortunately as the pool is not designed to call
destructors of allocated types. Typically strings in the AST are derived
from input and therefore a QStringRef is fine. The bindingIdentifier in
the PatterElement however is sometimes synthesized, so a separate
storage for dynamically allocated strings in the memory pool allows for
using QStringRef again.

Change-Id: I94d090df653d784c554452722b3b759031e4735b
Reviewed-by: Lars Knoll <lars.knoll@qt.io>

4 files changed, 23 insertions, 16 deletions

diff --git a/src/qml/parser/qqmljs.g b/src/qml/parser/qqmljs.g
index 7928881d8c..510baef352 100644
--- a/src/qml/parser/qqmljs.g
+++ b/src/qml/parser/qqmljs.g
@@ -1471,7 +1471,7 @@ CoverParenthesizedExpressionAndArrowParameterList: T_LPAREN T_RPAREN;
 CoverParenthesizedExpressionAndArrowParameterList: T_LPAREN BindingRestElement T_RPAREN;
 /.
     case $rule_number: {
-        AST::FormalParameterList *node = (new (pool) AST::FormalParameterList(nullptr, sym(2).PatternElement))->finish();
+        AST::FormalParameterList *node = (new (pool) AST::FormalParameterList(nullptr, sym(2).PatternElement))->finish(pool);
         sym(1).Node = node;
         coverExpressionErrorLocation = loc(2);
         coverExpressionType = CE_FormalParameterList;
@@ -1491,7 +1491,7 @@ CoverParenthesizedExpressionAndArrowParameterList: T_LPAREN Expression_In T_COMM
         }
         coverExpressionErrorLocation = loc(4);
         coverExpressionType = CE_FormalParameterList;
-        sym(1).Node = list->finish();
+        sym(1).Node = list->finish(pool);
     } break;
 ./
 
@@ -3551,7 +3551,7 @@ FormalParameters: ;
 FormalParameters: BindingRestElement;
 /.
     case $rule_number: {
-        AST::FormalParameterList *node = (new (pool) AST::FormalParameterList(nullptr, sym(1).PatternElement))->finish();
+        AST::FormalParameterList *node = (new (pool) AST::FormalParameterList(nullptr, sym(1).PatternElement))->finish(pool);
         sym(1).Node = node;
     } break;
 ./
@@ -3561,14 +3561,14 @@ FormalParameters: FormalParameterList;
 FormalParameters: FormalParameterList T_COMMA;
 /.
     case $rule_number: {
-        sym(1).Node = sym(1).FormalParameterList->finish();
+        sym(1).Node = sym(1).FormalParameterList->finish(pool);
     } break;
 ./
 
 FormalParameters: FormalParameterList T_COMMA BindingRestElement;
 /.
     case $rule_number: {
-        AST::FormalParameterList *node = (new (pool) AST::FormalParameterList(sym(1).FormalParameterList, sym(3).PatternElement))->finish();
+        AST::FormalParameterList *node = (new (pool) AST::FormalParameterList(sym(1).FormalParameterList, sym(3).PatternElement))->finish(pool);
         sym(1).Node = node;
     } break;
 ./
@@ -3646,7 +3646,7 @@ ArrowParameters: BindingIdentifier;
     case $rule_number: {
         AST::PatternElement *e = new (pool) AST::PatternElement(stringRef(1), nullptr, AST::PatternElement::Binding);
         e->identifierToken = loc(1);
-        sym(1).FormalParameterList = (new (pool) AST::FormalParameterList(nullptr, e))->finish();
+        sym(1).FormalParameterList = (new (pool) AST::FormalParameterList(nullptr, e))->finish(pool);
     } break;
 ./
 
@@ -3662,7 +3662,7 @@ ArrowParameters: CoverParenthesizedExpressionAndArrowParameterList;
                 syntaxError(loc(1), "Invalid Arrow parameter list.");
                 return false;
             }
-            sym(1).Node = list->finish();
+            sym(1).Node = list->finish(pool);
         }
     } break;
 ./
@@ -3744,7 +3744,7 @@ MethodDefinition: T_SET PropertyName T_LPAREN PropertySetParameterList T_RPAREN
 PropertySetParameterList: FormalParameter;
 /.
     case $rule_number: {
-        AST::FormalParameterList *node = (new (pool) AST::FormalParameterList(nullptr, sym(1).PatternElement))->finish();
+        AST::FormalParameterList *node = (new (pool) AST::FormalParameterList(nullptr, sym(1).PatternElement))->finish(pool);
         sym(1).Node = node;
     } break;
 ./
diff --git a/src/qml/parser/qqmljsast.cpp b/src/qml/parser/qqmljsast.cpp
index b338c5bbfe..1c997e1de7 100644
--- a/src/qml/parser/qqmljsast.cpp
+++ b/src/qml/parser/qqmljsast.cpp
@@ -422,7 +422,7 @@ bool PatternElement::convertLiteralToAssignmentPattern(MemoryPool *pool, SourceL
     }
 
     if (auto *i = cast<IdentifierExpression *>(lhs)) {
-        bindingIdentifier = i->name.toString();
+        bindingIdentifier = i->name;
         identifierToken = i->identifierToken;
         return true;
     }
@@ -959,7 +959,7 @@ QStringList FormalParameterList::formals() const
     int i = 0;
     for (const FormalParameterList *it = this; it; it = it->next) {
         if (it->element) {
-            QString name = it->element->bindingIdentifier;
+            QString name = it->element->bindingIdentifier.toString();
             int duplicateIndex = formals.indexOf(name);
             if (duplicateIndex >= 0) {
                 // change the name of the earlier argument to enforce the lookup semantics from the spec
@@ -993,7 +993,7 @@ void FormalParameterList::accept0(Visitor *visitor)
     visitor->endVisit(this);
 }
 
-FormalParameterList *FormalParameterList::finish()
+FormalParameterList *FormalParameterList::finish(QQmlJS::MemoryPool *pool)
 {
     FormalParameterList *front = next;
     next = nullptr;
@@ -1001,7 +1001,7 @@ FormalParameterList *FormalParameterList::finish()
     int i = 0;
     for (const FormalParameterList *it = this; it; it = it->next) {
         if (it->element && it->element->bindingIdentifier.isEmpty())
-            it->element->bindingIdentifier = QLatin1String("arg#") + QString::number(i);
+            it->element->bindingIdentifier = pool->newString(QLatin1String("arg#") + QString::number(i));
         ++i;
     }
     return front;
@@ -1220,7 +1220,7 @@ void PatternElement::boundNames(QStringList *names)
         else if (PatternPropertyList *p = propertyList())
             p->boundNames(names);
     } else {
-        names->append(bindingIdentifier);
+        names->append(bindingIdentifier.toString());
     }
 }
 
diff --git a/src/qml/parser/qqmljsast_p.h b/src/qml/parser/qqmljsast_p.h
index 66a4e357e9..67abbbf88a 100644
--- a/src/qml/parser/qqmljsast_p.h
+++ b/src/qml/parser/qqmljsast_p.h
@@ -671,7 +671,7 @@ public:
     { kind = K; }
 
     PatternElement(const QStringRef &n, ExpressionNode *i = nullptr, Type t = Binding)
-        : bindingIdentifier(n.toString()), initializer(i), type(t)
+        : bindingIdentifier(n), initializer(i), type(t)
     {
         Q_ASSERT(t >= RestElement);
         kind = K;
@@ -705,7 +705,7 @@ public:
 
 // attributes
     SourceLocation identifierToken;
-    QString bindingIdentifier;
+    QStringRef bindingIdentifier;
     ExpressionNode *bindingTarget = nullptr;
     ExpressionNode *initializer = nullptr;
     Type type = Literal;
@@ -2212,7 +2212,7 @@ public:
     SourceLocation lastSourceLocation() const override
     { return next ? next->lastSourceLocation() : element->lastSourceLocation(); }
 
-    FormalParameterList *finish();
+    FormalParameterList *finish(MemoryPool *pool);
 
 // attributes
     PatternElement *element = nullptr;
diff --git a/src/qml/parser/qqmljsmemorypool_p.h b/src/qml/parser/qqmljsmemorypool_p.h
index 58768fd805..9a480f1224 100644
--- a/src/qml/parser/qqmljsmemorypool_p.h
+++ b/src/qml/parser/qqmljsmemorypool_p.h
@@ -83,6 +83,7 @@ public:
 
             free(_blocks);
         }
+        qDeleteAll(strings);
     }
 
     inline void *allocate(size_t size)
@@ -104,6 +105,11 @@ public:
 
     template <typename Tp> Tp *New() { return new (this->allocate(sizeof(Tp))) Tp(); }
 
+    QStringRef newString(const QString &string) {
+        strings.append(new QString(string));
+        return QStringRef(strings.last());
+    }
+
 private:
     Q_NEVER_INLINE void *allocate_helper(size_t size)
     {
@@ -143,6 +149,7 @@ private:
     int _blockCount = -1;
     char *_ptr = nullptr;
     char *_end = nullptr;
+    QVector<QString*> strings;
 
     enum
     {