Fail gracefully if profile Temp dir can not be accessed.

Fail gracefully if profile Temp dir can not be accessed.

BUG=60634, 67627
TEST=Manually interfere with Temp directory creation, see that expected failures happen.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71788

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

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:42: // Fall back to
system temp.  System temp is less than ideal,
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?

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:70:
UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackerCantGetTemp", i, 10);
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.

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:75:
UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackerCantCreateTemp", i, 10);
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?

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:100: // ReportError()
already called.
typo: ReportFailure

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
File chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:33:
file.open(WideToUTF8(filename.ToWStringHack()).c_str());
This looks problematic - potential filename incompatibility on different OSs. 
Instead, just use file_util::WriteFile.

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:41: if
(scoped_temp_dir.IsValid()) {
nit: remove {}

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:120:
//PathService::Override(chrome::DIR_USER_DATA_TEMP, temp_dir_);
remove unused code

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:189:
PrepareUnpackerEnv();
shouldn't this have EXPECT or ASSERT on it? (and other callsites)

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:239:
TEST_F(SandboxedExtensionUnpackerTest, CreateTempDirectory) {
If we ditch the multiple dir support for now, is this test still necessary?

[Sam Kerner (Chrome), 2011-01-18 19:26:44]

Removed the temp fallback code.  I would like to get your thoughts on the value
of falling back to another temp if the sandbox can't access the profile, but I
will leave it out of this CL in the interest of fixing the more urgent issue.

Added a histogram to let us see why this line is failing:

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

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:42: // Fall back to
system temp.  System temp is less than ideal,
On 2011/01/14 22:34:44, Erik Kay wrote:
> 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?

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.

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:70:
UMA_HISTOGRAM_ENUMERATION("Extensions.UnpackerCantGetTemp", i, 10);
On 2011/01/14 22:34:44, Erik Kay wrote:
> 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.

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.

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:100: // ReportError()
already called.
On 2011/01/14 22:34:44, Erik Kay wrote:
> typo: ReportFailure

Done.

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
File chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc (right):

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:33:
file.open(WideToUTF8(filename.ToWStringHack()).c_str());
On 2011/01/14 22:34:44, Erik Kay wrote:
> This looks problematic - potential filename incompatibility on different OSs. 
> Instead, just use file_util::WriteFile.

Done.

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:41: if
(scoped_temp_dir.IsValid()) {
On 2011/01/14 22:34:44, Erik Kay wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:120:
//PathService::Override(chrome::DIR_USER_DATA_TEMP, temp_dir_);
On 2011/01/14 22:34:44, Erik Kay wrote:
> remove unused code

Done.

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:189:
PrepareUnpackerEnv();
On 2011/01/14 22:34:44, Erik Kay wrote:
> shouldn't this have EXPECT or ASSERT on it? (and other callsites)

Yes.  Done.

http://codereview.chromium.org/6297003/diff/2001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker_unittest.cc:239:
TEST_F(SandboxedExtensionUnpackerTest, CreateTempDirectory) {
On 2011/01/14 22:34:44, Erik Kay wrote:
> If we ditch the multiple dir support for now, is this test still necessary?

No, but it could be used to test error handling of the code that sets up the
temp directory.

[Erik does not do reviews, 2011-01-18 21:36:34]

Yep.  Let's come back to mitigation once we have a bit more data.

http://codereview.chromium.org/6297003/diff/8001/chrome/browser/extensions/sa...
File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):

http://codereview.chromium.org/6297003/diff/8001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:52:
ReportFailure(l10n_util::GetStringFUTF8(
maybe we should also add a histogram straight into ReportFailure since it's a
central chokepoint

http://codereview.chromium.org/6297003/diff/8001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:74: // ReportFailure()
already called.
can this fit on the same line as the return?

http://codereview.chromium.org/6297003/diff/8001/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (left):

http://codereview.chromium.org/6297003/diff/8001/chrome/common/chrome_paths.c...
chrome/common/chrome_paths.cc:185: create_dir = true;
need to add a comment here or in the header so that people realize that this
doesn't ensure that the directory is created

http://codereview.chromium.org/6297003/diff/8001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/6297003/diff/8001/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:526: // has never had an
extension installed, or in a rare error.  Developers
"has never used the profile temp dir before" (I believe it's used in a few other
places now)

http://codereview.chromium.org/6297003/diff/8001/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:532: // Use histogram to see
why.
add TODO remove this code when fixed

[Sam Kerner (Chrome), 2011-01-19 05:01:24]

Ready for another look.

http://codereview.chromium.org/6297003/diff/8001/chrome/browser/extensions/sa...
File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):

http://codereview.chromium.org/6297003/diff/8001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:52:
ReportFailure(l10n_util::GetStringFUTF8(
On 2011/01/18 21:36:34, Erik Kay wrote:
> maybe we should also add a histogram straight into ReportFailure since it's a
> central chokepoint

Added a histogram to count failures.  Also added one to count success, so that
we can see the ratio.

I am not sure if you are suggesting that we count failures, or record the reason
in an enum.  Passing an enum of the failure type in at every call to
ReportFailure() is something I would prefer to do in a different CL.

http://codereview.chromium.org/6297003/diff/8001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:74: // ReportFailure()
already called.
On 2011/01/18 21:36:34, Erik Kay wrote:
> can this fit on the same line as the return?

Done.

http://codereview.chromium.org/6297003/diff/8001/chrome/common/chrome_paths.cc
File chrome/common/chrome_paths.cc (left):

http://codereview.chromium.org/6297003/diff/8001/chrome/common/chrome_paths.c...
chrome/common/chrome_paths.cc:185: create_dir = true;
On 2011/01/18 21:36:34, Erik Kay wrote:
> need to add a comment here or in the header so that people realize that this
> doesn't ensure that the directory is created

Done.

http://codereview.chromium.org/6297003/diff/8001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/6297003/diff/8001/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:526: // has never had an
extension installed, or in a rare error.  Developers
On 2011/01/18 21:36:34, Erik Kay wrote:
> "has never used the profile temp dir before" (I believe it's used in a few
other
> places now)

Done.

http://codereview.chromium.org/6297003/diff/8001/chrome/common/extensions/ext...
chrome/common/extensions/extension_file_util.cc:532: // Use histogram to see
why.
On 2011/01/18 21:36:34, Erik Kay wrote:
> add TODO remove this code when fixed

Done.

[Erik does not do reviews, 2011-01-19 05:14:16]

LGTM

http://codereview.chromium.org/6297003/diff/8001/chrome/browser/extensions/sa...
File chrome/browser/extensions/sandboxed_extension_unpacker.cc (right):

http://codereview.chromium.org/6297003/diff/8001/chrome/browser/extensions/sa...
chrome/browser/extensions/sandboxed_extension_unpacker.cc:52:
ReportFailure(l10n_util::GetStringFUTF8(
On 2011/01/19 05:01:24, Sam Kerner (Chrome) wrote:
> On 2011/01/18 21:36:34, Erik Kay wrote:
> > maybe we should also add a histogram straight into ReportFailure since it's
a
> > central chokepoint
> 
> Added a histogram to count failures.  Also added one to count success, so that
> we can see the ratio.
> 
> I am not sure if you are suggesting that we count failures, or record the
reason
> in an enum.  Passing an enum of the failure type in at every call to
> ReportFailure() is something I would prefer to do in a different CL.

yeah, an enum would be best, but I'm fine doing that in a separate CL.