commit f98294088ced875aa2652461d123204a9aa2d538
author Brett Wilson <brettw@chromium.org> Mon Mar 19 23:36:09 2018 +0000
committer Commit Bot <commit-bot@chromium.org> Mon Mar 19 23:36:09 2018 +0000
tree 3620fbe607e1a3042937cf9281942752008def0c
parent 956d591f69eda7d5d422e1b9a04aef01641a8658

GN: Don't crash when mutating the list in a foreach.

Avoid a crash caused by mutation of the list being iterated over from inside
the foreach loop. This does a full copy of the the iterated list since the
code inside can't mutate the array via the loop variable anyway. Although
theoretically slower, this doesn't seem to have a measurable performance
regression in practice (we generally iterate over few large lists).

Adds documentation and tests for iteration while mutating the underlying list
variable.

Bug: 818525
Change-Id: I221fa230685b8998f5874154cad8d5c655b8006c
Reviewed-on: https://chromium-review.googlesource.com/959228
Commit-Queue: Brett Wilson <brettw@chromium.org>
Reviewed-by: Roland McGrath <mcgrathr@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#544209}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: dcba727e7e8b00cb26c4d797d956d48d2e98371f

tools/gn/function_foreach.cc

diff --git a/tools/gn/function_foreach.cc b/tools/gn/function_foreach.cc
index e94fbe6..b0af694 100644
--- a/tools/gn/function_foreach.cc
+++ b/tools/gn/function_foreach.cc
@@ -4,7 +4,6 @@
 
 #include "tools/gn/err.h"
 #include "tools/gn/functions.h"
-#include "tools/gn/parse_node_value_adapter.h"
 #include "tools/gn/parse_tree.h"
 #include "tools/gn/scope.h"
 
@@ -21,8 +20,9 @@
     }
 
   Executes the loop contents block over each item in the list, assigning the
-  loop_var to each item in sequence. The loop_var will be a copy so assigning
-  to it will not mutate the list.
+  loop_var to each item in sequence. The <loop_var> will be a copy so assigning
+  to it will not mutate the list. The loop will iterate over a copy of <list>
+  so mutating it inside the loop will not affect iteration.
 
   The block does not introduce a new scope, so that variable assignments inside
   the loop will be visible once the loop terminates.
@@ -65,11 +65,15 @@
   }
   base::StringPiece loop_var(identifier->value().value());
 
-  // Extract the list to iterate over.
-  ParseNodeValueAdapter list_adapter;
-  if (!list_adapter.InitForType(scope, args_vector[1].get(), Value::LIST, err))
+  // Extract the list to iterate over. Always copy in case the code changes
+  // the list variable inside the loop.
+  Value list_value = args_vector[1]->Execute(scope, err);
+  if (err->has_error())
     return Value();
-  const std::vector<Value>& list = list_adapter.get().list_value();
+  list_value.VerifyTypeIs(Value::Type::LIST, err);
+  if (err->has_error())
+    return Value();
+  const std::vector<Value>& list = list_value.list_value();
 
   // Block to execute.
   const BlockNode* block = function->block();

tools/gn/function_foreach_unittest.cc

diff --git a/tools/gn/function_foreach_unittest.cc b/tools/gn/function_foreach_unittest.cc
index 462d714..3886724 100644
--- a/tools/gn/function_foreach_unittest.cc
+++ b/tools/gn/function_foreach_unittest.cc
@@ -73,3 +73,28 @@
   EXPECT_TRUE(setup.scope()->CheckForUnusedVars(&err));
   EXPECT_FALSE(err.has_error());
 }
+
+// Checks that the list can be modified during iteration without crashing.
+TEST(FunctionForeach, ListModification) {
+  TestWithScope setup;
+  TestParseInput input_grow(
+      "a = [1, 2]\n"
+      "foreach(i, a) {\n"
+      "  print(i)\n"
+      "  if (i <= 8) {\n"
+      "    a += [ i + 2 ]\n"
+      "  }\n"
+      "}\n"
+      "print(a)");
+  ASSERT_FALSE(input_grow.has_error());
+
+  Err err;
+  input_grow.parsed()->Execute(setup.scope(), &err);
+  ASSERT_FALSE(err.has_error()) << err.message();
+
+  // The result of the loop should have been unaffected by the mutations of
+  // the list variable inside the loop, but the modifications made to it
+  // should have been persisted.
+  EXPECT_EQ("1\n2\n[1, 2, 3, 4]\n", setup.print_output());
+  setup.print_output().clear();
+}