Tell IDB frontend about data loss

Tell IDB frontend about data loss.

When IDB performs "cleanup" on its backing store pass a flag
indicating such to the frontend, which will expose it to JS.

Blink side https://codereview.chromium.org/17060008/

BUG=172626
R=aedla@chromium.org, cevans@chromium.org, jam@chromium.org, jamesr@chromium.org, jsbell@chromium.org, tsepez@chromium.org

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

[dgrogan, 2013-06-14 20:16:29]

Josh, could you review this?

I didn't add the plumbing for a new attribute on the success event since that
would always be "none." We can add it if we ever get a more sophisticated
recovery implementation that could "partial". Similarly, I'm going to fill out
the spec bug to reflect that the new attribute is only present on the
upgradeneeded event.

[jsbell, 2013-06-14 20:42:58]

overall LGTM with some nits.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:340: return
OpenBackingStore(
Add EXPECT_FALSE(data_loss) ?

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

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.cc:67: bool data_loss)
Pluming this through VersionChangeOperation seems odd at first glance, although
it makes sense if you follow the flow of the flag through the system.

Some alternatives that come to mind.
* Have it be a state on database, i.e. "when opened, there was some data loss".
I guess it would need to be reset after being plucked out so only the first
connection would see it.
* Have it be a state on the backing store itself, i.e. "when opened, there was
some data loss". This would let us signal each subsequent database open that
there was data loss, but then every database would get it until all handles were
closed, unless we added even more state tracking. :(

I don't think either of those are better than what's here, though.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.cc:1475: false);
Can you give the argument a default value instead of passing explicit false in
these cases?

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.cc:1510: false);
Ditto.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database_unittest.cc:120: false);
Give the argument a default value instead of passing explicit false?

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database_unittest.cc:132: false);
Ditto?

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database_unittest.cc:191: false);
Ditto?

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_factory.cc (right):

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_factory.cc:58: bool data_loss;
Add a TODO to figure out if this should be plumbed back to script when this API
gets a more robust design?

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_factory.cc:91: bool data_loss;
Also add a TODO here?

https://codereview.chromium.org/17033004/diff/2001/content/common/indexed_db/...
File content/common/indexed_db/indexed_db_messages.h (right):

https://codereview.chromium.org/17033004/diff/2001/content/common/indexed_db/...
content/common/indexed_db/indexed_db_messages.h:290: IPC_STRUCT_MEMBER(int32,
data_loss)
Why not bool?

https://codereview.chromium.org/17033004/diff/2001/content/test/data/indexedd...
File content/test/data/indexeddb/open_bad_db.js (right):

https://codereview.chromium.org/17033004/diff/2001/content/test/data/indexedd...
content/test/data/indexeddb/open_bad_db.js:16: shouldBe("event.dataLoss",
"'total'");
Will it be possible to land this before the Blink changes?

[dgrogan, 2013-06-14 22:06:43]

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:340: return
OpenBackingStore(
On 2013/06/14 20:42:58, jsbell wrote:
> Add EXPECT_FALSE(data_loss) ?

Done.

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

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.cc:67: bool data_loss)
I considered keeping some state like you describe but also didn't like any of
the options. Shrug.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.cc:1475: false);
On 2013/06/14 20:42:58, jsbell wrote:
> Can you give the argument a default value instead of passing explicit false in
> these cases?

Done.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database.cc:1510: false);
On 2013/06/14 20:42:58, jsbell wrote:
> Ditto.

Done.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database_unittest.cc:120: false);
On 2013/06/14 20:42:58, jsbell wrote:
> Give the argument a default value instead of passing explicit false?

jar@ recently reminded me that default arguments are against the style guide,
though now that I read it, it recommends simply using an overload instead. Seems
reasonable.

Simulated default argument via method overload it is.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database_unittest.cc:132: false);
On 2013/06/14 20:42:58, jsbell wrote:
> Ditto?

Done.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_database_unittest.cc:191: false);
On 2013/06/14 20:42:58, jsbell wrote:
> Ditto?

Done.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
File content/browser/indexed_db/indexed_db_factory.cc (right):

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_factory.cc:58: bool data_loss;
On 2013/06/14 20:42:58, jsbell wrote:
> Add a TODO to figure out if this should be plumbed back to script when this
API
> gets a more robust design?

Done.

https://codereview.chromium.org/17033004/diff/2001/content/browser/indexed_db...
content/browser/indexed_db/indexed_db_factory.cc:91: bool data_loss;
On 2013/06/14 20:42:58, jsbell wrote:
> Also add a TODO here?

Done.

https://codereview.chromium.org/17033004/diff/2001/content/common/indexed_db/...
File content/common/indexed_db/indexed_db_messages.h (right):

https://codereview.chromium.org/17033004/diff/2001/content/common/indexed_db/...
content/common/indexed_db/indexed_db_messages.h:290: IPC_STRUCT_MEMBER(int32,
data_loss)
On 2013/06/14 20:42:58, jsbell wrote:
> Why not bool?

Originally I had it as enum/int everywhere.

https://codereview.chromium.org/17033004/diff/2001/content/test/data/indexedd...
File content/test/data/indexeddb/open_bad_db.js (right):

https://codereview.chromium.org/17033004/diff/2001/content/test/data/indexedd...
content/test/data/indexeddb/open_bad_db.js:16: shouldBe("event.dataLoss",
"'total'");
On 2013/06/14 20:42:58, jsbell wrote:
> Will it be possible to land this before the Blink changes?

Ugh, no. Looks like this will be a 4-patch change.

1) Add Blink API alongside old function signature
2) Chrome code
3) Blink code, remove old Blink API function signature.
4) This test

[dgrogan, 2013-06-17 22:52:43]

Josh, could you take another look at this? It uses the enum declared in the
Blink API.

[jsbell, 2013-06-17 23:21:48]

lgtm with DCHECK/style nits

https://codereview.chromium.org/17033004/diff/19001/content/browser/in_proces...
File content/browser/in_process_webkit/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/17033004/diff/19001/content/browser/in_proces...
content/browser/in_process_webkit/indexed_db_callbacks.cc:58: const
IndexedDBDatabaseMetadata&,
Not new in this patch, but name the argument: /*metadata*/

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:341:
EXPECT_FALSE(data_loss);
Should be: EXPECT_EQ(WebKit::WebIDBCallbacks::None, data_loss)

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database.cc:1563: DCHECK(!data_loss);
Should be: DCHECK_NE(WebKit::WebIDBCallbacks::None, data_loss)

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database.cc:1646: DCHECK(!data_loss);
Ditto.

[dgrogan, 2013-06-17 23:52:40]

https://codereview.chromium.org/17033004/diff/19001/content/browser/in_proces...
File content/browser/in_process_webkit/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/17033004/diff/19001/content/browser/in_proces...
content/browser/in_process_webkit/indexed_db_callbacks.cc:58: const
IndexedDBDatabaseMetadata&,
On 2013/06/17 23:21:48, jsbell wrote:
> Not new in this patch, but name the argument: /*metadata*/

Done.

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:341:
EXPECT_FALSE(data_loss);
On 2013/06/17 23:21:48, jsbell wrote:
> Should be: EXPECT_EQ(WebKit::WebIDBCallbacks::None, data_loss)

Done.

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database.cc:1563: DCHECK(!data_loss);
On 2013/06/17 23:21:48, jsbell wrote:
> Should be: DCHECK_NE(WebKit::WebIDBCallbacks::None, data_loss)

Done.

https://codereview.chromium.org/17033004/diff/19001/content/browser/indexed_d...
content/browser/indexed_db/indexed_db_database.cc:1646: DCHECK(!data_loss);
On 2013/06/17 23:21:48, jsbell wrote:
> Ditto.

Done.

[commit-bot: I haz the power, 2013-06-18 14:52:51]

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

[commit-bot: I haz the power, 2013-06-18 15:01:28]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[dgrogan, 2013-06-18 15:03:03]

tsepez, could you review this as an IPC owner?

[dgrogan, 2013-06-18 23:34:09]

cevans@, looks like Tom is OOO. Could you review this as an IPC owner?

[Chris Evans, 2013-06-18 23:39:15]

On 2013/06/18 23:34:09, dgrogan wrote:
> cevans@, looks like Tom is OOO. Could you review this as an IPC owner?

@aedla, can you take this one? You've done some good IDB IPC work in the past
IIRC.

[aedla, 2013-06-19 10:56:51]

https://codereview.chromium.org/17033004/diff/11016/content/common/indexed_db...
File content/common/indexed_db/indexed_db_messages.h (right):

https://codereview.chromium.org/17033004/diff/11016/content/common/indexed_db...
content/common/indexed_db/indexed_db_messages.h:290: IPC_STRUCT_MEMBER(int32,
data_loss)
Could this be sent as WebKit::WebIDBCallbacks::DataLoss instead?

And then registered as
IPC_ENUM_TRAITS_MAX_VALUE(WebKit::WebIDBDatabase::TaskType,
                          WebKit::WebIDBDatabase::DataLossTotal)

Then the enum will be validated by the receiver. On a browser -> renderer
message it doesn't matter much, but we're trying move in the direction where all
enums are validated across IPC. IPC_ENUM_TRAITS is now deprecated.

[dgrogan, 2013-06-19 16:21:21]

aedla, ptal.

What happens if a compromised renderer tries to send a value that is too large?

[aedla, 2013-06-19 19:20:12]

On 2013/06/19 16:21:21, dgrogan wrote:
> aedla, ptal.

LGTM

> What happens if a compromised renderer tries to send a value that is too
large?

I think the cast back to enum will trigger undefined behavior. The enum value
will likely just be out of range. There have been issues where the handler
indexes an array out of bounds with bad enums.

[aedla, 2013-06-19 19:20:35]

https://codereview.chromium.org/17033004/diff/43001/content/common/indexed_db...
File content/common/indexed_db/indexed_db_messages.h (right):

https://codereview.chromium.org/17033004/diff/43001/content/common/indexed_db...
content/common/indexed_db/indexed_db_messages.h:27:
IPC_ENUM_TRAITS_MAX_VALUE(WebKit::WebIDBCallbacks::DataLoss, \
nit: I think \ is not actually necessary, is it?

[commit-bot: I haz the power, 2013-06-19 19:20:45]

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

[dgrogan, 2013-06-19 19:22:38]

https://codereview.chromium.org/17033004/diff/43001/content/common/indexed_db...
File content/common/indexed_db/indexed_db_messages.h (right):

https://codereview.chromium.org/17033004/diff/43001/content/common/indexed_db...
content/common/indexed_db/indexed_db_messages.h:27:
IPC_ENUM_TRAITS_MAX_VALUE(WebKit::WebIDBCallbacks::DataLoss, \
On 2013/06/19 19:20:35, aedla wrote:
> nit: I think \ is not actually necessary, is it?

Nope, not necessary, fixed.

[commit-bot: I haz the power, 2013-06-19 19:23:34]

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

[commit-bot: I haz the power, 2013-06-19 19:34:20]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[dgrogan, 2013-06-19 19:36:49]

cevans@, seems that aedla@ is not an OWNER. Could you rubber stamp his LGTM?

[dgrogan, 2013-06-20 16:15:33]

ping scarybeasts

[dgrogan, 2013-06-21 16:26:18]

inferno, could you review this?

aedla and jsbell already reviewed but it needs an lgtm from a
content/common/indexed_db/*_messages.h owner.

[Chris Evans, 2013-06-21 18:02:56]

On 2013/06/21 16:26:18, dgrogan wrote:
> inferno, could you review this?
> 
> aedla and jsbell already reviewed but it needs an lgtm from a
> content/common/indexed_db/*_messages.h owner.

I rubber stamp LGTM based on @aedla's review. We should have @aedla added to
*messages* OWNERS :)

[commit-bot: I haz the power, 2013-06-21 18:03:48]

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

[commit-bot: I haz the power, 2013-06-21 18:17:00]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[dgrogan, 2013-06-21 23:28:31]

jamesr, I added an enum to third_party/WebKit/public/platform/WebIDBCallbacks.h
that is used in content/browser. Could you comment on whether the exception in
content/browser/DEPS is ok? Seems hacky but I think it follows the "strictly
enum..." guidelines.

[jamesr, 2013-06-21 23:44:47]

That's fine - WebIDBCallbacks.h has no symbols, so it's not an issue. lgtm
(although I don't have powers here).

having to get exceptions for all of these is going to be a pain, though. we've
talked about having a directory that has nothing but POD+enum+inline/pure
virtual directories.  of course, once we fix up the dependencies for
chrome/browser and content/browser patches that try to use symbols will just
fail to link, so maybe we just wait

[dgrogan, 2013-06-21 23:48:04]

sky@, could you rubberstamp this as owner of content/browser?

[Tom Sepez, 2013-06-24 17:28:47]

Messages LGTM (sorry to take so long, was out last week).

[commit-bot: I haz the power, 2013-06-24 18:41:33]

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

[dgrogan, 2013-06-24 23:43:40]

jam@, could you review content/browser/DEPS as an OWNER of content? jamesr
already gave it an lgt m

[jam, 2013-06-25 18:03:45]

On 2013/06/24 23:43:40, dgrogan wrote:
> jam@, could you review content/browser/DEPS as an OWNER of content? jamesr
> already gave it an lgt m

lgtm feel free to have TBRd a change like that in the future

[commit-bot: I haz the power, 2013-06-25 19:48:36]

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

[commit-bot: I haz the power, 2013-06-25 19:48:41]

Failed to apply patch for
content/browser/indexed_db/indexed_db_backing_store.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/indexed_db/indexed_db_backing_store.cc
  Hunk #1 FAILED at 421.
  Hunk #2 succeeded at 548 (offset -1 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
content/browser/indexed_db/indexed_db_backing_store.cc.rej

Patch:       content/browser/indexed_db/indexed_db_backing_store.cc
Index: content/browser/indexed_db/indexed_db_backing_store.cc
diff --git a/content/browser/indexed_db/indexed_db_backing_store.cc
b/content/browser/indexed_db/indexed_db_backing_store.cc
index
e3b6a43369b235807614276dd7c349a372e01ed1..a5f2f8db0198ced8179330a434d961c4b0635bf5
100644
--- a/content/browser/indexed_db/indexed_db_backing_store.cc
+++ b/content/browser/indexed_db/indexed_db_backing_store.cc
@@ -421,19 +421,26 @@ enum IndexedDBLevelDBBackingStoreOpenResult {
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::Open(
     const string16& database_identifier,
     const base::FilePath& path_base,
-    const string16& file_identifier) {
+    const string16& file_identifier,
+    WebKit::WebIDBCallbacks::DataLoss* data_loss) {
+  *data_loss = WebKit::WebIDBCallbacks::DataLossNone;
   DefaultLevelDBFactory leveldb_factory;
-  return IndexedDBBackingStore::Open(
-      database_identifier, path_base, file_identifier, &leveldb_factory);
+  return IndexedDBBackingStore::Open(database_identifier,
+                                     path_base,
+                                     file_identifier,
+                                     data_loss,
+                                     &leveldb_factory);
 }
 
 scoped_refptr<IndexedDBBackingStore> IndexedDBBackingStore::Open(
     const string16& database_identifier,
     const base::FilePath& path_base,
     const string16& file_identifier,
+    WebKit::WebIDBCallbacks::DataLoss* data_loss,
     LevelDBFactory* leveldb_factory) {
   IDB_TRACE("IndexedDBBackingStore::open");
   DCHECK(!path_base.empty());
+  *data_loss = WebKit::WebIDBCallbacks::DataLossNone;
 
   scoped_ptr<LevelDBComparator> comparator(new Comparator());
   scoped_ptr<LevelDBDatabase> db;
@@ -549,6 +556,7 @@ scoped_refptr<IndexedDBBackingStore>
IndexedDBBackingStore::Open(
     return scoped_refptr<IndexedDBBackingStore>();
   } else {
     LOG(ERROR) << "IndexedDB backing store open failed, attempting cleanup";
+    *data_loss = WebKit::WebIDBCallbacks::DataLossTotal;
     bool success = leveldb_factory->DestroyLevelDB(file_path);
     if (!success) {
       LOG(ERROR) << "IndexedDB backing store cleanup failed";

[commit-bot: I haz the power, 2013-06-25 20:06:00]

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

[dgrogan, 2013-06-25 23:40:53]

Committed patchset #13 manually as r208579 (presubmit successful).