FileAPI: Run database recovery on IOError.

FileAPI: Run database recovery on IOError.

In this case, the databases was actually corrupted but the database classes did not try to repair.
After this CL lands, these corruption should be repaired correctly.

TEST="content_unittests: FileSystemDirectoryDatabaseTest.TestRepairDatabase_MissingManifest & FileSystemOriginDatabaseTest.DatabaseRecoveryForMissingManifestTest"
BUG=229773
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193380

[tzik, 2013-04-10 06:08:48]

PTAL

[kinuko, 2013-04-10 06:23:49]

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_database_test_helper.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_database_test_helper.cc:99: picked_file_number =
number;
Maybe we could just delete all files rather than picking the smallest one?

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database.cc:717: if (!status.IsCorruption()
&& !status.IsIOError())
Let me confirm. What other error types could we have here?

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database_unittest.cc:664:
DeleteDatabaseFile(kDatabaseDirectory, leveldb::kDescriptorFile);
What happens when other files are deleted?

[tzik, 2013-04-10 06:49:20]

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_database_test_helper.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_database_test_helper.cc:99: picked_file_number =
number;
On 2013/04/10 06:23:49, kinuko wrote:
> Maybe we could just delete all files rather than picking the smallest one?

Done.

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database.cc:717: if (!status.IsCorruption()
&& !status.IsIOError())
On 2013/04/10 06:23:49, kinuko wrote:
> Let me confirm. What other error types could we have here?

Status can be one of:
 - NotFound,
 - Corruption,
 - NotSupported,
 - InvalidArgument,
 - IOError.

NotFound is only when we look up the database,
Corruption and IOError are now handled here,
NotSupported and InvalidArgument looks irrelevant to handle here.

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database_unittest.cc:664:
DeleteDatabaseFile(kDatabaseDirectory, leveldb::kDescriptorFile);
On 2013/04/10 06:23:49, kinuko wrote:
> What happens when other files are deleted?

For .sst deletion, that was handled as corruption.
Other file deletion for LOG, LOG.old, CURRENT, LOCK, lost/*  or *.log caused no
error.
MANIFEST-* file deletion seems only case that we didn't handled.

[kinuko, 2013-04-10 08:41:16]

looking good

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database.cc:717: if (!status.IsCorruption()
&& !status.IsIOError())
On 2013/04/10 06:49:20, tzik wrote:
> On 2013/04/10 06:23:49, kinuko wrote:
> > Let me confirm. What other error types could we have here?
> 
> Status can be one of:
>  - NotFound,
>  - Corruption,
>  - NotSupported,
>  - InvalidArgument,
>  - IOError.
> 
> NotFound is only when we look up the database,
> Corruption and IOError are now handled here,
> NotSupported and InvalidArgument looks irrelevant to handle here.

Can we have a brief comment about them? Or at least should we have a comment why
we don't bail out for corruption and IO error case?

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database_unittest.cc:664:
DeleteDatabaseFile(kDatabaseDirectory, leveldb::kDescriptorFile);
On 2013/04/10 06:49:20, tzik wrote:
> On 2013/04/10 06:23:49, kinuko wrote:
> > What happens when other files are deleted?
> 
> For .sst deletion, that was handled as corruption.
> Other file deletion for LOG, LOG.old, CURRENT, LOCK, lost/*  or *.log caused
no
> error.
> MANIFEST-* file deletion seems only case that we didn't handled.

Should we add tests for them too (even if they may not be handled by the new
code)?

https://codereview.chromium.org/13946004/diff/9001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_database_test_helper.cc (right):

https://codereview.chromium.org/13946004/diff/9001/webkit/fileapi/file_system...
webkit/fileapi/file_system_database_test_helper.cc:96: file_deleted = true;
Can we have a comment here like:

// We may have multiple files for the same type, so don't break here.

[tzik, 2013-04-10 10:59:49]

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database.cc:717: if (!status.IsCorruption()
&& !status.IsIOError())
On 2013/04/10 08:41:17, kinuko wrote:
> On 2013/04/10 06:49:20, tzik wrote:
> > On 2013/04/10 06:23:49, kinuko wrote:
> > > Let me confirm. What other error types could we have here?
> > 
> > Status can be one of:
> >  - NotFound,
> >  - Corruption,
> >  - NotSupported,
> >  - InvalidArgument,
> >  - IOError.
> > 
> > NotFound is only when we look up the database,
> > Corruption and IOError are now handled here,
> > NotSupported and InvalidArgument looks irrelevant to handle here.
> 
> Can we have a brief comment about them? Or at least should we have a comment
why
> we don't bail out for corruption and IO error case?

Done.

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database_unittest.cc:664:
DeleteDatabaseFile(kDatabaseDirectory, leveldb::kDescriptorFile);
On 2013/04/10 08:41:17, kinuko wrote:
> On 2013/04/10 06:49:20, tzik wrote:
> > On 2013/04/10 06:23:49, kinuko wrote:
> > > What happens when other files are deleted?
> > 
> > For .sst deletion, that was handled as corruption.
> > Other file deletion for LOG, LOG.old, CURRENT, LOCK, lost/*  or *.log caused
> no
> > error.
> > MANIFEST-* file deletion seems only case that we didn't handled.
> 
> Should we add tests for them too (even if they may not be handled by the new
> code)?

Added tests for them on OriginDatabase test.
Should we also add for DirectoryDatabase?

[kinuko, 2013-04-10 11:02:13]

lgtm

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database_unittest.cc:664:
DeleteDatabaseFile(kDatabaseDirectory, leveldb::kDescriptorFile);
On 2013/04/10 10:59:49, tzik wrote:
> On 2013/04/10 08:41:17, kinuko wrote:
> > On 2013/04/10 06:49:20, tzik wrote:
> > > On 2013/04/10 06:23:49, kinuko wrote:
> > > > What happens when other files are deleted?
> > > 
> > > For .sst deletion, that was handled as corruption.
> > > Other file deletion for LOG, LOG.old, CURRENT, LOCK, lost/*  or *.log
caused
> > no
> > > error.
> > > MANIFEST-* file deletion seems only case that we didn't handled.
> > 
> > Should we add tests for them too (even if they may not be handled by the new
> > code)?
> 
> Added tests for them on OriginDatabase test.
> Should we also add for DirectoryDatabase?

If you think we should do so please add them; I'm fine with this (if anything
changes in leveldb the other test will catch).

[tzik, 2013-04-10 13:01:09]

Thanks!

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_directory_database_unittest.cc (right):

https://codereview.chromium.org/13946004/diff/2001/webkit/fileapi/file_system...
webkit/fileapi/file_system_directory_database_unittest.cc:664:
DeleteDatabaseFile(kDatabaseDirectory, leveldb::kDescriptorFile);
On 2013/04/10 11:02:13, kinuko wrote:
> On 2013/04/10 10:59:49, tzik wrote:
> > On 2013/04/10 08:41:17, kinuko wrote:
> > > On 2013/04/10 06:49:20, tzik wrote:
> > > > On 2013/04/10 06:23:49, kinuko wrote:
> > > > > What happens when other files are deleted?
> > > > 
> > > > For .sst deletion, that was handled as corruption.
> > > > Other file deletion for LOG, LOG.old, CURRENT, LOCK, lost/*  or *.log
> caused
> > > no
> > > > error.
> > > > MANIFEST-* file deletion seems only case that we didn't handled.
> > > 
> > > Should we add tests for them too (even if they may not be handled by the
new
> > > code)?
> > 
> > Added tests for them on OriginDatabase test.
> > Should we also add for DirectoryDatabase?
> 
> If you think we should do so please add them; I'm fine with this (if anything
> changes in leveldb the other test will catch).

I believe it's enough to catch leveldb change. ;)

[commit-bot: I haz the power, 2013-04-10 13:01:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/13946004/15001

[commit-bot: I haz the power, 2013-04-10 14:09:52]

Change committed as 193380

[ericu, 2013-04-10 21:39:11]

Awesome--thanks for handling this!