[IndexedDB] Integrating failures and corruption with transaction

[IndexedDB] Integrating failures and corruption with transaction

R=cmumford@chromium.org
BUG=363397, 662246
Committed: https://crrev.com/50ab051b3e40c25d39262bca9b71395d20632dc9
Cr-Commit-Position: refs/heads/master@{#435089}

[dmurph, 2016-11-15 23:30:39]

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

[commit-bot: I haz the power, 2016-11-15 23:31:28]

Dry run: 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, 2016-11-16 00:15:33]

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

[commit-bot: I haz the power, 2016-11-16 00:15:34]

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

[dmurph, 2016-11-16 23:55:50]

Hi Chris,

I know that this isn't passing all tests yet, but can you PTAL at the
architecture?

Thanks!
Daniel

[cmumford, 2016-11-21 18:29:22]

I don't see a problem with this approach - only minor comments.

Can you add a bit more to the bug description - maybe describing the motivation
for this change?

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

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.h:354: // Returns true when
no error and we haven't reached the end of the data.
Do you think it would be clearer if this instead returned an enumeration? Maybe
something like: ERROR, SUCCESS, LAST_KEY (names are crappy, but you get the
idea).

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_cursor.cc (right):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_cursor.cc:44: if (closed_) {
No problem with your check, but Do you think we need |closed_|? Can we instead
check |cursor_|?

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_cursor.cc:112: "Error advancing cursor"));
Tweak error message - same as AdvanceOperation above.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (left):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1039:
factory_->HandleBackingStoreCorruption(backing_store_->origin(), error);
I'm OK with returning the status, and having the caller responsible for calling
HandleBackingStoreCorruption. Just keep in mind that function writes out the
error message, so that it can be reconstructed on the next open - so we still
want the error to be as specific as possible.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:994: leveldb::Status s =
leveldb::Status::OK();
leveldb::Status defaults to OK.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1355: leveldb::Status s =
leveldb::Status::OK();
You probably want to reuse this |s| down below where another one hides this one.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_transaction.cc:427:
base::ASCIIToUTF16(result.ToString()));
I think that leveldb::Status can contain paths to the profile directory, so we
need to be careful to sanitize the message if it's being sent back to the page
in any way. I think we do that someplace else in the code, but can't find it
with a quick search.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_transaction.h (right):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_transaction.h:38: using OperationResult =
leveldb::Status;
I'm not a fan of introducing OperationResult. It obfuscates the type, and isn't
even shorter than leveldb::Status.

[cmumford, 2016-11-22 17:37:11]

dmurph: Take a look at https://codereview.chromium.org/2519073003/ which returns
a tuple(db,status) instead of an out status param. I think using this pattern
would make this CL a bit cleaner.

[dmurph, 2016-11-22 18:24:22]

On 2016/11/22 17:37:11, cmumford wrote:
> dmurph: Take a look at https://codereview.chromium.org/2519073003/ which
returns
> a tuple(db,status) instead of an out status param. I think using this pattern
> would make this CL a bit cleaner.

I don't understand how that would make this cleaner. We don't have an output
variable we're populating.

Sorry for the delay on this CL, working on blob stuff currently.

[dmurph, 2016-11-22 23:32:28]

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

[commit-bot: I haz the power, 2016-11-22 23:33:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2016-11-22 23:33:11]

Let me know what you think. I don't want to do a ton of cleanup here, as this is
just a dependency for my next CL which changes the lifetime of the
IndexedDBTransaction to be more sane (big cleanup).

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

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.h:354: // Returns true when
no error and we haven't reached the end of the data.
On 2016/11/21 18:29:22, cmumford wrote:
> Do you think it would be clearer if this instead returned an enumeration?
Maybe
> something like: ERROR, SUCCESS, LAST_KEY (names are crappy, but you get the
> idea).

mmmm this is a large change, as I'll want to change all of the Advance/Continue
calls above. Effects lots of files. I'll change the private ones here only for
now.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_cursor.cc (right):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_cursor.cc:44: if (closed_) {
On 2016/11/21 18:29:22, cmumford wrote:
> No problem with your check, but Do you think we need |closed_|? Can we instead
> check |cursor_|?

Done.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_cursor.cc:112: "Error advancing cursor"));
On 2016/11/21 18:29:22, cmumford wrote:
> Tweak error message - same as AdvanceOperation above.

Done.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (left):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1039:
factory_->HandleBackingStoreCorruption(backing_store_->origin(), error);
On 2016/11/21 18:29:22, cmumford wrote:
> I'm OK with returning the status, and having the caller responsible for
calling
> HandleBackingStoreCorruption. Just keep in mind that function writes out the
> error message, so that it can be reconstructed on the next open - so we still
> want the error to be as specific as possible.

Yeah... We now put the leveldb error string in there. I wanted to keep it as
simple as possible and not do a pair/tuple of error + message.

We actually just need:
* If we've succeeded,
* If not, if we're a corruption
* If corruption, a message.

leveldb::Status has all of this. In these cases I can replace the message in the
status with our own, does that help?

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:994: leveldb::Status s =
leveldb::Status::OK();
On 2016/11/21 18:29:22, cmumford wrote:
> leveldb::Status defaults to OK.

mmmmm I think I prefer this if that's ok, it makes it explicit for the reader.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1355: leveldb::Status s =
leveldb::Status::OK();
On 2016/11/21 18:29:22, cmumford wrote:
> You probably want to reuse this |s| down below where another one hides this
one.

Done.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_transaction.cc:427:
base::ASCIIToUTF16(result.ToString()));
On 2016/11/21 18:29:22, cmumford wrote:
> I think that leveldb::Status can contain paths to the profile directory, so we
> need to be careful to sanitize the message if it's being sent back to the page
> in any way. I think we do that someplace else in the code, but can't find it
> with a quick search.

We don't actually send this back to the page, it's just used to send to
IndexedDbBackingStore::RecordCorruptionInfo, which we save to a file.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_transaction.h (right):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_transaction.h:38: using OperationResult =
leveldb::Status;
On 2016/11/21 18:29:22, cmumford wrote:
> I'm not a fan of introducing OperationResult. It obfuscates the type, and
isn't
> even shorter than leveldb::Status.

Done.

[commit-bot: I haz the power, 2016-11-23 00:04:57]

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

[commit-bot: I haz the power, 2016-11-23 00:04:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmurph, 2016-11-23 23:25:32]

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

[commit-bot: I haz the power, 2016-11-23 23:26:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2016-11-24 00:07:33]

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

[commit-bot: I haz the power, 2016-11-24 00:07:56]

Dry run: 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, 2016-11-24 02:09:30]

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

[commit-bot: I haz the power, 2016-11-24 02:09:31]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[dmurph, 2016-11-28 18:57:25]

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

[commit-bot: I haz the power, 2016-11-28 18:57:58]

Dry run: 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, 2016-11-28 19:43:42]

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

[commit-bot: I haz the power, 2016-11-28 19:43: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_...)

[cmumford, 2016-11-28 20:48:25]

Looking good - just one comment on dataLossMessage.

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

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.h:354: // Returns true when
no error and we haven't reached the end of the data.
On 2016/11/22 23:33:10, dmurph wrote:
> On 2016/11/21 18:29:22, cmumford wrote:
> > Do you think it would be clearer if this instead returned an enumeration?
> Maybe
> > something like: ERROR, SUCCESS, LAST_KEY (names are crappy, but you get the
> > idea).
> 
> mmmm this is a large change, as I'll want to change all of the
Advance/Continue
> calls above. Effects lots of files. I'll change the private ones here only for
> now.

Acknowledged.

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/2506773002/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_transaction.cc:427:
base::ASCIIToUTF16(result.ToString()));
On 2016/11/22 23:33:11, dmurph wrote:
> On 2016/11/21 18:29:22, cmumford wrote:
> > I think that leveldb::Status can contain paths to the profile directory, so
we
> > need to be careful to sanitize the message if it's being sent back to the
page
> > in any way. I think we do that someplace else in the code, but can't find it
> > with a quick search.
> 
> We don't actually send this back to the page, it's just used to send to
> IndexedDbBackingStore::RecordCorruptionInfo, which we save to a file.

Yes, but that file is read on the next db open in ReadCorruptionInfo(), and the
message is sent in dataLossMessage in the upgradeneeded event.

[dmurph, 2016-11-28 21:46:23]

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

[dmurph, 2016-11-28 21:47:17]

We now sanitize the corruption messages :)

[commit-bot: I haz the power, 2016-11-28 21:47:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cmumford, 2016-11-28 21:52:45]

lgtm - thx for the last fix.

[commit-bot: I haz the power, 2016-11-28 23:59:03]

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

[commit-bot: I haz the power, 2016-11-28 23:59:04]

Dry run: This issue passed the CQ dry run.

[dmurph, 2016-11-29 18:01:45]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2016-11-29 18:02:02]

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, 2016-11-29 18:12:47]

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

[commit-bot: I haz the power, 2016-11-29 18:12:48]

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

[dmurph, 2016-11-29 18:44:40]

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

[commit-bot: I haz the power, 2016-11-29 18:45:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2016-11-29 18:52:09]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2016-11-29 18:52:17]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-11-29 18:52:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/2506773002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-29 18:52:31]

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

[dmurph, 2016-11-29 19:49:24]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2016-11-29 19:54:26]

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

[commit-bot: I haz the power, 2016-11-29 19:54:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2016-11-29 20:09:58]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2016-11-29 20:10:10]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-11-29 20:10:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/2506773002/#ps160001
(title: "removed extra log statements")

[commit-bot: I haz the power, 2016-11-29 20:10:28]

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, 2016-11-29 22:13:33]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480450210055210,
"parent_rev": "7855022f78c33224c3065b6ff5bcad3b9c331052", "commit_rev":
"ac7a450b53ffcb51bb47cc21f4021234c96af540"}

[commit-bot: I haz the power, 2016-11-29 22:14:02]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-11-29 22:17:30]

Description was changed from

==========
[IndexedDB] Integrating failures and corruption with transaction

R=cmumford@chromium.org
BUG=363397,662246
==========

to

==========
[IndexedDB] Integrating failures and corruption with transaction

R=cmumford@chromium.org
BUG=363397,662246

Committed: https://crrev.com/50ab051b3e40c25d39262bca9b71395d20632dc9
Cr-Commit-Position: refs/heads/master@{#435089}
==========

[commit-bot: I haz the power, 2016-11-29 22:17:32]

Patchset 9 (id:??) landed as
https://crrev.com/50ab051b3e40c25d39262bca9b71395d20632dc9
Cr-Commit-Position: refs/heads/master@{#435089}