Fix an issue where themes would sporadically fail to install.

Trying to send decoded images over IPC didn't work too well.  Instead, we'll
write them to a file and have the browser slurp them in from there.  My first
instinct was to use SharedMemory, but that would require us to impose a limit
on the size of the decoded image data.

Also made sure that the undecoded images are deleted when we install.

BUG=13455
TEST=Try the repro steps in bug 13455 several times and make sure it works
every time.
Review URL: http://codereview.chromium.org/119255

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@17797 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 120 insertions, 41 deletions

diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index bf38f97..0a495dfd 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -144,8 +144,8 @@ class ExtensionsServiceBackend::UnpackerClient
       // in a unit test and run the unpacker directly in-process.
       ExtensionUnpacker unpacker(temp_extension_path_);
       if (unpacker.Run()) {
-        OnUnpackExtensionSucceeded(*unpacker.parsed_manifest(),
-                                   unpacker.decoded_images());
+        OnUnpackExtensionSucceededImpl(*unpacker.parsed_manifest(),
+                                       unpacker.decoded_images());
       } else {
         OnUnpackExtensionFailed(unpacker.error_message());
       }
@@ -162,9 +162,19 @@ class ExtensionsServiceBackend::UnpackerClient
     OnUnpackExtensionFailed("Chrome crashed while trying to install.");
   }
 
-  virtual void OnUnpackExtensionSucceeded(
+  virtual void OnUnpackExtensionSucceeded(const DictionaryValue& manifest) {
+     ExtensionUnpacker::DecodedImages images;
+     if (!ExtensionUnpacker::ReadImagesFromFile(temp_extension_path_,
+                                                &images)) {
+       OnUnpackExtensionFailed("Couldn't read image data from disk.");
+     } else {
+       OnUnpackExtensionSucceededImpl(manifest, images);
+     }
+  }
+
+  void OnUnpackExtensionSucceededImpl(
       const DictionaryValue& manifest,
-      const std::vector< Tuple2<SkBitmap, FilePath> >& images) {
+      const ExtensionUnpacker::DecodedImages& images) {
     // The extension was unpacked to the temp dir inside our unpacking dir.
     FilePath extension_dir = temp_extension_path_.DirName().AppendASCII(
         ExtensionsServiceBackend::kTempExtensionName);
@@ -965,6 +975,25 @@ void ExtensionsServiceBackend::OnExtensionUnpacked(
     return;
   }
 
+  // Delete any images that may be used by the browser.  We're going to write
+  // out our own versions of the parsed images, and we want to make sure the
+  // originals are gone for good.
+  std::set<FilePath> image_paths = extension.GetBrowserImages();
+  if (image_paths.size() != images.size()) {
+    ReportExtensionInstallError(extension_path,
+        "Decoded images don't match what's in the manifest.");
+    return;
+  }
+
+  for (std::set<FilePath>::iterator it = image_paths.begin();
+       it != image_paths.end(); ++it) {
+    if (!file_util::Delete(temp_extension_dir.Append(*it), false)) {
+      ReportExtensionInstallError(extension_path,
+                                  "Error removing old image file.");
+      return;
+    }
+  }
+
   // Write our parsed images back to disk as well.
   for (size_t i = 0; i < images.size(); ++i) {
     const SkBitmap& image = images[i].a;
diff --git a/chrome/browser/utility_process_host.h b/chrome/browser/utility_process_host.h
index 0efaa56..03d98c8 100644
--- a/chrome/browser/utility_process_host.h
+++ b/chrome/browser/utility_process_host.h
@@ -6,7 +6,6 @@
 #define CHROME_BROWSER_UTILITY_PROCESS_HOST_H_
 
 #include <string>
-#include <vector>
 
 #include "base/basictypes.h"
 #include "base/ref_counted.h"
@@ -17,7 +16,6 @@
 class CommandLine;
 class DictionaryValue;
 class MessageLoop;
-class SkBitmap;
 
 // This class acts as the browser-side host to a utility child process.  A
 // utility process is a short-lived sandboxed process that is created to run
@@ -38,9 +36,7 @@ class UtilityProcessHost : public ChildProcessHost {
     // Called when the extension has unpacked successfully.  |manifest| is the
     // parsed manifest.json file.  |images| contains a list of decoded images
     // and the associated paths where those images live on disk.
-    virtual void OnUnpackExtensionSucceeded(
-        const DictionaryValue& manifest,
-        const std::vector< Tuple2<SkBitmap, FilePath> >& images) {}
+    virtual void OnUnpackExtensionSucceeded(const DictionaryValue& manifest) {}
 
     // Called when an error occurred while unpacking the extension.
     // |error_message| contains a description of the problem.
diff --git a/chrome/common/extensions/extension.cc b/chrome/common/extensions/extension.cc
index c641afc..ffed0d5 100644
--- a/chrome/common/extensions/extension.cc
+++ b/chrome/common/extensions/extension.cc
@@ -805,3 +805,25 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
 
   return true;
 }
+
+std::set<FilePath> Extension::GetBrowserImages() {
+  std::set<FilePath> image_paths;
+
+  DictionaryValue* theme_images = GetThemeImages();
+  if (theme_images) {
+    for (DictionaryValue::key_iterator it = theme_images->begin_keys();
+         it != theme_images->end_keys(); ++it) {
+      std::wstring val;
+      if (theme_images->GetString(*it, &val)) {
+        image_paths.insert(FilePath::FromWStringHack(val));
+      }
+    }
+  }
+
+  for (PageActionMap::const_iterator it = page_actions().begin();
+       it != page_actions().end(); ++it) {
+    image_paths.insert(it->second->icon_path());
+  }
+
+  return image_paths;
+}
diff --git a/chrome/common/extensions/extension.h b/chrome/common/extensions/extension.h
index 9b79f30..245d7b6 100644
--- a/chrome/common/extensions/extension.h
+++ b/chrome/common/extensions/extension.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_COMMON_EXTENSIONS_EXTENSION_H_
 #define CHROME_COMMON_EXTENSIONS_EXTENSION_H_
 
+#include <set>
 #include <string>
 #include <vector>
 
@@ -202,6 +203,10 @@ class Extension {
   }
   bool IsTheme() { return is_theme_; }
 
+  // Returns a list of paths (relative to the extension dir) for images that
+  // the browser might load (like themes and page action icons).
+  std::set<FilePath> GetBrowserImages();
+
  private:
   // Helper method that loads a UserScript object from a
   // dictionary in the content_script list of the manifest.
diff --git a/chrome/common/extensions/extension_unpacker.cc b/chrome/common/extensions/extension_unpacker.cc
index 916b048..bc7b25a 100644
--- a/chrome/common/extensions/extension_unpacker.cc
+++ b/chrome/common/extensions/extension_unpacker.cc
@@ -14,12 +14,13 @@
 #include "base/values.h"
 #include "net/base/file_stream.h"
 #include "chrome/common/extensions/extension.h"
+#include "chrome/common/ipc_message_utils.h"
 #include "chrome/common/json_value_serializer.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/common/zip.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "webkit/glue/image_decoder.h"
-#include "chrome/common/zip.h"
 
 namespace {
 const char kCurrentVersionFileName[] = "Current Version";
@@ -28,6 +29,9 @@ const char kCurrentVersionFileName[] = "Current Version";
 // validation before finalizing install.
 const char kTempExtensionName[] = "TEMP_INSTALL";
 
+// The file to write our decoded images to, relative to the extension_path.
+const char kDecodedImagesFilename[] = "DECODED_IMAGES";
+
 // Chromium Extension magic number
 const char kExtensionFileMagic[] = "Cr24";
 
@@ -262,27 +266,43 @@ bool ExtensionUnpacker::Run() {
   }
 
   // Decode any images that the browser needs to display.
-  DictionaryValue* images = extension.GetThemeImages();
-  if (images) {
-    for (DictionaryValue::key_iterator it = images->begin_keys();
-         it != images->end_keys(); ++it) {
-      std::wstring val;
-      if (images->GetString(*it, &val)) {
-        if (!AddDecodedImage(FilePath::FromWStringHack(val)))
-          return false;  // Error was already reported.
-      }
-    }
+  std::set<FilePath> image_paths = extension.GetBrowserImages();
+  for (std::set<FilePath>::iterator it = image_paths.begin();
+       it != image_paths.end(); ++it) {
+    if (!AddDecodedImage(*it))
+      return false;  // Error was already reported.
   }
 
-  for (PageActionMap::const_iterator it = extension.page_actions().begin();
-       it != extension.page_actions().end(); ++it) {
-    if (!AddDecodedImage(it->second->icon_path()))
-      return false;  // Error was already reported.
+  return true;
+}
+
+bool ExtensionUnpacker::DumpImagesToFile() {
+  IPC::Message pickle;  // We use a Message so we can use WriteParam.
+  IPC::WriteParam(&pickle, decoded_images_);
+
+  FilePath path = extension_path_.DirName().AppendASCII(kDecodedImagesFilename);
+  if (!file_util::WriteFile(path, static_cast<const char*>(pickle.data()),
+                            pickle.size())) {
+    SetError("Could not write image data to disk.");
+    return false;
   }
 
   return true;
 }
 
+// static
+bool ExtensionUnpacker::ReadImagesFromFile(const FilePath& extension_path,
+                                           DecodedImages* images) {
+  FilePath path = extension_path.DirName().AppendASCII(kDecodedImagesFilename);
+  std::string file_str;
+  if (!file_util::ReadFileToString(path, &file_str))
+    return false;
+
+  IPC::Message pickle(file_str.data(), file_str.size());
+  void* iter = NULL;
+  return IPC::ReadParam(&pickle, &iter, images);
+}
+
 bool ExtensionUnpacker::AddDecodedImage(const FilePath& path) {
   // Make sure it's not referencing a file outside the extension's subdir.
   if (path.IsAbsolute() || PathContainsParentDirectory(path)) {
diff --git a/chrome/common/extensions/extension_unpacker.h b/chrome/common/extensions/extension_unpacker.h
index 7e83363..65af750 100644
--- a/chrome/common/extensions/extension_unpacker.h
+++ b/chrome/common/extensions/extension_unpacker.h
@@ -15,9 +15,10 @@
 class DictionaryValue;
 class SkBitmap;
 
-// Implements IO for the ExtensionsService.
-// TODO(aa): Extract an interface out of this for testing the frontend, once the
-// frontend has significant logic to test.
+// This class unpacks an extension.  It is designed to be used in a sandboxed
+// child process.  We unpack and parse various bits of the extension, then
+// report back to the browser process, who then transcodes the pre-parsed bits
+// and writes them back out to disk for later use.
 class ExtensionUnpacker {
  public:
   typedef std::vector< Tuple2<SkBitmap, FilePath> > DecodedImages;
@@ -29,6 +30,17 @@ class ExtensionUnpacker {
   // Otherwise, error_message will contain a string explaining what went wrong.
   bool Run();
 
+  // Write the decoded images to kDecodedImagesFilename.  We do this instead
+  // of sending them over IPC, since they are so large.  Returns true on
+  // success.
+  bool DumpImagesToFile();
+
+  // Read the decoded images back from the file we saved them to.
+  // |extension_path| is the path to the extension we unpacked that wrote the
+  // data. Returns true on success.
+  static bool ReadImagesFromFile(const FilePath& extension_path,
+                                 DecodedImages* images);
+
   const std::string& error_message() { return error_message_; }
   DictionaryValue* parsed_manifest() {
     return parsed_manifest_.get();
diff --git a/chrome/common/render_messages.h b/chrome/common/render_messages.h
index 5719ce9..76341c5 100644
--- a/chrome/common/render_messages.h
+++ b/chrome/common/render_messages.h
@@ -392,10 +392,6 @@ struct ViewHostMsg_ShowPopup_Params {
   std::vector<WebMenuItem> popup_items;
 };
 
-// Used by UtilityHostMsg_UnpackExtension_Succeeded.  We must define a typedef
-// because the preprocessor is stupid about commas inside macros.
-typedef Tuple2<SkBitmap, FilePath> UnpackExtension_ImagePathPair;
-
 namespace IPC {
 
 template <>
diff --git a/chrome/common/render_messages_internal.h b/chrome/common/render_messages_internal.h
index 5677f16..655df6a 100644
--- a/chrome/common/render_messages_internal.h
+++ b/chrome/common/render_messages_internal.h
@@ -1417,12 +1417,11 @@ IPC_BEGIN_MESSAGES(ViewHost)
   // because we ran out of spare message types.
 
   // Reply when the utility process is done unpacking an extension.  |manifest|
-  // is the parsed manifest.json file.  |images| is a list of decoded images
-  // and the path to where they should be written to, relative to the
-  // manifest file.
-  IPC_MESSAGE_CONTROL2(UtilityHostMsg_UnpackExtension_Succeeded,
-                       DictionaryValue /* manifest */,
-                       std::vector<UnpackExtension_ImagePathPair> /* images */)
+  // is the parsed manifest.json file.  The unpacker should also have written
+  // out a file containing decoded images from the extension.  See
+  // ExtensionUnpacker for details.
+  IPC_MESSAGE_CONTROL1(UtilityHostMsg_UnpackExtension_Succeeded,
+                       DictionaryValue /* manifest */)
 
   // Reply when the utility process has failed while unpacking an extension.
   // |error_message| is a user-displayable explanation of what went wrong.
diff --git a/chrome/utility/utility_thread.cc b/chrome/utility/utility_thread.cc
index 2330fd5..d1d7b37 100644
--- a/chrome/utility/utility_thread.cc
+++ b/chrome/utility/utility_thread.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/utility/utility_thread.h"
 
+#include "base/file_util.h"
 #include "base/values.h"
 #include "chrome/common/child_process.h"
 #include "chrome/common/extensions/extension_unpacker.h"
@@ -33,12 +34,11 @@ void UtilityThread::OnControlMessageReceived(const IPC::Message& msg) {
 
 void UtilityThread::OnUnpackExtension(const FilePath& extension_path) {
   ExtensionUnpacker unpacker(extension_path);
-  if (unpacker.Run()) {
+  if (unpacker.Run() && unpacker.DumpImagesToFile()) {
     Send(new UtilityHostMsg_UnpackExtension_Succeeded(
-        *unpacker.parsed_manifest(), unpacker.decoded_images()));
+         *unpacker.parsed_manifest()));
   } else {
-    Send(new UtilityHostMsg_UnpackExtension_Failed(
-        unpacker.error_message()));
+    Send(new UtilityHostMsg_UnpackExtension_Failed(unpacker.error_message()));
   }
 
   ChildProcess::current()->ReleaseProcess();