commit 5e74245e3c6faf48195a046baa161b86a2750635
author Mostyn Bramley-Moore <mostynb@vewd.com> Sat Dec 09 14:20:27 2017 +0000
committer Commit Bot <commit-bot@chromium.org> Sat Dec 09 14:20:27 2017 +0000
tree 7ae65b5a0ae4433ef864c10121faa85e89104c6d
parent 126323f19c42d54d837af41fe337d9f12fa70fcf

GN: don't deadlock when encountering import loops

If there is an import loop, and a single thread processes an item in
that loop twice, then GN would previously deadlock when trying to lock
the file the second time (debug builds would catch the deadlock and
exit).

that release build gn gen catches this error and dies with a useful
error message.

Test: make two .gni files in the tree import each other, then check
Bug: 
Change-Id: I041206bb8b89c7b3656aa85ef19c2df02f275bd5
Reviewed-on: https://chromium-review.googlesource.com/805239
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Mostyn Bramley-Moore <mostynb@vewd.com>
Cr-Original-Commit-Position: refs/heads/master@{#522991}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 06ebd496239c53c43f3fd18c887a4534d9f723f0

tools/gn/import_manager.cc

diff --git a/tools/gn/import_manager.cc b/tools/gn/import_manager.cc
index 92e40ab..405eb01 100644
--- a/tools/gn/import_manager.cc
+++ b/tools/gn/import_manager.cc
@@ -74,6 +74,10 @@
                              const ParseNode* node_for_err,
                              Scope* scope,
                              Err* err) {
+  // Key for the current import on the current thread in imports_in_progress_.
+  std::string key =
+      std::to_string(base::PlatformThread::CurrentId()) + file.value();
+
   // See if we have a cached import, but be careful to actually do the scope
   // copying outside of the lock.
   ImportInfo* import_info = nullptr;
@@ -85,6 +89,12 @@
 
     // Promote the ImportInfo to outside of the imports lock.
     import_info = info_ptr.get();
+
+    if (imports_in_progress_.find(key) != imports_in_progress_.end()) {
+      *err = Err(Location(), file.value() + " is part of an import loop.");
+      return false;
+    }
+    imports_in_progress_.insert(key);
   }
 
   // Now use the per-import-file lock to block this thread if another thread
@@ -128,6 +138,12 @@
   Scope::MergeOptions options;
   options.skip_private_vars = true;
   options.mark_dest_used = true;  // Don't require all imported values be used.
+
+  {
+    base::AutoLock lock(imports_lock_);
+    imports_in_progress_.erase(key);
+  }
+
   return import_scope->NonRecursiveMergeTo(scope, options, node_for_err,
                                            "import", err);
 }

tools/gn/import_manager.h

diff --git a/tools/gn/import_manager.h b/tools/gn/import_manager.h
index f371520..01d8af6 100644
--- a/tools/gn/import_manager.h
+++ b/tools/gn/import_manager.h
@@ -7,6 +7,7 @@
 
 #include <map>
 #include <memory>
+#include <unordered_set>
 #include <vector>
 
 #include "base/macros.h"
@@ -36,13 +37,16 @@
  private:
   struct ImportInfo;
 
-  // Protects access to imports_. Do not hold when actually executing imports.
+  // Protects access to imports_ and imports_in_progress_. Do not hold when
+  // actually executing imports.
   base::Lock imports_lock_;
 
   // Owning pointers to the scopes.
   typedef std::map<SourceFile, std::unique_ptr<ImportInfo>> ImportMap;
   ImportMap imports_;
 
+  std::unordered_set<std::string> imports_in_progress_;
+
   DISALLOW_COPY_AND_ASSIGN(ImportManager);
 };