Make TestingProfile delete failures explicitly CHECK-fail.

Make TestingProfile delete failures explicitly CHECK-fail.

TestingProfile currently crashes due to an IO-allowed assert failure if
the clean-up call to base::DeleteFile fails for some reason. This CL
explicitly CHECKs the result of Delete, rather than ignoring the result
and proceeding through to a difficult-to-debug assert later on.

BUG=717648

[Matt Giuca, 2017-05-03 05:11:25]

mgiuca@chromium.org changed reviewers:
+ jam@chromium.org

[Matt Giuca, 2017-05-03 05:11:25]

Hi John,

See bug for description. Note that there's an alternative CL:
https://codereview.chromium.org/2855123002. You should pick the one you like
best :)

[jam, 2017-05-03 16:02:12]

lgtm

Thanks for investigating this which is a side effect of my change to turn on IO
checks. I prefer this solution because the old behavior masked scoped temp
directories leaking after a test runs, which is something we want to fix. So
This change will make these tests fail, and will force us to have them clean up
properly.

[Matt Giuca, 2017-05-04 03:16:29]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2017-05-04 03:16:44]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Matt Giuca, 2017-05-04 03:17:26]

Great.

Thanks for playing "Choose Your Own CL".

[commit-bot: I haz the power, 2017-05-04 03:38:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 03:38:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2017-05-05 00:19:33]

On 2017/05/04 03:38:57, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Welp looks like this broke a lot of tests. I will investigate.

[Matt Giuca, 2017-05-05 03:25:26]

Had do add a check for IsValid(); otherwise Delete() returns false in the
legitimate case that temp_dir_ was never initialized.

This changes the behaviour such that Delete() itself is not called if either the
path is empty (no effect), or if the directory doesn't exist -- it means we now
won't CHECK-fail on a case where we try to delete an empty directory, but I
think that's not a problem (and writing logic to detect that case is complex).

[Matt Giuca, 2017-05-05 03:25:30]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2017-05-05 03:25:31]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2854323002/#ps20001
(title: "Only call delete if valid (i.e., path not empty).")

[commit-bot: I haz the power, 2017-05-05 03:25:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 04:20:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 04:20:27]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Matt Giuca, 2017-05-05 07:03:29]

The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 07:03:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Matt Giuca, 2017-05-05 07:07:14]

I think I know why those win and mac bots failed on Patch Set 2 (though I
haven't been able to test on those platforms).

If the ScopedTempDir directory has already been deleted, in PS2 it would've
returned false for IsValid which means we won't call Delete(), then the later
call to Delete() will explode. It's inconvenient that ScopedTempDir doesn't let
me directly check whether the path is empty (forcing me to also check if the
directory exists), so I've added IsEmpty.

Running the try jobs now (and going home)... PTAL. Unless you see a bunch of red
test results, in which case feel free to ignore this and I'll think harder about
what's going wrong.

Here's my full logic table for posterity:

There are four possibilities:
1. temp_dir_.path_.empty(): IsValid is false; Delete will fail in
   ~ScopedTempDir but silently (no thread error). This is fine.
2. !DirectoryExists(temp_dir_.path_): IsValid is false; Delete would
   succeed here (but we don't run it). It will check-fail-explode in
   ~ScopedTempDir. THIS IS BAD: we actually want to allow this case because
   it's harmless.
3. Delete() fails for some other reason. IsValid is true; Delete will
   CHECK-fail here. This is fine (we want to catch this case early).
4. Delete() succeeds here. This is fine; it will fail in ~ScopedTempDir but
   silently (no thread error).
CONCLUSION: We need to still call Delete() even if IsValid() returns false,
to correct case #2.

[commit-bot: I haz the power, 2017-05-05 08:52:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 08:52:43]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)