IndexedDB: Fixed cursor/blob use-after-free bug

IndexedDB: Fixed cursor/blob use-after-free bug

The IndexedDBDispatcherHost maintains a map of BLOB UUID's to BLOBs, but if two
(or more) cursors are both active and referencing the same BLOB then two (or
more) BLOBs would exist with the same UUID, and their keys would collide in this
map. This change reference counts these BLOBs to avoid duplication.

Also, access to the existing map was not synchronized and was accessed on two
different threads.

BUG=435880, 436137
Committed: https://crrev.com/2d74497dfa5e6fd6ddddc93248c322a57dd8dd2c
Cr-Commit-Position: refs/heads/master@{#307063}

[cmumford, 2014-12-03 23:27:55]

cmumford@chromium.org changed reviewers:
+ dgrogan@chromium.org, jsbell@chromium.org

[cmumford, 2014-12-03 23:27:55]

Blob implementation is a bit complex so requesting both of you take a look just
in case I missed something.

[jsbell, 2014-12-04 00:18:17]

Thanks for tracking this down! Two things:

1) Threading

HoldBlobDataHandle is called from the IO thread (in IndexedDBCallbacks) and
DropBlobDataHandle is called from the IDB task runner (in the
IndexedDBDispatcherHost), correct? 

Rather than introducing a lock, can we make the "Drop" calls happen on the IO
thread as well, either by posting to it or (even better) by tweaking
OverrideTaskRunnerForMessage so that we don't extra hop to the IDB thread then
back to IO?

(Ideally this would be documented with DCHECKs...)

2) Since BlobDataHandle is really just a refcount mechanism for BlobData, we
don't need to make HoldBlobDataHandle take actually take a BlobDataHandle...

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

https://codereview.chromium.org/774593004/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_callbacks.cc:231: if
(!dispatcher_host->IncrementBlobDataIfHeld(uuid)) {
Can we just make the HoldBlobDataHandle API take the uuid and mint a
BlobDataHandle if necessary?

https://codereview.chromium.org/774593004/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_callbacks.cc:254:
scoped_ptr<storage::BlobDataHandle> blob_data_handle(
Similarly, this BlobDataHandle is dropped if there's a matching UUID... but
since it's a new UUID that's never the case. So we could let the
HoldBlobDataHandle() do the work.

We could rename the methods to HoldBlobData(uuid)/DropBlobData(uuid)

[cmumford, 2014-12-04 18:00:30]

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

https://codereview.chromium.org/774593004/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_callbacks.cc:231: if
(!dispatcher_host->IncrementBlobDataIfHeld(uuid)) {
On 2014/12/04 00:18:16, jsbell wrote:
> Can we just make the HoldBlobDataHandle API take the uuid and mint a
> BlobDataHandle if necessary?

Done.

https://codereview.chromium.org/774593004/diff/1/content/browser/indexed_db/i...
content/browser/indexed_db/indexed_db_callbacks.cc:254:
scoped_ptr<storage::BlobDataHandle> blob_data_handle(
On 2014/12/04 00:18:16, jsbell wrote:
> Similarly, this BlobDataHandle is dropped if there's a matching UUID... but
> since it's a new UUID that's never the case. So we could let the
> HoldBlobDataHandle() do the work.
> 
> We could rename the methods to HoldBlobData(uuid)/DropBlobData(uuid)

Done.

[jsbell, 2014-12-04 18:13:42]

lgtm!

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

https://codereview.chromium.org/774593004/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:245:
std::pair<storage::BlobDataHandle*, int>(blob_data_handle.release(), 1);
Can you use std::make_pair here?

[cmumford, 2014-12-04 18:17:52]

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

https://codereview.chromium.org/774593004/diff/40001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:245:
std::pair<storage::BlobDataHandle*, int>(blob_data_handle.release(), 1);
On 2014/12/04 18:13:42, jsbell wrote:
> Can you use std::make_pair here?

Done.

[cmumford, 2014-12-04 20:19:50]

dgrogan@ PTAL - or rubber stamp if you're OK with jsbell's review.

[dgrogan, 2014-12-05 01:09:29]

lgtm

https://codereview.chromium.org/774593004/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/774593004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_callbacks.cc:225:
storage::BlobStorageContext* blob_storage_context,
Looks like you can ax this parameter.

[cmumford, 2014-12-05 19:08:09]

https://codereview.chromium.org/774593004/diff/60001/content/browser/indexed_...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/774593004/diff/60001/content/browser/indexed_...
content/browser/indexed_db/indexed_db_callbacks.cc:225:
storage::BlobStorageContext* blob_storage_context,
On 2014/12/05 01:09:29, dgrogan wrote:
> Looks like you can ax this parameter.

Done.

[cmumford, 2014-12-05 19:08:16]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-12-05 19:09:09]

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

[commit-bot: I haz the power, 2014-12-05 20:01:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-05 20:01:55]

Patchset 5 (id:??) landed as
https://crrev.com/2d74497dfa5e6fd6ddddc93248c322a57dd8dd2c
Cr-Commit-Position: refs/heads/master@{#307063}