Fix races when the same bits are downloaded from 2 URLs.

shell/application_manager/network_fetcher.cc

Index: shell/application_manager/network_fetcher.cc
diff --git a/shell/application_manager/network_fetcher.cc b/shell/application_manager/network_fetcher.cc
index 05aba7ab3605b5837b5acdb53e789b03f50c0246..ac55e1e8d342a973c3bf0c1e2ecb35d1bc451d52 100644
--- a/shell/application_manager/network_fetcher.cc
+++ b/shell/application_manager/network_fetcher.cc
@@ -5,6 +5,7 @@
 #include "shell/application_manager/network_fetcher.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
@@ -20,6 +21,7 @@
 #include "mojo/common/data_pipe_utils.h"
 #include "mojo/services/network/public/interfaces/network_service.mojom.h"
 #include "shell/application_manager/data_pipe_peek.h"
+#include "shell/switches.h"
 
 namespace mojo {
 namespace shell {
@@ -116,35 +118,49 @@ bool NetworkFetcher::ComputeAppId(const base::FilePath& path,
   return true;
 }
 
-bool NetworkFetcher::RenameToAppId(const base::FilePath& old_path,
+bool NetworkFetcher::RenameToAppId(const GURL& url,
+                                   const base::FilePath& old_path,
                                    base::FilePath* new_path) {
   std::string app_id;
   if (!ComputeAppId(old_path, &app_id))
     return false;
 
+  // Using a hash of the url as a directory to prevent a race when the same
+  // bytes are downloaded from 2 different urls.
+  std::string dirname = base::HexEncode(
+      crypto::SHA256HashString(url.spec()).data(), crypto::kSHA256Length);

[Nick Bray (chromium), 2015/03/19 17:45:20]

It's worth commenting this dirname will not be consistent for a particular
application because spec contains junk that is not considered part of a
canonical application name.  It's the leaf file name that needs to be consistent
for gdb serve. Comment / crosslink to a URL why we need this consistent naming,
too.

It also makes me wonder if there's a race where we have two sources trying to
connect to the same app at the same time.

[qsr, 2015/03/20 11:50:48]

done

+
   base::FilePath temp_dir;
   base::GetTempDir(&temp_dir);
+  base::FilePath app_dir = temp_dir.Append(dirname);
+  bool result = base::CreateDirectoryAndGetError(app_dir, nullptr);

[Nick Bray (chromium), 2015/03/19 17:45:20]

Comment on leaking the directory and why.

[qsr, 2015/03/20 11:50:49]

Done.

+  DCHECK(result);
   std::string unique_name = base::StringPrintf("%s.mojo", app_id.c_str());
-  *new_path = temp_dir.Append(unique_name);
+  *new_path = app_dir.Append(unique_name);
   return base::Move(old_path, *new_path);
 }
 
 void NetworkFetcher::CopyCompleted(
     base::Callback<void(const base::FilePath&, bool)> callback,
     bool success) {
-  // The copy completed, now move to $TMP/$APP_ID.mojo before the dlopen.
   if (success) {
-    success = false;
-    base::FilePath new_path;
-    if (RenameToAppId(path_, &new_path)) {
-      if (base::PathExists(new_path)) {
-        path_ = new_path;
-        success = true;
-        RecordCacheToURLMapping(path_, url_);
+    if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+            switches::kPredictableAppFilenames)) {
+      // The copy completed, now move to $TMP/$APP_ID.mojo before the dlopen.
+      success = false;
+      base::FilePath new_path;
+      if (RenameToAppId(url_, path_, &new_path)) {
+        if (base::PathExists(new_path)) {
+          path_ = new_path;
+          success = true;
+        }
       }
     }
   }
 
+  if (success)
+    RecordCacheToURLMapping(path_, url_);
+
   base::MessageLoop::current()->PostTask(FROM_HERE,
                                          base::Bind(callback, path_, success));
 }