Require commas between items in a GN list.

Fixing this issue exposed a race condition in .gni processing where an error parsing a file that another thread was waiting for wouldn't signal that other thread that the load was complete, so it would wait forever.

This patch separates out the loading from the signaling in the InputFileManager so we can't forget to do this.

BUG=368019
R=scottmg@chromium.org

Review URL: https://codereview.chromium.org/258073004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@266976 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 88 insertions, 55 deletions

diff --git a/net/BUILD.gn b/net/BUILD.gn
index c1077e3..41508a4 100644
--- a/net/BUILD.gn
+++ b/net/BUILD.gn
@@ -16,7 +16,7 @@ if (is_android) {
 # The list of net files is kept in net.gypi. Read it.
 gypi_values = exec_script(
     "//build/gypi_to_gn.py",
-    [ rebase_path("net.gypi") ]
+    [ rebase_path("net.gypi") ],
     "scope",
     [ "net.gypi" ])
 
diff --git a/skia/BUILD.gn b/skia/BUILD.gn
index f65b2cc..712a122 100644
--- a/skia/BUILD.gn
+++ b/skia/BUILD.gn
@@ -16,7 +16,7 @@ gypi_values = exec_script(
     "//build/gypi_to_gn.py",
     [ rebase_path("skia_gn_files.gypi"),
       "--replace=<(skia_include_path)=//third_party/skia/include",
-      "--replace=<(skia_src_path)=//third_party/skia/src" ]
+      "--replace=<(skia_src_path)=//third_party/skia/src" ],
     "scope",
     [ "skia_gn_files.gypi" ])
 
diff --git a/tools/gn/input_file_manager.cc b/tools/gn/input_file_manager.cc
index 04578b6..097474a 100644
--- a/tools/gn/input_file_manager.cc
+++ b/tools/gn/input_file_manager.cc
@@ -20,6 +20,61 @@ void InvokeFileLoadCallback(const InputFileManager::FileLoadCallback& cb,
   cb.Run(node);
 }
 
+bool DoLoadFile(const LocationRange& origin,
+                const BuildSettings* build_settings,
+                const SourceFile& name,
+                InputFile* file,
+                std::vector<Token>* tokens,
+                scoped_ptr<ParseNode>* root,
+                Err* err) {
+  // Do all of this stuff outside the lock. We should not give out file
+  // pointers until the read is complete.
+  if (g_scheduler->verbose_logging()) {
+    std::string logmsg = name.value();
+    if (origin.begin().file())
+      logmsg += " (referenced from " + origin.begin().Describe(false) + ")";
+    g_scheduler->Log("Loading", logmsg);
+  }
+
+  // Read.
+  base::FilePath primary_path = build_settings->GetFullPath(name);
+  ScopedTrace load_trace(TraceItem::TRACE_FILE_LOAD, name.value());
+  if (!file->Load(primary_path)) {
+    if (!build_settings->secondary_source_path().empty()) {
+      // Fall back to secondary source tree.
+      base::FilePath secondary_path =
+          build_settings->GetFullPathSecondary(name);
+      if (!file->Load(secondary_path)) {
+        *err = Err(origin, "Can't load input file.",
+                   "Unable to load either \n" +
+                   FilePathToUTF8(primary_path) + " or \n" +
+                   FilePathToUTF8(secondary_path));
+        return false;
+      }
+    } else {
+      *err = Err(origin,
+                 "Unable to load \"" + FilePathToUTF8(primary_path) + "\".");
+      return false;
+    }
+  }
+  load_trace.Done();
+
+  ScopedTrace exec_trace(TraceItem::TRACE_FILE_PARSE, name.value());
+
+  // Tokenize.
+  *tokens = Tokenizer::Tokenize(file, err);
+  if (err->has_error())
+    return false;
+
+  // Parse.
+  *root = Parser::Parse(*tokens, err);
+  if (err->has_error())
+    return false;
+
+  exec_trace.Done();
+  return true;
+}
+
 }  // namespace
 
 InputFileManager::InputFileData::InputFileData(const SourceFile& file_name)
@@ -211,53 +266,17 @@ bool InputFileManager::LoadFile(const LocationRange& origin,
                                 const SourceFile& name,
                                 InputFile* file,
                                 Err* err) {
-  // Do all of this stuff outside the lock. We should not give out file
-  // pointers until the read is complete.
-  if (g_scheduler->verbose_logging()) {
-    std::string logmsg = name.value();
-    if (origin.begin().file())
-      logmsg += " (referenced from " + origin.begin().Describe(false) + ")";
-    g_scheduler->Log("Loading", logmsg);
-  }
-
-  // Read.
-  base::FilePath primary_path = build_settings->GetFullPath(name);
-  ScopedTrace load_trace(TraceItem::TRACE_FILE_LOAD, name.value());
-  if (!file->Load(primary_path)) {
-    if (!build_settings->secondary_source_path().empty()) {
-      // Fall back to secondary source tree.
-      base::FilePath secondary_path =
-          build_settings->GetFullPathSecondary(name);
-      if (!file->Load(secondary_path)) {
-        *err = Err(origin, "Can't load input file.",
-                   "Unable to load either \n" +
-                   FilePathToUTF8(primary_path) + " or \n" +
-                   FilePathToUTF8(secondary_path));
-        return false;
-      }
-    } else {
-      *err = Err(origin,
-                 "Unable to load \"" + FilePathToUTF8(primary_path) + "\".");
-      return false;
-    }
-  }
-  load_trace.Done();
-
-  ScopedTrace exec_trace(TraceItem::TRACE_FILE_PARSE, name.value());
-
-  // Tokenize.
-  std::vector<Token> tokens = Tokenizer::Tokenize(file, err);
-  if (err->has_error())
-    return false;
-
-  // Parse.
-  scoped_ptr<ParseNode> root = Parser::Parse(tokens, err);
-  if (err->has_error())
-    return false;
+  std::vector<Token> tokens;
+  scoped_ptr<ParseNode> root;
+  bool success = DoLoadFile(origin, build_settings, name, file,
+                            &tokens, &root, err);
+  // Can't return early. We have to ensure that the completion event is
+  // signaled in all cases bacause another thread could be blocked on this one.
+
+  // Save this pointer for running the callbacks below, which happens after the
+  // scoped ptr ownership is taken away inside the lock.
   ParseNode* unowned_root = root.get();
 
-  exec_trace.Done();
-
   std::vector<FileLoadCallback> callbacks;
   {
     base::AutoLock lock(lock_);
@@ -265,8 +284,10 @@ bool InputFileManager::LoadFile(const LocationRange& origin,
 
     InputFileData* data = input_files_[name];
     data->loaded = true;
-    data->tokens.swap(tokens);
-    data->parsed_root = root.Pass();
+    if (success) {
+      data->tokens.swap(tokens);
+      data->parsed_root = root.Pass();
+    }
 
     // Unblock waiters on this event.
     //
@@ -288,7 +309,9 @@ bool InputFileManager::LoadFile(const LocationRange& origin,
   // Run pending invocations. Theoretically we could schedule each of these
   // separately to get some parallelism. But normally there will only be one
   // item in the list, so that's extra overhead and complexity for no gain.
-  for (size_t i = 0; i < callbacks.size(); i++)
-    callbacks[i].Run(unowned_root);
-  return true;
+  if (success) {
+    for (size_t i = 0; i < callbacks.size(); i++)
+      callbacks[i].Run(unowned_root);
+  }
+  return success;
 }
diff --git a/tools/gn/parser.cc b/tools/gn/parser.cc
index f533542..2d5e8fe 100644
--- a/tools/gn/parser.cc
+++ b/tools/gn/parser.cc
@@ -357,8 +357,17 @@ scoped_ptr<ListNode> Parser::ParseList(Token::Type stop_before,
   scoped_ptr<ListNode> list(new ListNode);
   list->set_begin_token(cur_token());
   bool just_got_comma = false;
+  bool first_time = true;
   while (!LookAhead(stop_before)) {
-    just_got_comma = false;
+    if (!first_time) {
+      if (!just_got_comma) {
+        // Require commas separate things in lists.
+        *err_ = Err(cur_token(), "Expected comma between items.");
+        return scoped_ptr<ListNode>();
+      }
+    }
+    first_time = false;
+
     // Why _OR? We're parsing things that are higher precedence than the ,
     // that separates the items of the list. , should appear lower than
     // boolean expressions (the lowest of which is OR), but above assignments.
diff --git a/tools/gn/parser_unittest.cc b/tools/gn/parser_unittest.cc
index adb8fdb..1205038 100644
--- a/tools/gn/parser_unittest.cc
+++ b/tools/gn/parser_unittest.cc
@@ -125,6 +125,7 @@ TEST(Parser, FunctionCall) {
       "  LITERAL(1)\n"
       "  LITERAL(2)\n");
   DoExpressionErrorTest("foo(1, 2,)", 1, 10);
+  DoExpressionErrorTest("foo(1 2)", 1, 7);
 }
 
 TEST(Parser, ParenExpression) {
diff --git a/tools/gn/secondary/tools/grit/grit_rule.gni b/tools/gn/secondary/tools/grit/grit_rule.gni
index 89c7b8f..789e40e 100644
--- a/tools/gn/secondary/tools/grit/grit_rule.gni
+++ b/tools/gn/secondary/tools/grit/grit_rule.gni
@@ -38,7 +38,7 @@ template("grit") {
     [ "--inputs", source_path, "-f", resource_ids] + grit_flags, "list lines")
   # The inputs are relative to the current (build) directory, rebase to
   # the current one.
-  grit_inputs = rebase_path(grit_inputs_build_rel, "." root_build_dir)
+  grit_inputs = rebase_path(grit_inputs_build_rel, ".", root_build_dir)
 
   grit_outputs_build_rel = exec_script(grit_info_script,
     [ "--outputs", "$output_dir", source_path, "-f", resource_ids ] +
diff --git a/ui/gfx/BUILD.gn b/ui/gfx/BUILD.gn
index 7833945..415913e 100644
--- a/ui/gfx/BUILD.gn
+++ b/ui/gfx/BUILD.gn
@@ -271,7 +271,7 @@ component("gfx") {
   if (is_android) {
     sources -= [
       "animation/throb_animation.cc",
-      "canvas_skia.cc"
+      "canvas_skia.cc",
       "display_observer.cc",
       "selection_model.cc",
     ]