Fix skydb --gdb to always have symbols

shell/dynamic_application_loader.cc

 // Copyright 2014 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 "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"
 #include "base/process/process.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "mojo/common/common_type_converters.h"
 #include "mojo/common/data_pipe_utils.h"
 #include "mojo/public/cpp/system/data_pipe.h"
@@ skipping 252 matching lines @@
                                uint32_t skip) override {
     if (skip != 0) {
       MojoResult result = ReadDataRaw(
           response_->body.get(), nullptr, &skip,
           MOJO_READ_DATA_FLAG_ALL_OR_NONE | MOJO_READ_DATA_FLAG_DISCARD);
       DCHECK_EQ(result, MOJO_RESULT_OK);
     }
     return response_.Pass();
   }
 
   static void RecordCacheToURLMapping(const base::FilePath& path,

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

We do not need this anymore, right?

                                       const GURL& url) {
     // This is used to extract symbols on android.
     // TODO(eseidel): All users of this log should move to using the map file.
     LOG(INFO) << "Caching mojo app " << url << " at " << path.value();
 
     base::FilePath temp_dir;
     base::GetTempDir(&temp_dir);
     base::ProcessId pid = base::Process::Current().pid();
     std::string map_name = base::StringPrintf("mojo_shell.%d.maps", pid);
     base::FilePath map_path = temp_dir.Append(map_name);
 
     // TODO(eseidel): Paths or URLs with spaces will need quoting.
     std::string map_entry =
         base::StringPrintf("%s %s\n", path.value().data(), url.spec().data());
     // TODO(eseidel): AppendToFile is missing O_CREAT, crbug.com/450696
     if (!PathExists(map_path))
       base::WriteFile(map_path, map_entry.data(), map_entry.length());
     else
       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 {
     if (!path_.empty() || !response_) {
       base::MessageLoop::current()->PostTask(
           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 {
     DCHECK(response_);
     return response_->mime_type;
   }
 
   bool HasMojoMagic() override {
     std::string magic;
     return BlockingPeekNBytes(response_->body.get(), &magic, strlen(kMojoMagic),
@@ skipping 102 matching lines @@
   // TODO(darin): What should we do about service errors? This implies that
   // the app closed its handle to the service manager. Maybe we don't care?
 }
 
 void DynamicApplicationLoader::LoaderComplete(Loader* loader) {
   loaders_.erase(std::find(loaders_.begin(), loaders_.end(), loader));
 }
 
 }  // namespace shell
 }  // namespace mojo