IndexedDB: Ensure overlapping commits correctly update blob journals

IndexedDB: Ensure overlapping commits correctly update blob journals

The IDB implementation uses journals to track blob files which are
to-be-written or to-be-deleted so that the files can be cleaned up
following a crash or abort. The journal records are updated as part of
a two-phase commit, and during database deletion.

The journal updates and cleanups incorrectly assumed that only one
transaction could commit at a time per backing store, so the first
phase of a second transaction could inadvertently "clean up" the in
progress work by a previous transaction, resulting in missing files.

Untangle this and ensure that (1) transaction commits only
append/remove journal entries, not the entire journal, and (2) changes
outside transactions are deferred if transactions are running.

R=cmumford@chromium.org
BUG=447836
Committed: https://crrev.com/f27dd1d7e9322388546c613cfed39bafda153859
Cr-Commit-Position: refs/heads/master@{#313998}

[jsbell, 2015-01-21 23:43:59]

cmumford@ - please take a look?

[cmumford, 2015-01-22 00:56:57]

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:636: if (!journal.size())
Nit: if (journal.empty()) reads much better.

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:659: BlobJournalType
old_journal;
Do we still need "old_journal"?

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:4001:
blobs_to_write_.push_back(journal_entry);
Nit: Do you even need the journal_entry variable?

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:4134:
--backing_store_->committing_transaction_count_;
We sure it won't underflow? DCHECK maybe?

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.h:608: // Remove the blob
directory for the specified database.
Nit: Comment pretty obvious from function name/args.

But, as you know, the blob stuff is really quite complex so maybe erring on the
side of too many comments isn't bad. Does this function delete all blobs
contained within - that's good info for this comment.

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:174: // in the
real implementation, therefore must be mutable here.
Why is RemoveBlobFile properly const? Seams reasonable for it not to be.

[jsbell, 2015-01-22 19:40:45]

Thanks. Better suggestions for tracking the outstanding count are welcome.

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:636: if (!journal.size())
On 2015/01/22 00:56:57, cmumford wrote:
> Nit: if (journal.empty()) reads much better.

Fixed here and other blob places.

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:659: BlobJournalType
old_journal;
On 2015/01/22 00:56:57, cmumford wrote:
> Do we still need "old_journal"?

Nope - removed.

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:4001:
blobs_to_write_.push_back(journal_entry);
On 2015/01/22 00:56:57, cmumford wrote:
> Nit: Do you even need the journal_entry variable?

Nope - just made it inline.

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.cc:4134:
--backing_store_->committing_transaction_count_;
On 2015/01/22 00:56:57, cmumford wrote:
> We sure it won't underflow? DCHECK maybe?

Added a DCHECK (here and in Rollback). I wasn't sure how fancy to get here. This
could be done by holding a refcounted object (and and check w/ HasOneRef instead
of count == 0) or something.

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store.h (right):

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store.h:608: // Remove the blob
directory for the specified database.
On 2015/01/22 00:56:57, cmumford wrote:
> Nit: Comment pretty obvious from function name/args.
> 
> But, as you know, the blob stuff is really quite complex so maybe erring on
the
> side of too many comments isn't bad. Does this function delete all blobs
> contained within - that's good info for this comment.

Noted that it removes any contained files too.

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/865013002/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:174: // in the
real implementation, therefore must be mutable here.
On 2015/01/22 00:56:57, cmumford wrote:
> Why is RemoveBlobFile properly const? Seams reasonable for it not to be.

The various blob methods are a mix of modifying the transaction state and just
helpers factoring out chunks of work. I sprinkled const in to make better sense
of what was going on. Apart from the override in the test, RemoveBlobFile() is
const since all it does is delete the file on disk. So the calling functions can
be const as well, which made it all easier to understand... this seems like one
of the few times mutable makes sense. Otherwise, a whole tree of callers can't
be const.

WDYT?

[jsbell, 2015-01-28 17:04:16]

ping cmumford@

[cmumford, 2015-01-29 22:27:18]

lgtm

[jsbell, 2015-01-30 18:00:01]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-01-30 18:01:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/865013002/40001

[commit-bot: I haz the power, 2015-01-30 20:18:03]

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

[commit-bot: I haz the power, 2015-01-30 20:18:04]

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

[jsbell, 2015-01-30 20:33:54]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-01-30 20:34:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/865013002/40001

[commit-bot: I haz the power, 2015-01-30 21:47:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-30 21:48:10]

Patchset 3 (id:??) landed as
https://crrev.com/f27dd1d7e9322388546c613cfed39bafda153859
Cr-Commit-Position: refs/heads/master@{#313998}