This is the implementation of the primary and secondary blob journals for

This is the implementation of the primary and secondary blob journals for
IDB/Blob support.  It's not yet used at all, so there are a few fake calls to
eliminate compiler errors until the rest of the code lands.

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

[jsbell, 2014-04-30 21:18:05]

lgtm

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2065:
BlobJournalType::iterator journal_iter;
nit: move into the for()'s init clause, since the use is scoped and it shouldn't
make it wrap more.

[ericu, 2014-04-30 21:21:13]

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2065:
BlobJournalType::iterator journal_iter;
On 2014/04/30 21:18:05, jsbell wrote:
> nit: move into the for()'s init clause, since the use is scoped and it
shouldn't
> make it wrap more.

Yeah, good point.

[cmumford, 2014-04-30 21:29:29]

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:491:
INTERNAL_READ_ERROR(KEY_EXISTS_IN_OBJECT_STORE);
INTERNAL_READ_ERROR_UNTESTED assuming you don't have a unit test that hits this
line. Same for others.

P.S. Do you think we should add a new histogram value for this error?

P.P.S. Should we have a if (s.IsCorruption()) check here and delete/cleanup if
so?

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:495: if (!found)
why not just (if !found || !data.size())?

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:509:
leveldb_transaction->Remove(level_db_key);
A one line function doesn't seem worth the added complexity - unless there are
upcoming changes in a future CL.

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:561: std::string key =
LiveBlobJournalKey::Encode();
nit: I see that sometimes you use a const string for keys, and sometimes not.
Intentional? From what I can see none are modified.

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2266: return
leveldb::Status::OK();
No need to worry about journal_transaction->Commit()?

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2277:
RemoveBlobFile(database_id, blob_key);
Since this returns false on error can we at least log this failure?

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:380: for (iter =
journal.begin(); iter != journal.end(); ++iter) {
If the journal is empty (which may not be possible?) is it OK to leave "into"
untouched?

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:619: BlobJournalType
output;
Should an empty slice be considered an error?

[ericu, 2014-04-30 22:16:09]

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:491:
INTERNAL_READ_ERROR(KEY_EXISTS_IN_OBJECT_STORE);
On 2014/04/30 21:29:30, cmumford wrote:
> INTERNAL_READ_ERROR_UNTESTED assuming you don't have a unit test that hits
this
> line. Same for others.

I made the quick fix; no test yet.

> P.S. Do you think we should add a new histogram value for this error?

Yeah, let's do.  Added.  Also added the changes to histograms.xml.

> P.P.S. Should we have a if (s.IsCorruption()) check here and delete/cleanup if
> so?

So far it looks like we're really only specifically dealing with corruption on
open.  In other places we just tend to fail.  I like that centralization in
general.

However, I've got that TODO in there because I'm not really sure what kind of
failures we're going to get.  If the whole database is corrupt, we need no
specific fix here.  But if we've got a bug that corrupts just the journal [which
probably wouldn't lead to IsCorruption(s)], we'd want local cleanup.

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:495: if (!found)
On 2014/04/30 21:29:30, cmumford wrote:
> why not just (if !found || !data.size())?

Done.

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:509:
leveldb_transaction->Remove(level_db_key);
On 2014/04/30 21:29:30, cmumford wrote:
> A one line function doesn't seem worth the added complexity - unless there are
> upcoming changes in a future CL.

No, there's just the one line, called in two places.  It feels to me like it
abstracts away the implementation of the journal a little bit, but it's not a
big deal.  Josh, do you have a preference?

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:561: std::string key =
LiveBlobJournalKey::Encode();
On 2014/04/30 21:29:30, cmumford wrote:
> nit: I see that sometimes you use a const string for keys, and sometimes not.
> Intentional? From what I can see none are modified.

Nope; just an oversight.  Fixed.

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2266: return
leveldb::Status::OK();
On 2014/04/30 21:29:30, cmumford wrote:
> No need to worry about journal_transaction->Commit()?

I'm not sure what you mean.  It's discarded on exit if we quit early [the scoped
destructor just clears it], and we return its status if we return normally.

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2277:
RemoveBlobFile(database_id, blob_key);
On 2014/04/30 21:29:30, cmumford wrote:
> Since this returns false on error can we at least log this failure?

Actually, we should do more than that.  Changed to fail with IOError status if a
deletion fails--that way we don't drop the journal, and we'll try again on those
files.  If they're not there when we try again, RemoveBlobFoo will return
success.

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_leveldb_coding.cc (right):

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:380: for (iter =
journal.begin(); iter != journal.end(); ++iter) {
On 2014/04/30 21:29:30, cmumford wrote:
> If the journal is empty (which may not be possible?) is it OK to leave "into"
> untouched?

Yes.  The convention in this file is that all EncodeXXX functions append, rather
than replacing.

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_leveldb_coding.cc:619: BlobJournalType
output;
On 2014/04/30 21:29:30, cmumford wrote:
> Should an empty slice be considered an error?

No, that's an empty journal.

[jsbell, 2014-04-30 22:21:45]

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:509:
leveldb_transaction->Remove(level_db_key);
On 2014/04/30 22:16:10, ericu wrote:
> On 2014/04/30 21:29:30, cmumford wrote:
> > A one line function doesn't seem worth the added complexity - unless there
are
> > upcoming changes in a future CL.
> 
> No, there's just the one line, called in two places.  It feels to me like it
> abstracts away the implementation of the journal a little bit, but it's not a
> big deal.  Josh, do you have a preference?

I like abstracting it.

[cmumford, 2014-04-30 22:32:32]

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:2266: return
leveldb::Status::OK();
On 2014/04/30 22:16:10, ericu wrote:
> On 2014/04/30 21:29:30, cmumford wrote:
> > No need to worry about journal_transaction->Commit()?
> 
> I'm not sure what you mean.  It's discarded on exit if we quit early [the
scoped
> destructor just clears it], and we return its status if we return normally.

I misread it - thought transaction was a param - my bad.

[cmumford, 2014-04-30 22:40:08]

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/264483002/diff/50001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_backing_store.cc:491:
INTERNAL_READ_ERROR(KEY_EXISTS_IN_OBJECT_STORE);
On 2014/04/30 22:16:10, ericu wrote:
> On 2014/04/30 21:29:30, cmumford wrote:
> > INTERNAL_READ_ERROR_UNTESTED assuming you don't have a unit test that hits
> this
> > line. Same for others.
> 
> I made the quick fix; no test yet.
> 
> > P.S. Do you think we should add a new histogram value for this error?
> 
> Yeah, let's do.  Added.  Also added the changes to histograms.xml.
> 
> > P.P.S. Should we have a if (s.IsCorruption()) check here and delete/cleanup
if
> > so?
> 
> So far it looks like we're really only specifically dealing with corruption on
> open.  In other places we just tend to fail.  I like that centralization in
> general.
> 
> However, I've got that TODO in there because I'm not really sure what kind of
> failures we're going to get.  If the whole database is corrupt, we need no
> specific fix here.  But if we've got a bug that corrupts just the journal
[which
> probably wouldn't lead to IsCorruption(s)], we'd want local cleanup.

Unfortunately all corruptions are not detectable at open time. r260697 added
detection of corruption after open which was improved in a few latter commits:
r264256 and r1914193. As long as there's a TODO them I'm OK.

[cmumford, 2014-04-30 23:48:29]

lgtm

[ericu, 2014-04-30 23:54:42]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-04-30 23:55:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/264483002/90001

[commit-bot: I haz the power, 2014-04-30 23:59:20]

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

[commit-bot: I haz the power, 2014-04-30 23:59:20]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[ericu, 2014-05-01 00:04:49]

+asvitkine for owner approval of the histogram change.

[Alexei Svitkine (slow), 2014-05-01 00:06:52]

lgtm

[ericu, 2014-05-01 13:57:28]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-05-01 13:57:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/264483002/100001

[commit-bot: I haz the power, 2014-05-01 14:48:19]

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

[commit-bot: I haz the power, 2014-05-01 14:48:19]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[ericu, 2014-05-01 18:31:59]

The CQ bit was checked by ericu@chromium.org

[commit-bot: I haz the power, 2014-05-01 18:32:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/264483002/100001

[commit-bot: I haz the power, 2014-05-01 23:38:29]

Change committed as 267665