Complete registration of blobs before sending back an indexed DB value that contains them.

content/browser/indexed_db/indexed_db_callbacks.cc

Index: content/browser/indexed_db/indexed_db_callbacks.cc
diff --git a/content/browser/indexed_db/indexed_db_callbacks.cc b/content/browser/indexed_db/indexed_db_callbacks.cc
index f4c348765b3495e32af1580d0b84e330629131c8..7bb629e86d191be05888683057443c0fb9090703 100644
--- a/content/browser/indexed_db/indexed_db_callbacks.cc
+++ b/content/browser/indexed_db/indexed_db_callbacks.cc
@@ -6,7 +6,14 @@
 
 #include <algorithm>
 
+#include "base/guid.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/time/time.h"
+#include "content/browser/child_process_security_policy_impl.h"
+#include "content/browser/fileapi/fileapi_message_filter.h"
+#include "content/browser/indexed_db/indexed_db_blob_info.h"
 #include "content/browser/indexed_db/indexed_db_connection.h"
+#include "content/browser/indexed_db/indexed_db_context_impl.h"
 #include "content/browser/indexed_db/indexed_db_cursor.h"
 #include "content/browser/indexed_db/indexed_db_database_callbacks.h"
 #include "content/browser/indexed_db/indexed_db_database_error.h"
@@ -14,7 +21,12 @@
 #include "content/browser/indexed_db/indexed_db_value.h"
 #include "content/common/indexed_db/indexed_db_constants.h"
 #include "content/common/indexed_db/indexed_db_messages.h"
+#include "webkit/browser/blob/blob_storage_context.h"
 #include "webkit/browser/quota/quota_manager.h"
+#include "webkit/common/blob/blob_data.h"
+#include "webkit/common/blob/shareable_file_reference.h"
+
+using webkit_blob::ShareableFileReference;
 
 namespace content {
 
@@ -169,6 +181,149 @@ void IndexedDBCallbacks::OnSuccess(scoped_ptr<IndexedDBConnection> connection,
   dispatcher_host_ = NULL;
 }
 
+static std::string CreateBlobData(
+    const IndexedDBBlobInfo& blob_info,
+    scoped_refptr<IndexedDBDispatcherHost> dispatcher_host,
+    webkit_blob::BlobStorageContext* blob_storage_context,
+    base::TaskRunner* task_runner) {
+  scoped_refptr<ShareableFileReference> shareable_file =
+      ShareableFileReference::Get(blob_info.file_path());

[cmumford, 2014/04/17 18:07:57]

Why do your own "Get" before calling "GetOrCreate"?

[ericu, 2014/04/17 21:36:29]

We only want the callback to get called once.  If there's already one attached,
that's enough.  If we call AddFinalReleaseCallback again, it'll get added to a
list of callbacks, rather than replacing the one that's there, and they'll all
get called.

+  if (!shareable_file.get()) {
+    shareable_file = ShareableFileReference::GetOrCreate(
+        blob_info.file_path(),
+        ShareableFileReference::DONT_DELETE_ON_FINAL_RELEASE,
+        task_runner);
+    shareable_file->AddFinalReleaseCallback(blob_info.release_callback());
+  }
+
+  std::string uuid(base::GenerateGUID());
+  scoped_refptr<webkit_blob::BlobData> blob_data =
+      new webkit_blob::BlobData(uuid);
+  blob_data->AppendFile(
+      blob_info.file_path(), 0, blob_info.size(), blob_info.last_modified());
+  scoped_ptr<webkit_blob::BlobDataHandle> blob_data_handle(
+      blob_storage_context->AddFinishedBlob(blob_data.get()));
+  dispatcher_host->HoldBlobDataHandle(uuid, blob_data_handle);
+
+  return uuid;
+}
+
+static bool CreateAllBlobs(
+    const std::vector<IndexedDBBlobInfo>& blob_info,
+    std::vector<IndexedDBMsg_BlobOrFileInfo>* blob_or_file_info,
+    scoped_refptr<IndexedDBDispatcherHost> dispatcher_host) {
+  size_t i;
+  if (!dispatcher_host->blob_storage_context())
+    return false;
+  for (i = 0; i < blob_info.size(); ++i) {
+    (*blob_or_file_info)[i].uuid =

[cmumford, 2014/04/17 18:07:57]

I see below the contract is that blob_or_file_info->size() == blob_info.size()
before calling this function? I'd put a DCHECK_EQ here to make it obvious that's
a precondition.

[ericu, 2014/04/17 21:36:29]

Done.

+        CreateBlobData(blob_info[i],
+                       dispatcher_host,
+                       dispatcher_host->blob_storage_context(),
+                       dispatcher_host->Context()->TaskRunner());
+  }
+  return true;
+}
+
+static void BlobLookupForIDBCursor(

[jsbell, 2014/04/17 18:22:22]

Can this be made a template function on <ParamsType, MsgType> ? It looks like
that would handle the IDBCursor, CursorContinue, ValueWithKey and Value cases.

[ericu, 2014/04/17 21:36:29]

Done.

+    IndexedDBMsg_CallbacksSuccessIDBCursor_Params* params,

[cmumford, 2014/04/17 18:07:57]

Can we make this param reference type to simplify?

[ericu, 2014/04/17 21:36:29]

I think that leads to an extra copy due to the bind.

+    scoped_refptr<IndexedDBDispatcherHost> dispatcher_host,
+    const std::vector<IndexedDBBlobInfo>& blob_info,
+    std::vector<IndexedDBMsg_BlobOrFileInfo>* blob_or_file_info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (CreateAllBlobs(blob_info, blob_or_file_info, dispatcher_host))
+    dispatcher_host->Send(new IndexedDBMsg_CallbacksSuccessIDBCursor(*params));
+}
+
+static void BlobLookupForCursorContinue(
+    IndexedDBMsg_CallbacksSuccessCursorContinue_Params* params,
+    scoped_refptr<IndexedDBDispatcherHost> dispatcher_host,
+    const std::vector<IndexedDBBlobInfo>& blob_info,
+    std::vector<IndexedDBMsg_BlobOrFileInfo>* blob_or_file_info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (CreateAllBlobs(blob_info, blob_or_file_info, dispatcher_host))
+    dispatcher_host->Send(
+        new IndexedDBMsg_CallbacksSuccessCursorContinue(*params));
+}
+
+static void BlobLookupForValueWithKey(
+    IndexedDBMsg_CallbacksSuccessValueWithKey_Params* params,
+    scoped_refptr<IndexedDBDispatcherHost> dispatcher_host,
+    const std::vector<IndexedDBBlobInfo>& blob_info,
+    std::vector<IndexedDBMsg_BlobOrFileInfo>* blob_or_file_info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (CreateAllBlobs(blob_info, blob_or_file_info, dispatcher_host))
+    dispatcher_host->Send(
+        new IndexedDBMsg_CallbacksSuccessValueWithKey(*params));
+}
+
+static void BlobLookupForValue(
+    IndexedDBMsg_CallbacksSuccessValue_Params* params,
+    scoped_refptr<IndexedDBDispatcherHost> dispatcher_host,
+    const std::vector<IndexedDBBlobInfo>& blob_info,
+    std::vector<IndexedDBMsg_BlobOrFileInfo>* blob_or_file_info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  if (CreateAllBlobs(blob_info, blob_or_file_info, dispatcher_host))
+    dispatcher_host->Send(new IndexedDBMsg_CallbacksSuccessValue(*params));
+}
+
+static void BlobLookupForCursorPrefetch(
+    IndexedDBMsg_CallbacksSuccessCursorPrefetch_Params* params,
+    scoped_refptr<IndexedDBDispatcherHost> dispatcher_host,
+    const std::vector<IndexedDBValue>& values,
+    std::vector<std::vector<IndexedDBMsg_BlobOrFileInfo> >*
+        blob_or_file_infos) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(values.size() == blob_or_file_infos->size());

[cmumford, 2014/04/17 18:07:57]

I'd dup/move this DCHECK to CreateAllBlobs. Also consider DCHECK_EQ.

[ericu, 2014/04/17 21:36:29]

CreateAllBlobs gets a single value at a time, not all of values.
Changed to DCHECK_EQ.

+
+  std::vector<IndexedDBValue>::const_iterator value_iter;
+  std::vector<std::vector<IndexedDBMsg_BlobOrFileInfo> >::iterator blob_iter;
+  for (value_iter = values.begin(), blob_iter = blob_or_file_infos->begin();
+       value_iter != values.end();
+       ++value_iter, ++blob_iter) {
+    if (!CreateAllBlobs(value_iter->blob_info, &*blob_iter, dispatcher_host))
+      return;
+  }
+  dispatcher_host->Send(
+      new IndexedDBMsg_CallbacksSuccessCursorPrefetch(*params));
+}
+
+static void FillInBlobData(
+    const std::vector<IndexedDBBlobInfo>& blob_info,
+    std::vector<IndexedDBMsg_BlobOrFileInfo>* blob_or_file_info) {
+  for (std::vector<IndexedDBBlobInfo>::const_iterator iter = blob_info.begin();
+       iter != blob_info.end();
+       ++iter) {
+    if (iter->is_file()) {
+      IndexedDBMsg_BlobOrFileInfo info;

[cmumford, 2014/04/17 18:07:57]

Where is uuid initialized?

[ericu, 2014/04/17 21:36:29]

In CreateAllBlobs, called from the closure that gets created right after the
call to FillInBlobData.

+      info.is_file = true;
+      info.mime_type = iter->type();
+      info.file_name = iter->file_name();
+      info.file_path = iter->file_path().AsUTF16Unsafe();
+      DCHECK_NE(-1, iter->size());
+      info.size = iter->size();
+      info.last_modified = iter->last_modified().ToDoubleT();
+      blob_or_file_info->push_back(info);
+    } else {
+      IndexedDBMsg_BlobOrFileInfo info;
+      info.mime_type = iter->type();
+      info.size = iter->size();
+      blob_or_file_info->push_back(info);
+    }
+  }
+}
+
+void IndexedDBCallbacks::RegisterBlobsAndSend(
+    const std::vector<IndexedDBBlobInfo>& blob_info,
+    const base::Closure& callback) {
+  std::vector<IndexedDBBlobInfo>::const_iterator iter;
+  for (iter = blob_info.begin(); iter != blob_info.end(); ++iter) {
+    iter->mark_used_callback().Run();
+  }
+  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO));
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, callback);
+}
+
 void IndexedDBCallbacks::OnSuccess(scoped_refptr<IndexedDBCursor> cursor,
                                    const IndexedDBKey& key,
                                    const IndexedDBKey& primary_key,
@@ -182,18 +337,29 @@ void IndexedDBCallbacks::OnSuccess(scoped_refptr<IndexedDBCursor> cursor,
   DCHECK_EQ(blink::WebIDBDataLossNone, data_loss_);
 
   int32 ipc_object_id = dispatcher_host_->Add(cursor.get());
-  IndexedDBMsg_CallbacksSuccessIDBCursor_Params params;
-  params.ipc_thread_id = ipc_thread_id_;
-  params.ipc_callbacks_id = ipc_callbacks_id_;
-  params.ipc_cursor_id = ipc_object_id;
-  params.key = key;
-  params.primary_key = primary_key;
+  scoped_ptr<IndexedDBMsg_CallbacksSuccessIDBCursor_Params> params(
+      new IndexedDBMsg_CallbacksSuccessIDBCursor_Params());
+  params->ipc_thread_id = ipc_thread_id_;
+  params->ipc_callbacks_id = ipc_callbacks_id_;
+  params->ipc_cursor_id = ipc_object_id;
+  params->key = key;
+  params->primary_key = primary_key;
   if (value && !value->empty())
-    std::swap(params.value, value->bits);
+    std::swap(params->value, value->bits);
   // TODO(alecflett): Avoid a copy here: the whole params object is
   // being copied into the message.
-  dispatcher_host_->Send(new IndexedDBMsg_CallbacksSuccessIDBCursor(params));
-
+  if (!value || value->blob_info.empty()) {

[jsbell, 2014/04/17 18:22:22]

Can callback messages ever end up re-ordered now?

e.g. if a success(value-with-blob) is invoked, followed by a
success(value-without-blob), could the latter ever end up sent over IPC first
due to the extra hop to the IO thread?

(IndexedDB requires that the order be maintained)

[ericu, 2014/04/17 21:36:29]

There's no extra hop.  The new code jumps over earlier, but they both end up
sending from the IO thread.  When you call Send, it checks to see which thread
you're on; if you're not on the IO thread, it posts a task across to continue. 
Ordering should still be guaranteed.

[jsbell, 2014/04/17 21:46:50]

Groovy. I'd thought it was sent on the IO thread then went digging, saw thread
handling code over in ipc/, got distracted by shiny things, and thought I'd just
assume the worst. :)

That said, seems like a good candidate for a layout test to add eventually. The
master patches are getting svelte!

+    dispatcher_host_->Send(new IndexedDBMsg_CallbacksSuccessIDBCursor(*params));
+  } else {
+    IndexedDBMsg_CallbacksSuccessIDBCursor_Params* p = params.get();
+    FillInBlobData(value->blob_info, &p->blob_or_file_info);
+    RegisterBlobsAndSend(value->blob_info,
+                         base::Bind(BlobLookupForIDBCursor,
+                                    base::Owned(params.release()),
+                                    dispatcher_host_,
+                                    value->blob_info,
+                                    base::Unretained(&p->blob_or_file_info)));
+  }
   dispatcher_host_ = NULL;
 }
 
@@ -214,25 +380,38 @@ void IndexedDBCallbacks::OnSuccess(const IndexedDBKey& key,
   DCHECK(idb_cursor);
   if (!idb_cursor)
     return;
-  IndexedDBMsg_CallbacksSuccessCursorContinue_Params params;
-  params.ipc_thread_id = ipc_thread_id_;
-  params.ipc_callbacks_id = ipc_callbacks_id_;
-  params.ipc_cursor_id = ipc_cursor_id_;
-  params.key = key;
-  params.primary_key = primary_key;
+
+  scoped_ptr<IndexedDBMsg_CallbacksSuccessCursorContinue_Params> params(
+      new IndexedDBMsg_CallbacksSuccessCursorContinue_Params());
+  params->ipc_thread_id = ipc_thread_id_;
+  params->ipc_callbacks_id = ipc_callbacks_id_;
+  params->ipc_cursor_id = ipc_cursor_id_;
+  params->key = key;
+  params->primary_key = primary_key;
   if (value && !value->empty())
-    std::swap(params.value, value->bits);
+    std::swap(params->value, value->bits);
   // TODO(alecflett): Avoid a copy here: the whole params object is
   // being copied into the message.
-  dispatcher_host_->Send(
-      new IndexedDBMsg_CallbacksSuccessCursorContinue(params));
+  if (!value || value->blob_info.empty()) {
+    dispatcher_host_->Send(
+        new IndexedDBMsg_CallbacksSuccessCursorContinue(*params));
+  } else {
+    IndexedDBMsg_CallbacksSuccessCursorContinue_Params* p = params.get();
+    FillInBlobData(value->blob_info, &p->blob_or_file_info);
+    RegisterBlobsAndSend(value->blob_info,
+                         base::Bind(BlobLookupForCursorContinue,
+                                    base::Owned(params.release()),
+                                    dispatcher_host_,
+                                    value->blob_info,
+                                    base::Unretained(&p->blob_or_file_info)));
+  }
   dispatcher_host_ = NULL;
 }
 
 void IndexedDBCallbacks::OnSuccessWithPrefetch(
     const std::vector<IndexedDBKey>& keys,
     const std::vector<IndexedDBKey>& primary_keys,
-    const std::vector<IndexedDBValue>& values) {
+    std::vector<IndexedDBValue>& values) {
   DCHECK_EQ(keys.size(), primary_keys.size());
   DCHECK_EQ(keys.size(), values.size());
 
@@ -252,17 +431,48 @@ void IndexedDBCallbacks::OnSuccessWithPrefetch(
     msgPrimaryKeys.push_back(primary_keys[i]);
   }
 
-  IndexedDBMsg_CallbacksSuccessCursorPrefetch_Params params;
-  params.ipc_thread_id = ipc_thread_id_;
-  params.ipc_callbacks_id = ipc_callbacks_id_;
-  params.ipc_cursor_id = ipc_cursor_id_;
-  params.keys = msgKeys;
-  params.primary_keys = msgPrimaryKeys;
-  std::vector<IndexedDBValue>::const_iterator iter;
-  for (iter = values.begin(); iter != values.end(); ++iter)
-    params.values.push_back(iter->bits);
-  dispatcher_host_->Send(
-      new IndexedDBMsg_CallbacksSuccessCursorPrefetch(params));
+  scoped_ptr<IndexedDBMsg_CallbacksSuccessCursorPrefetch_Params> params(
+      new IndexedDBMsg_CallbacksSuccessCursorPrefetch_Params());
+  params->ipc_thread_id = ipc_thread_id_;
+  params->ipc_callbacks_id = ipc_callbacks_id_;
+  params->ipc_cursor_id = ipc_cursor_id_;
+  params->keys = msgKeys;
+  params->primary_keys = msgPrimaryKeys;
+  std::vector<std::string>& values_bits = params->values;
+  values_bits.resize(values.size());
+  std::vector<std::vector<IndexedDBMsg_BlobOrFileInfo> >& values_blob_infos =
+    params->blob_or_file_infos;
+  values_blob_infos.resize(values.size());
+  std::vector<IndexedDBValue>::iterator iter = values.begin();

[jsbell, 2014/04/17 18:22:22]

nit: can you move this to just before the for() loop that uses it?

[ericu, 2014/04/17 21:36:29]

Done.

+
+  bool found_blob_info = false;
+  for (size_t i = 0; iter != values.end(); ++iter, ++i) {
+    values_bits[i].swap(iter->bits);
+    if (iter->blob_info.size()) {
+      found_blob_info = true;
+      FillInBlobData(iter->blob_info, &values_blob_infos[i]);
+      std::vector<IndexedDBBlobInfo>::const_iterator blob_iter;
+      for (blob_iter = iter->blob_info.begin();
+           blob_iter != iter->blob_info.end();
+           ++blob_iter) {
+        blob_iter->mark_used_callback().Run();
+      }
+    }
+  }
+
+  if (found_blob_info) {
+    BrowserThread::PostTask(

[cmumford, 2014/04/17 18:07:57]

Worthy of a DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::IO)); here too?

[ericu, 2014/04/17 21:36:29]

All of the OnSuccess methods are called from the IDB task runner; it seems
unnecessary to me.  I can add it to the beginning of each of them if you feel
strongly.

+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(BlobLookupForCursorPrefetch,
+                   base::Owned(params.release()),
+                   dispatcher_host_,
+                   values,
+                   base::Unretained(&params->blob_or_file_infos)));
+  } else {
+    dispatcher_host_->Send(
+        new IndexedDBMsg_CallbacksSuccessCursorPrefetch(*params.get()));
+  }
   dispatcher_host_ = NULL;
 }
 
@@ -277,34 +487,56 @@ void IndexedDBCallbacks::OnSuccess(IndexedDBValue* value,
   DCHECK_EQ(kNoDatabaseCallbacks, ipc_database_callbacks_id_);
   DCHECK_EQ(blink::WebIDBDataLossNone, data_loss_);
 
-  IndexedDBMsg_CallbacksSuccessValueWithKey_Params params;
-  params.ipc_thread_id = ipc_thread_id_;
-  params.ipc_callbacks_id = ipc_callbacks_id_;
-  params.primary_key = key;
-  params.key_path = key_path;
+  scoped_ptr<IndexedDBMsg_CallbacksSuccessValueWithKey_Params> params(
+      new IndexedDBMsg_CallbacksSuccessValueWithKey_Params());
+  params->ipc_thread_id = ipc_thread_id_;
+  params->ipc_callbacks_id = ipc_callbacks_id_;
+  params->primary_key = key;
+  params->key_path = key_path;
   if (value && !value->empty())
-    std::swap(params.value, value->bits);
-
-  dispatcher_host_->Send(new IndexedDBMsg_CallbacksSuccessValueWithKey(params));
+    std::swap(params->value, value->bits);
+  if (!value || value->blob_info.empty()) {
+    dispatcher_host_->Send(
+        new IndexedDBMsg_CallbacksSuccessValueWithKey(*params));
+  } else {
+    IndexedDBMsg_CallbacksSuccessValueWithKey_Params* p = params.get();
+    FillInBlobData(value->blob_info, &p->blob_or_file_info);
+    RegisterBlobsAndSend(value->blob_info,
+                         base::Bind(BlobLookupForValueWithKey,
+                                    base::Owned(params.release()),
+                                    dispatcher_host_,
+                                    value->blob_info,
+                                    base::Unretained(&p->blob_or_file_info)));
+  }
   dispatcher_host_ = NULL;
 }
 
 void IndexedDBCallbacks::OnSuccess(IndexedDBValue* value) {
   DCHECK(dispatcher_host_.get());
-
   DCHECK(kNoCursor == ipc_cursor_id_ || value == NULL);
   DCHECK_EQ(kNoTransaction, host_transaction_id_);
   DCHECK_EQ(kNoDatabase, ipc_database_id_);
   DCHECK_EQ(kNoDatabaseCallbacks, ipc_database_callbacks_id_);
   DCHECK_EQ(blink::WebIDBDataLossNone, data_loss_);
 
-  IndexedDBMsg_CallbacksSuccessValue_Params params;
-  params.ipc_thread_id = ipc_thread_id_;
-  params.ipc_callbacks_id = ipc_callbacks_id_;
+  scoped_ptr<IndexedDBMsg_CallbacksSuccessValue_Params> params(
+      new IndexedDBMsg_CallbacksSuccessValue_Params());
+  params->ipc_thread_id = ipc_thread_id_;
+  params->ipc_callbacks_id = ipc_callbacks_id_;
   if (value && !value->empty())
-    std::swap(params.value, value->bits);
-
-  dispatcher_host_->Send(new IndexedDBMsg_CallbacksSuccessValue(params));
+    std::swap(params->value, value->bits);
+  if (!value || value->blob_info.empty()) {
+    dispatcher_host_->Send(new IndexedDBMsg_CallbacksSuccessValue(*params));
+  } else {
+    IndexedDBMsg_CallbacksSuccessValue_Params* p = params.get();
+    FillInBlobData(value->blob_info, &p->blob_or_file_info);
+    RegisterBlobsAndSend(value->blob_info,
+                         base::Bind(BlobLookupForValue,
+                                    base::Owned(params.release()),
+                                    dispatcher_host_,
+                                    value->blob_info,
+                                    base::Unretained(&p->blob_or_file_info)));
+  }
   dispatcher_host_ = NULL;
 }