Fail gracefully if profile Temp dir can not be accessed.

chrome/browser/extensions/sandboxed_extension_unpacker.cc

Index: chrome/browser/extensions/sandboxed_extension_unpacker.cc
diff --git a/chrome/browser/extensions/sandboxed_extension_unpacker.cc b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
index 9d7e45b7c1c98cfc20b7b70053c4ee6d0ba0e70b..b47d83f97a4eb908fae5c717824e2f3f9fffd8f5 100644
--- a/chrome/browser/extensions/sandboxed_extension_unpacker.cc
+++ b/chrome/browser/extensions/sandboxed_extension_unpacker.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -12,12 +12,15 @@
 #include "base/file_util.h"
 #include "base/file_util_proxy.h"
 #include "base/message_loop.h"
+#include "base/metrics/histogram.h"
+#include "base/path_service.h"
 #include "base/scoped_handle.h"
 #include "base/task.h"
 #include "base/utf_string_conversions.h"  // TODO(viettrungluu): delete me.
 #include "chrome/browser/browser_thread.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
+#include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
@@ -29,32 +32,74 @@
 #include "grit/generated_resources.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 
+namespace {
+
+const int kUnpackPathKeys[] = {
+  // First try unpacking in the temp directory inside the user's
+  // profile.
+  chrome::DIR_USER_DATA_TEMP,
+
+  // Fall back to system temp.  System temp is less than ideal,

[Erik does not do reviews, 2011/01/14 22:34:44]

I don't think this is a good idea.  Much of Chrome can't function properly if
the profile dir isn't writable.  I think the previous behavior of CHECKing is
closer to what we want (perhaps adding a friendly dialog).  Is there some
legitimate case that we want to support here?

[Sam Kerner (Chrome), 2011/01/18 19:26:45]

The fall-back to /tmp serves two purposes:

* Mitigate the problem while we figure out why we are getting crash reports on
the following line:

  CHECK(PathService::Get(chrome::DIR_USER_DATA_TEMP, &user_data_temp_dir));

* There have been several bugs involving the sandbox being unable to access the
unpack directory.  Given that history, it would not surprise me if there were
more.  Falling back to /tmp would have mitigated the problem for many users.  I
did not implement falling back on sandbox failure in this CL, but I think doing
so would be an improvement.

+  // because moving a directory across volumes is more likly to fail:
+  // crbug.com/37572 .  However, if we can't access a better unpack
+  // directory, this is better than nothing.
+  base::DIR_TEMP
+};
+
+}
+
 const char SandboxedExtensionUnpacker::kExtensionHeaderMagic[] = "Cr24";
 
 SandboxedExtensionUnpacker::SandboxedExtensionUnpacker(
     const FilePath& crx_path,
-    const FilePath& temp_path,
     ResourceDispatcherHost* rdh,
     SandboxedExtensionUnpackerClient* client)
-    : crx_path_(crx_path), temp_path_(temp_path),
+    : crx_path_(crx_path),
       thread_identifier_(BrowserThread::ID_COUNT),
       rdh_(rdh), client_(client), got_response_(false) {
 }
 
-void SandboxedExtensionUnpacker::Start() {
-  // We assume that we are started on the thread that the client wants us to do
-  // file IO on.
+bool SandboxedExtensionUnpacker::CreateTempDirectory(
+    const int unpack_path_keys[], size_t unpack_path_keys_size) {
   CHECK(BrowserThread::GetCurrentThreadIdentifier(&thread_identifier_));
 
-  // Create a temporary directory to work in.
-  if (!temp_dir_.CreateUniqueTempDirUnderPath(temp_path_)) {
-    // Could not create temporary directory.
+  FilePath temp_path;
+  bool found_path = false;
+  for (size_t i = 0; !found_path && i < unpack_path_keys_size; ++i) {
+    if (!PathService::Get(unpack_path_keys[i], &temp_path)) {
+      UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackerCantGetTemp", i, 10);

[Erik does not do reviews, 2011/01/14 22:34:44]

From what I can tell, the only way this can fail is if the path doesn't exist
and file_util::CreateDirectory fails to create it.  This follows exactly the
same logic that we use for a number of other directories.  If this is really
failing, we need to understand why rather than trying to work around it.

[Sam Kerner (Chrome), 2011/01/18 19:26:45]

I had some other theories as to why PathService::Get() could fail, and some of
them would be solved by moving the call to the file thread.  I expected to use
this histogram to see if the issue went away with this change.

After checking out the version that gives the crash reports and stepping thru
with the debugger, none of those theories are plausible.  So you are right that
these histograms will not tell us anything useful.  Removed.

+      continue;
+    }
+
+    if (!temp_dir_.CreateUniqueTempDirUnderPath(temp_path)) {
+      UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackerCantCreateTemp", i, 10);

[Erik does not do reviews, 2011/01/14 22:34:44]

simply knowing that this is failing might not be that helpful.  the real
question is why.  Maybe we can extract an error code and try an enumeration
instead?

+      continue;
+    }
+
+    found_path = true;
+  }
+
+  if (!found_path) {
+    // TODO(skerner): This should have its own string.
+    // Using an existing string so that the change can be merged.
     ReportFailure(l10n_util::GetStringFUTF8(
         IDS_EXTENSION_PACKAGE_INSTALL_ERROR,
         ASCIIToUTF16("COULD_NOT_CREATE_TEMP_DIRECTORY")));
-    return;
+    return false;
   }
 
+  return true;
+}
+
+void SandboxedExtensionUnpacker::Start() {
+  // We assume that we are started on the thread that the client wants us to do
+  // file IO on.
+  CHECK(BrowserThread::GetCurrentThreadIdentifier(&thread_identifier_));
+
+  if (!CreateTempDirectory(kUnpackPathKeys, arraysize(kUnpackPathKeys)))
+    // ReportError() already called.

[Erik does not do reviews, 2011/01/14 22:34:44]

typo: ReportFailure

[Sam Kerner (Chrome), 2011/01/18 19:26:45]

Done.

+    return;
+
   // Initialize the path that will eventually contain the unpacked extension.
   extension_root_ = temp_dir_.path().AppendASCII(
       extension_filenames::kTempExtensionName);