GN: Store parse error messages (so they aren't dropped due to races)

tools/gn/input_file_manager.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "tools/gn/input_file_manager.h"
 
 #include "base/bind.h"
 #include "base/stl_util.h"
 #include "tools/gn/filesystem_utils.h"
 #include "tools/gn/parser.h"
@@ skipping 197 matching lines @@
       {
         base::AutoUnlock unlock(lock_);
         data->completion_event->Wait();
       }
       // If there were multiple waiters on the same event, we now need to wake
       // up the next one.
       data->completion_event->Signal();
     }
   }
 
-  // The other load could have failed. In this case that error was probably
-  // printed to the console, but we need to return something here, so make up a
-  // dummy error.
-  //
-  // There is a race condition. The other load could have failed, but if the
-  // other thread is delayed for some reason, this thread could end up
-  // reporting the error to the scheduler first (since first error report
-  // wins). The user will see this one and the "real" one will be discarded.
+  // The other load could have failed. It is possible that this thread's error
+  // will be reported to the scheduler before the other thread's (and the first
+  // error reported "wins"). Forward the parse error from the other load for
+  // this thread so that the error message is useful.
   if (!data->parsed_root) {

[brettw, 2014/10/14 17:13:09]

Remove {}

[cjhopman, 2014/10/16 18:26:02]

Done.

-    *err = Err(origin, "File parse failed.",
-        "If you see this, I'm really sorry, but a race condition has caused\n"
-        "me to eat your error message. It was crunchy. If the parse error\n"
-        "in your imported file isn't obvious, try re-running GN.");
+    *err = *data->parse_error;
   }
   return data->parsed_root.get();
 }
 
 void InputFileManager::AddDynamicInput(const SourceFile& name,
                                        InputFile** file,
                                        std::vector<Token>** tokens,
                                        scoped_ptr<ParseNode>** parse_root) {
   InputFileData* data = new InputFileData(name);
   {
@@ skipping 48 matching lines @@
   std::vector<FileLoadCallback> callbacks;
   {
     base::AutoLock lock(lock_);
     DCHECK(input_files_.find(name) != input_files_.end());
 
     InputFileData* data = input_files_[name];
     data->loaded = true;
     if (success) {
       data->tokens.swap(tokens);
       data->parsed_root = root.Pass();
+    } else {
+      data->parse_error.reset(new Err(*err));
     }
 
     // Unblock waiters on this event.
     //
     // It's somewhat bad to signal this inside the lock. When it's used, it's
     // lazily created inside the lock. So we need to do the check and signal
     // inside the lock to avoid race conditions on the lazy creation of the
     // lock.
     //
     // We could avoid this by creating the lock every time, but the lock is
     // very seldom used and will generally be NULL, so my current theory is that
     // several signals of a completion event inside a lock is better than
     // creating about 1000 extra locks (one for each file).
     if (data->completion_event)
       data->completion_event->Signal();
 
     callbacks.swap(data->scheduled_callbacks);
   }
 
   // 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.
   if (success) {
     for (const auto& cb : callbacks)
       cb.Run(unowned_root);
   }
   return success;
 }