Fix skydb --gdb to always have symbols

shell/dynamic_application_loader.cc

Index: shell/dynamic_application_loader.cc
diff --git a/shell/dynamic_application_loader.cc b/shell/dynamic_application_loader.cc
index 374484531eaba8ca9b5b7f9f5703bfdeeaf22309..632bf21f8adf2e4666eb36bb17616f9534860800 100644
--- a/shell/dynamic_application_loader.cc
+++ b/shell/dynamic_application_loader.cc
@@ -4,12 +4,14 @@
 
 #include "shell/dynamic_application_loader.h"
 
+#include <fstream>

[Aaron Boodman, 2015/01/23 21:56:40]

We don't typically use stl's stream support in Chromium. Not sure if it's a
style thing tho...

[eseidel, 2015/01/23 22:12:21]

Yeah, ComputeAppId is lifted directly from android/md5sum.cc which does use
fstream.  I'm happy to change it to something else?

[Aaron Boodman, 2015/01/23 22:29:00]

How about base::File::Read() and friends?

 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
+#include "base/md5.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
@@ -304,6 +306,55 @@ class DynamicApplicationLoader::NetworkLoader : public Loader {
       base::AppendToFile(map_path, map_entry.data(), map_entry.length());
   }
 
+  // Using MD5 because base/md5.h has an incremental API, any crypto would work.

[Aaron Boodman, 2015/01/23 21:56:41]

Yeah it would be better to use sha256 I think, if only to prevent developers
from having to think about what collision would mean. I'm surprised we don't
have other incremental hashes lying around, but if we don't, I think this can be
a TODO.

[eseidel, 2015/01/23 22:12:21]

Agreed.  I'll change it to say TODO, sadly md5 is our only incremental api.

+  // Derived from android/md5sum.cc
+  static bool ComputeAppId(const base::FilePath& path,
+                           std::string* digest_string) {
+    std::ifstream stream(path.value().data());
+    if (!stream.good()) {
+      LOG(ERROR) << "Failed to open " << path.value() << " for computing AppId";
+      return false;
+    }
+    base::MD5Context ctx;
+    base::MD5Init(&ctx);
+    char buf[1024];
+    while (stream.good()) {
+      std::streamsize bytes_read = stream.readsome(buf, sizeof(buf));
+      if (bytes_read == 0)
+        break;
+      base::MD5Update(&ctx, base::StringPiece(buf, bytes_read));
+    }
+    if (stream.fail()) {
+      LOG(ERROR) << "Error reading " << path.value();
+      return false;
+    }
+    base::MD5Digest digest;
+    base::MD5Final(&digest, &ctx);
+    *digest_string = base::MD5DigestToBase16(digest);
+    return true;
+  }
+
+  void RenameToAppId(base::Callback<void(const base::FilePath&, bool)> callback,
+                     bool copy_success) {
+    // The copy has completed move to a predictable location $TMP/$APP_ID.mojo.

[Aaron Boodman, 2015/01/23 21:56:40]

I don't think this comment is true .. we haven't moved to APP_ID yet, right?
That is below.

[eseidel, 2015/01/23 22:12:21]

Yeah, sorry I was trying to explain what the method was supposed to do.  There
should be a comma before 'move'.

+    if (copy_success) {

[Aaron Boodman, 2015/01/23 21:56:41]

If the copy failed, we're SOL. Should not do anymore work and send false to
other-callback.

[eseidel, 2015/01/23 22:12:21]

We already send false if the copy failed (by skipping the if block), I also
fixed it to send false if the move fails.

[Aaron Boodman, 2015/01/23 22:29:00]

I mean we should not call RecordCacheToURLMapping() or the line after that. If
|copy_success| is false, we should early return and call the next callback
immediately with false.

+      std::string app_id;
+      if (ComputeAppId(path_, &app_id)) {
+        base::FilePath temp_dir;
+        base::GetTempDir(&temp_dir);
+        std::string map_name = base::StringPrintf("%s.mojo", app_id.data());

[Aaron Boodman, 2015/01/23 21:56:40]

app_id.c_str()

[Aaron Boodman, 2015/01/23 21:56:40]

I think 'mapped_name' might be a better name for this variable (current name
makes me think it is referring to the map file itself).

[eseidel, 2015/01/23 22:12:21]

Done.

+        base::FilePath new_path = temp_dir.Append(map_name);
+        if (base::Move(path_, new_path))
+          path_ = new_path;
+      }
+    }
+    RecordCacheToURLMapping(path_, url_);
+
+    copy_success = copy_success && base::PathExists(path_);
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE, base::Bind(callback, path_, copy_success));
+  }
+
   void AsPath(
       base::TaskRunner* task_runner,
       base::Callback<void(const base::FilePath&, bool)> callback) override {
@@ -312,14 +363,12 @@ class DynamicApplicationLoader::NetworkLoader : public Loader {
           FROM_HERE, base::Bind(callback, path_, base::PathExists(path_)));
       return;
     }
+
     // We don't use the created file, just want the directory and random name.

[Aaron Boodman, 2015/01/23 21:56:41]

I think this comment is incorrect ... we do use the temp file now.

[eseidel, 2015/01/23 22:12:21]

Well, CreateTempFile creates and empty file and then immediately closes it.  We
overwrite the file by moving a new file over it, but I agree the comment isn't
very useful now.

[Aaron Boodman, 2015/01/23 22:29:00]

I see. Can you just delete it then.

     base::CreateTemporaryFile(&path_);
-    base::DeleteFile(path_, false);
-    path_ = path_.AddExtension(".mojo");  // Make libraries easy to spot.
     common::CopyToFile(response_->body.Pass(), path_, task_runner,
-                       base::Bind(callback, path_));
-
-    RecordCacheToURLMapping(path_, url_);
+                       base::Bind(&NetworkLoader::RenameToAppId,
+                                  base::Unretained(this), callback));

[Aaron Boodman, 2015/01/23 21:56:40]

Add a comment explaining why unretained is ok?

[eseidel, 2015/01/23 22:12:21]

I'm not sure unretained is OK.  If the copy for some reason takes longer than
DynamicApplicationLoader lives then I could imagine us crashing.  That would
only happen during an unexpected termination, but I could see it happening.  We
probably want a weak ptr to DynamicApplicationLoader instead.

[Aaron Boodman, 2015/01/23 22:29:00]

It actually looks to me like NetworkLoader cannot be deleted while this
operation is outstanding (modulo the entire shell shutting down), but
StartNetworkRequest is using a weakptr back to NetworkLoader, so I think you
should do the same here for consistency.

   }
 
   std::string MimeType() override {