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

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" ])
 

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" ])
 

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;
 }

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.

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) {

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 ] +

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",
     ]