IndexedDB: Prevent store/index deletion from racing ahead of use

IndexedDB: Prevent store/index deletion from racing ahead of use

In the Indexed DB spec, schema updates occur synchronously from the
perspective of script, e.g. you can create a store then use it
immediately without waiting for a request to complete. That model was
carried through to the back end, but with a subtle issue that the
deletion of a store or index could "race ahead" of a previously
requested use of it.

Change store/index deletion to take place in order with other
requests, so do all the work (in-memory and backing store) during the
scheduled operation, rather than on IPC receipt. Also change
store/index creation to do all the work (in-memory and backing store)
synchronously, since index population is already preemptively done.

And since this requires shuffling when the "abort" operations should
be queued, rework the API there slightly.

BUG=370056, 368271, 362723
R=cmumford@chromium.org,ericu@chromium.org

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

[jsbell, 2014-05-08 00:06:56]

To land this will require disabling a test that probes at this indirectly on the
Blink side. This CL changes Chrome's behavior to align with Firefox for that
test.

I want to try and add some unit tests around this before landing, but can you
take an initial look?

This is an intrusive change change, but the current behavior is definitely
broken; fortunately, it's only the kind of code you'd write for tests
(create/delete the same store/index within the same upgrade).

[jsbell, 2014-05-13 08:17:38]

I added some tests; nothing too exciting but they exercise the op and op/abort
paths.

cmumford@, ericu@ - PTAL?

[ericu, 2014-05-13 16:33:15]

Sorry about the delay.  It looks OK on the surface; I want to take a
little longer to look at the abort mechanism change.

On Tue, May 13, 2014 at 1:17 AM,  <jsbell@chromium.org> wrote:
> I added some tests; nothing too exciting but they exercise the op and
> op/abort
> paths.
>
> cmumford@, ericu@ - PTAL?
>
> https://codereview.chromium.org/277583002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cmumford, 2014-05-13 21:56:23]

Looks like storage/indexeddb/createIndex-after-failure.html is now failing.

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_transaction.cc:102: RunTasksIfStarted();
Will PostTask (run by RunTasksIfStarted) ever run the task right away - or will
it always post it to the queue? If so then might we have a case where code calls
ScheduleTask followed by ScheduleAbortTask and the abort isn't run?

[ericu, 2014-05-14 01:08:00]

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:331:
base::Bind(&IndexedDBDatabase::DeleteObjectStoreOperation,
This change makes the RemoveObjectStore call async.  Does that mean that one
could accidentally use an objectStore after deleting it, if the race condition
worked out right?  Or do we protect against that elsewhere?

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:755:
metadata_.object_stores.end());
Is using [] here the right fallback, or should we instead do a runtime check and
return if we don't find the object store?  What happens with the empty object
store we're creating in release mode in the case that we'd fail the DCHECK?

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:1338: metadata_.version =
kNoStringVersion;
Given that metadata_.version is obsolete, I don't see any docs on what it should
be here.  But this initialization to kNoStringVersion is new--just checking that
that's intentional.

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:256: // processed at
least one task before the CreateObjectStore call.
Why is that desirable?

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:260: void
RunPostedTasks() { message_loop_.RunUntilIdle(); }
MessageLoop::RunUntilIdle is deprecated.
All the cool kids use "base::RunLoop().RunUntilIdle();".

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:290: 
How about a test that captures the create-put-delete sequence that revealed the
DCHECK failure?

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_transaction.cc (right):

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_transaction.cc:102: RunTasksIfStarted();
On 2014/05/13 21:56:24, cmumford wrote:
> Will PostTask (run by RunTasksIfStarted) ever run the task right away - or
will
> it always post it to the queue? If so then might we have a case where code
calls
> ScheduleTask followed by ScheduleAbortTask and the abort isn't run?

No, PostTask always queues a task for asynchronous execution; it never runs the
task directly.

[jsbell, 2014-05-14 10:49:36]

On 2014/05/13 21:56:23, cmumford wrote:
> Looks like storage/indexeddb/createIndex-after-failure.html is now failing.
> 

Yes, the final 'get' changes from succeeding to failing, which matches Firefox.

[jsbell, 2014-05-14 11:05:52]

Thanks for taking a look. Further thoughts on splitting this up - or doing more
- are welcome!

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:331:
base::Bind(&IndexedDBDatabase::DeleteObjectStoreOperation,
On 2014/05/14 01:08:00, ericu wrote:
> This change makes the RemoveObjectStore call async.  Does that mean that one
> could accidentally use an objectStore after deleting it, if the race condition
> worked out right?  Or do we protect against that elsewhere?

That's the key bit of the patch.

Currently, script code can do:

store.put();
db.deleteObjectStore('store');

When this hits the back end, the put() validates the store's ID, then schedules
a PutOperation. Then the deleteObjectStore() *synchronously* deletes it from the
metadata, and schedules backing store cleanup. Then the PutOperation runs -
which only doesn't fail because the code uses `store = metadata.store_map[id]`
which magically generates a new entry in the map (thanks, STL!).

So the fix is to do the 'delete' in the correct order.

'create' is done early, since the indexing runs preemptively.

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database_unittest.cc (right):

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:256: // processed at
least one task before the CreateObjectStore call.
On 2014/05/14 01:08:00, ericu wrote:
> Why is that desirable?

The previous formulation of abort tasks was WEIRD. In pseudocode they had the
form:

updateMetadata();
schedule(updateBackingStore, revertMetadata);

So the async "undo" task did not actually correspond the async "do" task, it
corresponded to the thing done synchronously before the schedule() call. It
would be clearer as:

updateMetadata();
addAbortTask(revertMetadata); // if we need to abort, undo previous
scheduleTask(updateBackingStore); // asynchronously, do this

But... the guts of IndexedDBTransaction are still wired such that abort tasks
only run iff the transaction was "used", otherwise it's considered a no-op.
That's okay, since (1) the only abort tasks correspond to schema changes, (2)
schema changes are only allowed in version change transactions, and (3) version
change transactions always kick off with a real task.

IMHO that's worth simplifying, but as a different patch.

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:260: void
RunPostedTasks() { message_loop_.RunUntilIdle(); }
On 2014/05/14 01:08:00, ericu wrote:
> MessageLoop::RunUntilIdle is deprecated.
> All the cool kids use "base::RunLoop().RunUntilIdle();".

Will fix, thanks.

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database_unittest.cc:290: 
On 2014/05/14 01:08:00, ericu wrote:
> How about a test that captures the create-put-delete sequence that revealed
the
> DCHECK failure?

Will add one. (I have a blink layout test, but can probably do a unit test too.)

[jsbell, 2014-05-14 11:17:11]

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:755:
metadata_.object_stores.end());
On 2014/05/14 01:08:00, ericu wrote:
> Is using [] here the right fallback, or should we instead do a runtime check
and
> return if we don't find the object store?  What happens with the empty object
> store we're creating in release mode in the case that we'd fail the DCHECK?

The check is done at Put() time, so with this patch the DCHECK should actually
be valid. But for defense-in-depth, a runtime check is reasonable.

https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:1338: metadata_.version =
kNoStringVersion;
On 2014/05/14 01:08:00, ericu wrote:
> Given that metadata_.version is obsolete, I don't see any docs on what it
should
> be here.  But this initialization to kNoStringVersion is new--just checking
that
> that's intentional.

Intentional. It appeared to be missing, possibly a bug in the case where you
were upgrading a string-version to an int-version and it aborted, the metadata
might have not been rewound correctly. 

Maybe I should not touch the versionchange task/abort task stuff in this CL,
just to keep things simpler.

[ericu, 2014-05-14 18:08:22]

On Wed, May 14, 2014 at 4:05 AM,  <jsbell@chromium.org> wrote:
> Thanks for taking a look. Further thoughts on splitting this up - or doing
> more
> - are welcome!

I don't think it's too big to go in as-is.  Of course, my expectations
may be off due to the big CL I'm hoping to drop on you next week ;'>.

>
https://codereview.chromium.org/277583002/diff/40001/content/browser/indexed_...
> content/browser/indexed_db/indexed_db_database.cc:331:
> base::Bind(&IndexedDBDatabase::DeleteObjectStoreOperation,
> On 2014/05/14 01:08:00, ericu wrote:
>>
>> This change makes the RemoveObjectStore call async.  Does that mean
>
> that one
>>
>> could accidentally use an objectStore after deleting it, if the race
>
> condition
>>
>> worked out right?  Or do we protect against that elsewhere?
>
>
> That's the key bit of the patch.
>
> Currently, script code can do:
>
> store.put();
> db.deleteObjectStore('store');

Right--I got that.  What I'm wondering about is, after this change, I
might be able to get away with this obviously wrong code:

db.deleteObjectStore("store");
store.put("foo", "bar");

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jsbell, 2014-05-14 20:41:08]

On 2014/05/14 18:08:22, ericu wrote:
> Right--I got that.  What I'm wondering about is, after this change, I
> might be able to get away with this obviously wrong code:
> 
> db.deleteObjectStore("store");
> store.put("foo", "bar");

So that should be caught in two places:

(1) the Blink code should reject it in IDBObjectStore::put()'s isDeleted() check
since the metadata for store is deleted synchronously in Blink even before any
IPC is sent to the browser
(2) the Chromium IndexedDBDatabase::Put() should reject it c/o
ValidateObjectStoreId()

[ericu, 2014-05-14 20:47:02]

On Wed, May 14, 2014 at 1:41 PM,  <jsbell@chromium.org> wrote:
> On 2014/05/14 18:08:22, ericu wrote:
>>
>> Right--I got that.  What I'm wondering about is, after this change, I
>> might be able to get away with this obviously wrong code:
>
>
>> db.deleteObjectStore("store");
>> store.put("foo", "bar");
>
>
> So that should be caught in two places:
>
> (1) the Blink code should reject it in IDBObjectStore::put()'s isDeleted()
> check
> since the metadata for store is deleted synchronously in Blink even before
> any
> IPC is sent to the browser

OK, that's the kind of thing I was looking for.

> (2) the Chromium IndexedDBDatabase::Put() should reject it c/o
> ValidateObjectStoreId()

ValidateObjectStoreId depends on metadata_.object_stores, which is
updated by RemoveObjectStore, which is now async instead of
synchronous.

> https://codereview.chromium.org/277583002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jsbell, 2014-05-19 22:07:07]

More testing revealed a fun edge case.

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:430: return;
Seems safer to early exit here; if the backing store can't be updated, leave the
in-memory metadata in place. The abort will restore the metadata on the
front-end as well.

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:1305: return;
Ditto.

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.h (right):

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.h:181: int64 object_store_id,
Metadata should be snapshotted asynchronously, so ID is sufficient here.

c/o a race found by running transaction-basics.html a 2500 times; would DCHECK
fail about 0.5% of the time. The failure case was 
deleteIndex/deleteObjectStore/abort - the abort would first restore the
objectStore but with metadata including the index, and second restoring the
index - *boom*.

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.h:194: int64 index_id,
Metadata should be snapshotted asynchronously, so ID is sufficient here.

[cmumford, 2014-05-20 15:29:32]

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:1289: const
IndexedDBObjectStoreMetadata object_store_metadata =
Can this be a const reference?

[jsbell, 2014-05-20 15:34:43]

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/277583002/diff/80001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_database.cc:1289: const
IndexedDBObjectStoreMetadata object_store_metadata =
On 2014/05/20 15:29:33, cmumford wrote:
> Can this be a const reference?

No, because metadata_ is mutated by RemoveObjectStore() and a pristine copy is
needed for the AbortTask.

[jsbell, 2014-05-20 20:39:13]

FYI, blink-side tests are at: https://codereview.chromium.org/291243003

[jsbell, 2014-05-21 17:46:24]

Any other feedback on this?

(Or anything that I missed from previous feedback?)

[cmumford, 2014-05-21 17:57:33]

lgtm

[ericu, 2014-05-21 19:51:28]

lgtm

[jsbell, 2014-05-22 16:18:15]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-05-22 16:19:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jsbell@chromium.org/277583002/80001

[commit-bot: I haz the power, 2014-05-22 19:42:43]

Change committed as 272282