Don't reset the database when there was an I/O error.

Don't reset the database when there was an I/O error.

Unlike corruption errors, I/O errors have the potential to be ephemeral.
We shouldn't aggressively delete the database in those cases. Instead,
punt that decision up to the web app.

Also, can remove the full disk catch-all. That was intended to catch
full disks masquerading as other I/O errors.

BUG=239882
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227444

[dgrogan, 2013-10-04 21:47:04]

Josh or Alec, could you review this?

[jsbell, 2013-10-04 23:32:33]

lgtm with nits

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_backing_store.cc (left):

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:559: } else if
(*is_disk_full) {
Just to confirm my reading:

* RecoveryCouldBeFruitful will now return true for ENOSPC (etc) so this would
never be reached.
* We already histogram ENOSPC (etc) via RecordOSError so we're not losing any
logging coverage

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:561:
HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_DISK_FULL);
I'm guessing we're not gaining anything by logging disk full at the IDB layer in
addition to the leveldb layer.

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc
(right):

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc:141:
MockErrorLevelDBFactory<int> mock_leveldb_factory3(EIO, false);
Do we have any cases where DestroyLevelDB() is called now? If not, should we
simplify the fixture?

https://codereview.chromium.org/26045002/diff/5001/third_party/leveldatabase/...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/26045002/diff/5001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:395: bool
RecoveryCouldBeFruitful(leveldb::Status status) {
Could this be renamed now to StatusIsIOError or some such?

[dgrogan, 2013-10-05 01:06:16]

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_backing_store.cc (left):

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:559: } else if
(*is_disk_full) {
On 2013/10/04 23:32:33, jsbell wrote:
> Just to confirm my reading:
> 
> * RecoveryCouldBeFruitful will now return true for ENOSPC (etc) so this would
> never be reached.

This could be reached if there was <100k free space and none of the IO
operations returned ENOSPC. But its purpose was to allow for full disks to
manifest as all kinds of errors, not just ENOSPC, which supposedly used to
happen on CrOS and it didn't make sense to reset the db in those cases. But now
that we don't reset the db in the case of any IO error, we don't need this.

> * We already histogram ENOSPC (etc) via RecordOSError so we're not losing any
> logging coverage

Correct.

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store.cc:561:
HistogramOpenStatus(INDEXED_DB_BACKING_STORE_OPEN_DISK_FULL);
On 2013/10/04 23:32:33, jsbell wrote:
> I'm guessing we're not gaining anything by logging disk full at the IDB layer
in
> addition to the leveldb layer.

Yeah, we still have
https://uma.googleplex.com/histograms/?dayCount=1&endDate=10-03-2013&version=...

which shows that full disks still cause a large portion of failures.

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc
(right):

https://codereview.chromium.org/26045002/diff/5001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_cleanup_on_io_error_unittest.cc:141:
MockErrorLevelDBFactory<int> mock_leveldb_factory3(EIO, false);
On 2013/10/04 23:32:33, jsbell wrote:
> Do we have any cases where DestroyLevelDB() is called now? If not, should we
> simplify the fixture?

Good point. I added a TODO. Even though it's just like 10 lines of code, I don't
want to delete it because I have an inkling that we're going to turn deleting
back on for a subset of I/O errors in a near-future release.

https://codereview.chromium.org/26045002/diff/5001/third_party/leveldatabase/...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/26045002/diff/5001/third_party/leveldatabase/...
third_party/leveldatabase/env_chromium.cc:395: bool
RecoveryCouldBeFruitful(leveldb::Status status) {
On 2013/10/04 23:32:33, jsbell wrote:
> Could this be renamed now to StatusIsIOError or some such?

Done.

[dgrogan, 2013-10-05 01:11:18]

I'm not going to submit until I've heard back from you that we're on the same
page with the 100k / ENOSPC thing.

[jsbell, 2013-10-07 18:52:00]

lgtm

[commit-bot: I haz the power, 2013-10-07 18:52:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/26045002/11001

[commit-bot: I haz the power, 2013-10-07 19:05:19]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-10-08 00:13:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgrogan@chromium.org/26045002/11001

[commit-bot: I haz the power, 2013-10-08 02:52:35]

Change committed as 227444