[BlobStorage & IndexedDB] Manually refcount in-transit blobs for IDB put messages (issue 2828953002 by dmurph@chromium.org)

[dmu...@chromium.org]

Reviewers: jsbell, pwnall
CL: https://codereview.chromium.org/2828953002/

Message:
Victor & Josh, can you PTAL at how I do this? Is this worth the extra code /
complexity (especially if I clean it up a bit)?

Pros:
* It makes auto-blobing easier (necessary to refcount the auto-blob blob so it
survives transfer)
* It makes all blobs sent to IDB survive transfer

Cons:
* We will remove this when we make blobs a mojo service

I vote yes - although I can make it a little better w/ supporting ref'ing a list
of uuids.

Description:
[BlobStorage & IndexedDB] Manually refcount in-transit blobs for IDB put
messages

This is a proof-of-concept, let me know if you like this direction.

R=pwnall,jsbell
BUG=351753,710736,713409

Affected files (+48, -3 lines):
M content/browser/blob_storage/blob_dispatcher_host.h
M content/browser/blob_storage/blob_dispatcher_host.cc
M content/browser/indexed_db/database_impl.cc
M content/browser/indexed_db/indexed_db_dispatcher_host.h
M content/browser/indexed_db/indexed_db_dispatcher_host.cc
M content/browser/renderer_host/render_process_host_impl.cc
M third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp


Index: content/browser/blob_storage/blob_dispatcher_host.cc
diff --git a/content/browser/blob_storage/blob_dispatcher_host.cc b/content/browser/blob_storage/blob_dispatcher_host.cc
index bea813ddc5c4f7abdfc20eccc633918fb790b792..e21cf8b48b91071550f18e76db7cba7b1f0c4a45 100644
--- a/content/browser/blob_storage/blob_dispatcher_host.cc
+++ b/content/browser/blob_storage/blob_dispatcher_host.cc
@@ -70,6 +70,11 @@ BlobDispatcherHost::~BlobDispatcherHost() {
ClearHostFromBlobStorageContext();
}

+void BlobDispatcherHost::AckBlobRecievedFromIDB(const std::string& uuid) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ OnDecrementBlobRefCount(uuid);
+}
+
void BlobDispatcherHost::OnChannelClosing() {
ClearHostFromBlobStorageContext();
public_blob_urls_.clear();
Index: content/browser/blob_storage/blob_dispatcher_host.h
diff --git a/content/browser/blob_storage/blob_dispatcher_host.h b/content/browser/blob_storage/blob_dispatcher_host.h
index e1bfbe50411cf27437f81a3fa10c98ffbfbebeb1..ad5c9afd9e06a5f1cedb99f5c49a02f355a4fa70 100644
--- a/content/browser/blob_storage/blob_dispatcher_host.h
+++ b/content/browser/blob_storage/blob_dispatcher_host.h
@@ -47,6 +47,9 @@ class CONTENT_EXPORT BlobDispatcherHost : public BrowserMessageFilter {
scoped_refptr<ChromeBlobStorageContext> blob_storage_context,
scoped_refptr<storage::FileSystemContext> file_system_context);

+ // Decrements the refcount for the given blob for this renderer.
+ void AckBlobRecievedFromIDB(const std::string& uuid);
+
// BrowserMessageFilter implementation.
void OnChannelClosing() override;
bool OnMessageReceived(const IPC::Message& message) override;
Index: content/browser/indexed_db/database_impl.cc
diff --git a/content/browser/indexed_db/database_impl.cc b/content/browser/indexed_db/database_impl.cc
index beb3da9371d8e484f41f32bb1c974c43107f07b4..5fbda65c93ba134d2fc68e84ad39384ec2d2c85e 100644
--- a/content/browser/indexed_db/database_impl.cc
+++ b/content/browser/indexed_db/database_impl.cc
@@ -287,6 +287,7 @@ void DatabaseImpl::Put(
std::unique_ptr<storage::BlobDataHandle> handle =
dispatcher_host_->blob_storage_context()->GetBlobDataFromUUID(
info->uuid);
+ dispatcher_host_->AckBlobRecievedFromIDB(info->uuid);

// Due to known issue crbug.com/351753, blobs can die while being passed to
// a different process. So this case must be handled gracefully.
Index: content/browser/indexed_db/indexed_db_dispatcher_host.cc
diff --git a/content/browser/indexed_db/indexed_db_dispatcher_host.cc b/content/browser/indexed_db/indexed_db_dispatcher_host.cc
index 2ecd7b18d642e4973abb93ff24a562a8ec218617..d340a93e461676afc99dec8d851e9464eae5da73 100644
--- a/content/browser/indexed_db/indexed_db_dispatcher_host.cc
+++ b/content/browser/indexed_db/indexed_db_dispatcher_host.cc
@@ -12,6 +12,7 @@
#include "base/sequenced_task_runner.h"
#include "base/stl_util.h"
#include "base/strings/utf_string_conversions.h"
+#include "content/browser/blob_storage/blob_dispatcher_host.h"
#include "content/browser/indexed_db/indexed_db_callbacks.h"
#include "content/browser/indexed_db/indexed_db_connection.h"
#include "content/browser/indexed_db/indexed_db_context_impl.h"
@@ -75,6 +76,7 @@ IndexedDBDispatcherHost::IndexedDBDispatcherHost(
scoped_refptr<ChromeBlobStorageContext> blob_storage_context)
: indexed_db_context_(std::move(indexed_db_context)),
blob_storage_context_(std::move(blob_storage_context)),
+ blob_dispatcher_host_(nullptr),
idb_runner_(indexed_db_context_->TaskRunner()),
ipc_process_id_(ipc_process_id),
weak_factory_(this) {
@@ -109,6 +111,16 @@ void IndexedDBDispatcherHost::AddCursorBinding(
cursor_bindings_.AddBinding(std::move(cursor), std::move(request));
}

+void IndexedDBDispatcherHost::SetBlobDispatcherHost(
+ scoped_refptr<BlobDispatcherHost> host) {
+ blob_dispatcher_host_ = std::move(host);
+}
+
+void IndexedDBDispatcherHost::AckBlobRecievedFromIDB(const std::string& uuid) {
+ DCHECK(blob_dispatcher_host_);
+ blob_dispatcher_host_->AckBlobRecievedFromIDB(uuid);
+}
+
std::string IndexedDBDispatcherHost::HoldBlobData(
const IndexedDBBlobInfo& blob_info) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Index: content/browser/indexed_db/indexed_db_dispatcher_host.h
diff --git a/content/browser/indexed_db/indexed_db_dispatcher_host.h b/content/browser/indexed_db/indexed_db_dispatcher_host.h
index c64c71f5a9966d9f8271f203bf261d9c278b829d..bfcbca291bf137dbd3a5085e1d17a26c711ae6aa 100644
--- a/content/browser/indexed_db/indexed_db_dispatcher_host.h
+++ b/content/browser/indexed_db/indexed_db_dispatcher_host.h
@@ -33,6 +33,7 @@ class Origin;
}

namespace content {
+class BlobDispatcherHost;
class IndexedDBBlobInfo;
class IndexedDBContextImpl;

@@ -66,6 +67,11 @@ class CONTENT_EXPORT IndexedDBDispatcherHost
}
int ipc_process_id() const { return ipc_process_id_; }

+ void SetBlobDispatcherHost(scoped_refptr<BlobDispatcherHost> host);
+
+ // Decrements the refcount for the given blob for this renderer.
+ void AckBlobRecievedFromIDB(const std::string& uuid);
+
std::string HoldBlobData(const IndexedDBBlobInfo& blob_info);
void DropBlobData(const std::string& uuid);

@@ -109,6 +115,8 @@ class CONTENT_EXPORT IndexedDBDispatcherHost

scoped_refptr<IndexedDBContextImpl> indexed_db_context_;
scoped_refptr<ChromeBlobStorageContext> blob_storage_context_;
+ // TODO(dmurph): Remove once blobs are mojo'd.
+ scoped_refptr<BlobDispatcherHost> blob_dispatcher_host_;
scoped_refptr<base::SequencedTaskRunner> idb_runner_;

// Maps blob uuid to a pair (handle, ref count). Entry is added and/or count
Index: content/browser/renderer_host/render_process_host_impl.cc
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index e779da3670b2c0ce4a7944ebba76613d91d1be91..07d3c9d6ff37121cb285c9b088f019309712533c 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -1115,9 +1115,14 @@ void RenderProcessHostImpl::CreateMessageFilters() {
GetID(), storage_partition_impl_->GetURLRequestContext(),
storage_partition_impl_->GetFileSystemContext(),
blob_storage_context.get(), StreamContext::GetFor(browser_context)));
- AddFilter(new BlobDispatcherHost(
- GetID(), blob_storage_context,
- make_scoped_refptr(storage_partition_impl_->GetFileSystemContext())));
+
+ scoped_refptr<BlobDispatcherHost> blob_dispatcher_host =
+ new BlobDispatcherHost(
+ GetID(), blob_storage_context,
+ make_scoped_refptr(storage_partition_impl_->GetFileSystemContext()));
+ AddFilter(blob_dispatcher_host.get());
+ indexed_db_factory_->SetBlobDispatcherHost(std::move(blob_dispatcher_host));
+
AddFilter(new FileUtilitiesMessageFilter(GetID()));
AddFilter(
new DatabaseMessageFilter(storage_partition_impl_->GetDatabaseTracker()));
Index: third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
diff --git a/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp b/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
index 2022121aacc4b027b5b200a749a67e4ef3c6f685..7d9eca215848c614b4b1e83b87e054fa63e91579 100644
--- a/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
+++ b/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
@@ -43,7 +43,9 @@
#include "modules/indexeddb/IDBTracing.h"
#include "platform/Histogram.h"
#include "platform/SharedBuffer.h"
+#include "public/platform/Platform.h"
#include "public/platform/WebBlobInfo.h"
+#include "public/platform/WebBlobRegistry.h"
#include "public/platform/WebData.h"
#include "public/platform/WebVector.h"
#include "public/platform/modules/indexeddb/WebIDBKey.h"
@@ -506,6 +508,15 @@ IDBRequest* IDBObjectStore::put(ScriptState* script_state,
serialized_value->ToWireBytes(wire_bytes);
RefPtr<SharedBuffer> value_buffer = SharedBuffer::AdoptVector(wire_bytes);

+ // Add blob refs
+ blink::Platform* platform = blink::Platform::Current();
+ DCHECK(platform);
+ blink::WebBlobRegistry* registry = platform->GetBlobRegistry();
+ DCHECK(registry);
+ for (const WebBlobInfo& info : blob_info) {
+ registry->AddBlobDataRef(info.Uuid());
+ }
+
BackendDB()->Put(transaction_->Id(), Id(), WebData(value_buffer), blob_info,
key, static_cast<WebIDBPutMode>(put_mode),
request->CreateWebCallbacks().release(), index_ids,

[pwn...@chromium.org]

LGTM.

It seems to me that there isn't that much extra complexity (am I missing
something?), and this CL is fixing a lifetime bug.


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

https://codereview.chromium.org/2828953002/diff/1/content/browser/indexed_db/indexed_db_dispatcher_host.cc#newcode116
content/browser/indexed_db/indexed_db_dispatcher_host.cc:116:
blob_dispatcher_host_ = std::move(host);
Would it make sense to DCHECK that the host_ is nullptr? My intuition is
that this gets called exactly once for a host.

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

https://codereview.chromium.org/2828953002/diff/1/content/browser/indexed_db/indexed_db_dispatcher_host.h#newcode118
content/browser/indexed_db/indexed_db_dispatcher_host.h:118: //

TODO(dmurph): Remove once blobs are mojo'd.

Would it be worth pointing to this CL in the TODO? IIUC, all the
plumbing here could go away if mojo can hang onto the Blobs.

https://codereview.chromium.org/2828953002/

[mich...@chromium.org]

some drive by comments...


https://codereview.chromium.org/2828953002/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp
(right):

https://codereview.chromium.org/2828953002/diff/1/third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp#newcode517
third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:517:
registry->AddBlobDataRef(info.Uuid());
Manual refcounting always makes me a little uneasy (are their any cases
where the balancing decref is not called and where is that balancing
decref)

I think the serialized_value created on line 386 contains a map of
BlobDataHandles, so long as the handles in that map are retained the
blobs will not be deleted. Does the 'request' or 'callbacks' object
outlive put() completion? If so, maybe one of those could retain the
references w/o manual refcounting.

Also, is this change needed? At the time put() is called, there are
valid refs on any blobs in the value being put. Is there any chance the
release msg of those refs could beat the put msg sent on line 520? Maybe
we can't depend on the ordering of put vs release messages.

https://codereview.chromium.org/2828953002/

[dmu...@chromium.org]

Thanks for the idea Michael! That's way better. I need to add a test to the
IDBRequest to verify that the handles are cleared when OnSuccess(key) or
OnError() are called.

https://codereview.chromium.org/2828953002/

[mich...@chromium.org]

thnx! lgtm


https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp#newcode293
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:293:
ClearTransitBlobHandles();
There are many OnSuccess variants, no doubt this is the one that get's
called for Put(), maybe to ensure and make clear this method isn't
needed for the other variants, add a DCHECK(!transit_blobs) to
OnSucessInternal.

https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.h
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right):

https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.h#newcode146
third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:146: void
StorePutOperationBlobs(
nit: consider naming the StoreXXX and ClearXXX methods with the same
noun at the end

https://codereview.chromium.org/2828953002/

[dmu...@chromium.org]

https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp#newcode293
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:293:
ClearTransitBlobHandles();

On 2017/04/25 23:37:00, michaeln wrote:
> There are many OnSuccess variants, no doubt this is the one that get's
called
> for Put(), maybe to ensure and make clear this method isn't needed for
the other
> variants, add a DCHECK(!transit_blobs) to OnSucessInternal.

Done.

https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.h
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right):

https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Source/modules/indexeddb/IDBRequest.h#newcode146
third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:146: void
StorePutOperationBlobs(

On 2017/04/25 23:37:00, michaeln wrote:
> nit: consider naming the StoreXXX and ClearXXX methods with the same
noun at the
> end

Done.

https://codereview.chromium.org/2828953002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2828953002/60001

https://codereview.chromium.org/2828953002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ca74aee3773bb77867cf02d5f81725d10b33e4ab

https://codereview.chromium.org/2828953002/