commit b79031308cc878488202beb99883ec1f2efd9a6d
author David 'Digit' Turner <digit@google.com> Wed Nov 24 19:12:30 2021 +0100
committer Commit Bot <commit-bot@chromium.org> Wed Nov 24 20:46:36 2021 +0000
tree 3890e502540d3171ef1e277de3cd2d2327db9679
parent de679ebabebe2e448dd4e9f288cad29cfe7b1a4b

Fix metadata collection bug.

A recent CL [1] introduced a subtle bug in metadata collection,
which is only triggered when collecting through a target that
doesn't have its own metadata (but whose dependencies do).

This CL fixes the issue, and adds a new unit-test to detect
a similar issue in the future.

The issue was that the `has_metadata()` check should not have
been introduced, because when this condition is false, it
prevented a call to WalkStep() which actually modifies
`next_walk_keys`. The effect was that metadata walk was cut
too soon.

[1] https://gn-review.googlesource.com/c/gn/+/12401

Bug: chromium/1273069

Change-Id: Iae1eee1e30b6130e93294204d282474233014525
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/12560
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: David Turner <digit@google.com>

src/gn/target.cc

diff --git a/src/gn/target.cc b/src/gn/target.cc
index 62f17cd..0d225b6 100644
--- a/src/gn/target.cc
+++ b/src/gn/target.cc
@@ -1223,8 +1223,11 @@
     // Origin is null because this isn't declared anywhere, and should never
     // trigger any errors.
     next_walk_keys.push_back(Value(nullptr, ""));
-  } else if (has_metadata()) {
+  } else {
     // Otherwise, we walk this target and collect the appropriate data.
+    // NOTE: Always call WalkStep() even when have_metadata() is false,
+    // because WalkStep() will append to 'next_walk_keys' in this case.
+    // See https://crbug.com/1273069.
     if (!metadata().WalkStep(settings()->build_settings(), keys_to_extract,
                              keys_to_walk, rebase_dir, &next_walk_keys,
                              &current_result, err))

src/gn/target_unittest.cc

diff --git a/src/gn/target_unittest.cc b/src/gn/target_unittest.cc
index add6d82..0dcff0c 100644
--- a/src/gn/target_unittest.cc
+++ b/src/gn/target_unittest.cc
@@ -1456,6 +1456,53 @@
   EXPECT_EQ(result, expected);
 }
 
+TEST(TargetTest, CollectMetadataWithRecurseHole) {
+  TestWithScope setup;
+
+  TestTarget one(setup, "//foo:one", Target::SOURCE_SET);
+  Value a_expected(nullptr, Value::LIST);
+  a_expected.list_value().push_back(Value(nullptr, "foo"));
+  one.metadata().contents().insert(
+      std::pair<std::string_view, Value>("a", a_expected));
+
+  Value b_expected(nullptr, Value::LIST);
+  b_expected.list_value().push_back(Value(nullptr, true));
+  one.metadata().contents().insert(
+      std::pair<std::string_view, Value>("b", b_expected));
+
+  // Target two does not have metadata but depends on three
+  // which does.
+  TestTarget two(setup, "//foo:two", Target::SOURCE_SET);
+
+  TestTarget three(setup, "//foo:three", Target::SOURCE_SET);
+  Value a_3_expected(nullptr, Value::LIST);
+  a_3_expected.list_value().push_back(Value(nullptr, "bar"));
+  three.metadata().contents().insert(
+      std::pair<std::string_view, Value>("a", a_3_expected));
+
+  one.public_deps().push_back(LabelTargetPair(&two));
+  two.public_deps().push_back(LabelTargetPair(&three));
+
+  std::vector<std::string> data_keys;
+  data_keys.push_back("a");
+  data_keys.push_back("b");
+
+  std::vector<std::string> walk_keys;
+
+  Err err;
+  std::vector<Value> result;
+  std::set<const Target*> targets;
+  one.GetMetadata(data_keys, walk_keys, SourceDir(), false, &result, &targets,
+                  &err);
+  EXPECT_FALSE(err.has_error());
+
+  std::vector<Value> expected;
+  expected.push_back(Value(nullptr, "bar"));
+  expected.push_back(Value(nullptr, "foo"));
+  expected.push_back(Value(nullptr, true));
+  EXPECT_EQ(result, expected);
+}
+
 TEST(TargetTest, CollectMetadataWithBarrier) {
   TestWithScope setup;