|
|
Created:
3 years, 8 months ago by pwnall Modified:
3 years, 7 months ago CC:
Mads Ager (chromium), blink-reviews, chromium-reviews, cmumford, DmitrySkiba, jsbell+idb_chromium.org, kinuko+fileapi, kinuko+watch, kouhei+heap_chromium.org, Mircea Trofin, nhiroki, oilpan-reviews, tzik Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWrap large IndexedDB values into Blobs before writing to LevelDB.
Gist: IndexedDB values are currently stored in LevelDB according to the
serialization logic in SerializedScriptValue (SSV). After this CL, an
SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB
stores a reference to the Blob.
"small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB
"large" v8::Value -> SSV -> IDBValue (SSV output) -> Blob w/ SSV output
-> IDBValue (Blob reference) -> LevelDB
On the write side, the IndexedDB (IDB) value serialization logic is
extracted from IDBObjectStore::put() into a separate class,
IDBValueWrapper. This is mostly to avoid further swelling
IDBObjectStore::put(), which is already rather large. IDBValueWrapper
knows how to turn an IDBValue into a SerializedScriptValue (SSV), and
how to wrap the SSV into a Blob, if necessary.
The read-side changes are more complex due to the following:
1) IDB request results are laziliy deserialized from IDBValue, because
the SSV deserialization logic is expensive. The deserialization is done
synchronously, but Blob data can only be fetched asynchronously. So,
IDB values must be unwrapped from Blobs before the SSV logic is
invoked.
2) The events for IDB request results must be dispatched in the order in
which the requests were issued. If an IDB result's SSV requires
Blob-unwrapping, all the following results must be queued until the
Blob is unwrapped and the result's event can be delivered.
The queueing is handled by (sigh) a layer of indirection. Previously,
WebIDBCallbacksImpl called straight into IDBRequest::EnqueueResult().
This CL routes all callbacks that can be queued into
IDBRequest::HandleResult() overloads that match the
IDBRequest::EnqueueResult() signatures. When a result needs to be queued
(either due to Blob-unwrapping, or because it arrives after another
queued result), its data is saved in a IDBRequestQueueItem. These queued
results are tracked by a per-transaction IDBRequestQueue.
When a IDBValue contains a Blob reference, the Blob's content is
asynchronously fetched by IDBRequestLoader. Afterwards,
IDBValueUnwrapper turns the Blob data into an IDBValue that can be
de-serialized by the SerializedScriptValue logic.
BUG=713409
TBR=haraken
Review-Url: https://codereview.chromium.org/2822453003
Cr-Commit-Position: refs/heads/master@{#474874}
Committed: https://chromium.googlesource.com/chromium/src/+/9f25693ac827e8ea87699023e640760fd09ef9bb
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed compilation errors on Windows and no-DCHECKs. #
Total comments: 50
Patch Set 3 : Attempt to fix compile error. #Patch Set 4 : Rebase + start addressing feedback. #
Total comments: 20
Patch Set 5 : Addressed feedback. #Patch Set 6 : WIP: Getting IDBRequestTest.EventsAfterStopping to pass. #
Total comments: 81
Patch Set 7 : Rebased. #Patch Set 8 : Addressed round 2 of feedback. #Patch Set 9 : Rebased. #
Total comments: 41
Patch Set 10 : Test fixes and most of round 3 of feedback. #Patch Set 11 : WTF Bind #Patch Set 12 : Finished up round 3 of feedback. #Patch Set 13 : Fixed Windows compilation. #
Total comments: 14
Patch Set 14 : Addressed last round of feedback. #Messages
Total messages: 130 (100 generated)
dmurph@chromium.org changed reviewers: + dmurph@chromium.org
https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:77: Blob* wrapper = Ok so this is how we'll deal with the lifetime. This whole class should know about ALL blobs, whether they are in the original value or if they are the new wrapped blob (interesting question about what happens if we have a big value that also has a blob - but I think it should work out?) 1. We temporarily store this object in our wrapper just to keep it from destructing before this object is destructed. 2. We add this blob's uuid to blob_info_. 3. We have a method called AddRefToBlobs() that goes through blob_info_ and adds a ref using.. probably a new IPC (meh) or just calling webblobregistry_impl's addref method for each. 4. In the caller (IDBObjectStore.cpp) we call wrap, then call AddRefToBlobs, before sending it over the wire. 5. On the browser side, in DatabaseImpl's Put method (https://cs.chromium.org/chromium/src/content/browser/indexed_db/database_impl...) we grab the BlobDispatcherHost for the current renderer (a little tricky - this will require some wiring) and dec the ref there for each blob that was transfered AFTER the put operation is scheduled - Or maybe as something to happen after the transaction? We have to dec the ref after we copy/write the blob to a file. Does this make sense?
On 2017/04/13 18:05:00, dmurph wrote: > https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/m... > File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp (right): > > https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/m... > third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:77: Blob* > wrapper = > Ok so this is how we'll deal with the lifetime. This whole class should know > about ALL blobs, whether they are in the original value or if they are the new > wrapped blob (interesting question about what happens if we have a big value > that also has a blob - but I think it should work out?) > > 1. We temporarily store this object in our wrapper just to keep it from > destructing before this object is destructed. > 2. We add this blob's uuid to blob_info_. > 3. We have a method called AddRefToBlobs() that goes through blob_info_ and adds > a ref using.. probably a new IPC (meh) or just calling webblobregistry_impl's > addref method for each. > 4. In the caller (IDBObjectStore.cpp) we call wrap, then call AddRefToBlobs, > before sending it over the wire. > 5. On the browser side, in DatabaseImpl's Put method > (https://cs.chromium.org/chromium/src/content/browser/indexed_db/database_impl...) > we grab the BlobDispatcherHost for the current renderer (a little tricky - this > will require some wiring) and dec the ref there for each blob that was > transfered AFTER the put operation is scheduled - Or maybe as something to > happen after the transaction? We have to dec the ref after we copy/write the > blob to a file. > > Does this make sense? I am definitely on board with the general plan. Our discussion yesterday was the main thing that inspired me to go fully fledged class to do wrapping, rather than try to inline it at the right place in IDBObjectStore::put() :) One thing that I don't quite understand is what is happening today to the Blobs that hang off of a value passed to ::put(). I know they'll be guaranteed alive until serialization ends, because the v8::Value in the SerializedScriptValue keeps them alive. I don't know what keeps them alive between the moment they're serialized and the moment IDBRequest::OnSuccess() is called. I'll bug you with Blob lifecycle questions some more after I spend some time playing with the patch. Thank you for bearing with the early code!
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Patchset #2 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Description was changed from ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. BUG= ========== to ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. Gist: IndexedDB values are currently stored in LevelDB according to the serialization logic in SerializedScriptValue (SSV). After this CL, an SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB stores a reference to the Blob. "small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB "large" v8::Value -> SSV -> Blob -> IDBValue (Blob reference) -> LevelDB On the write side, the IndexedDB (IDB) value serialization logic is extracted from IDBObjectStore::put() into a separate class, IDBValueWrapper. This is mostly to avoid further swelling IDBObjectStore::put(), which is already rather large. IDBValueWrapper knows how to turn an IDBValue into a SerializedScriptValue (SSV), and how to wrap the SSV into a Blob, if necessary. The read-side changes are more complex due to the following: 1) IDB request results are laziliy deserialized from IDBValue, because the SSV deserialization logic is expensive. The deserialization is done synchronously, but Blob data can only be fetched asynchronously. So, IDB values must be unwrapped from Blobs before the SSV logic is invoked. 2) The events for IDB request results must be dispatched in the order in which the requests were issued. If an IDB result's SSV requires Blob-unwrapping, all the following results must be queued until the Blob is unwrapped and the result's event can be delivered. The queueing is handled by (sigh) a layer of indirection. Previously, WebIDBCallbacksImpl called straight into IDBRequest::OnSuccess() and IDBRequest::OnError(). This CL routes all callbacks that can be queued into IDBRequest::QueueResult() overloads that match the IDBRequest:OnSuccess() and IDBRequest::OnError() signatures. When a result needs to be queued (either due to Blob-unwrapping, or because it arrives after another queued result), its data is saved in a IDBRequestQueueItem. These queued results are tracked by a per-transaction IDBRequestQueue. When a IDBValue contains a Blob reference, the Blob's content is asynchronously fetched by IDBRequestLoader. Afterwards, IDBValueUnwrapper turns the Blob data into an IDBValue that can be de-serialized by the SerializedScriptValue logic. BUG=713409 ==========
Description was changed from ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. Gist: IndexedDB values are currently stored in LevelDB according to the serialization logic in SerializedScriptValue (SSV). After this CL, an SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB stores a reference to the Blob. "small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB "large" v8::Value -> SSV -> Blob -> IDBValue (Blob reference) -> LevelDB On the write side, the IndexedDB (IDB) value serialization logic is extracted from IDBObjectStore::put() into a separate class, IDBValueWrapper. This is mostly to avoid further swelling IDBObjectStore::put(), which is already rather large. IDBValueWrapper knows how to turn an IDBValue into a SerializedScriptValue (SSV), and how to wrap the SSV into a Blob, if necessary. The read-side changes are more complex due to the following: 1) IDB request results are laziliy deserialized from IDBValue, because the SSV deserialization logic is expensive. The deserialization is done synchronously, but Blob data can only be fetched asynchronously. So, IDB values must be unwrapped from Blobs before the SSV logic is invoked. 2) The events for IDB request results must be dispatched in the order in which the requests were issued. If an IDB result's SSV requires Blob-unwrapping, all the following results must be queued until the Blob is unwrapped and the result's event can be delivered. The queueing is handled by (sigh) a layer of indirection. Previously, WebIDBCallbacksImpl called straight into IDBRequest::OnSuccess() and IDBRequest::OnError(). This CL routes all callbacks that can be queued into IDBRequest::QueueResult() overloads that match the IDBRequest:OnSuccess() and IDBRequest::OnError() signatures. When a result needs to be queued (either due to Blob-unwrapping, or because it arrives after another queued result), its data is saved in a IDBRequestQueueItem. These queued results are tracked by a per-transaction IDBRequestQueue. When a IDBValue contains a Blob reference, the Blob's content is asynchronously fetched by IDBRequestLoader. Afterwards, IDBValueUnwrapper turns the Blob data into an IDBValue that can be de-serialized by the SerializedScriptValue logic. BUG=713409 ========== to ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. Gist: IndexedDB values are currently stored in LevelDB according to the serialization logic in SerializedScriptValue (SSV). After this CL, an SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB stores a reference to the Blob. "small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB "large" v8::Value -> SSV -> IDBValue (SSV output) -> Blob w/ SSV output -> IDBValue (Blob reference) -> LevelDB On the write side, the IndexedDB (IDB) value serialization logic is extracted from IDBObjectStore::put() into a separate class, IDBValueWrapper. This is mostly to avoid further swelling IDBObjectStore::put(), which is already rather large. IDBValueWrapper knows how to turn an IDBValue into a SerializedScriptValue (SSV), and how to wrap the SSV into a Blob, if necessary. The read-side changes are more complex due to the following: 1) IDB request results are laziliy deserialized from IDBValue, because the SSV deserialization logic is expensive. The deserialization is done synchronously, but Blob data can only be fetched asynchronously. So, IDB values must be unwrapped from Blobs before the SSV logic is invoked. 2) The events for IDB request results must be dispatched in the order in which the requests were issued. If an IDB result's SSV requires Blob-unwrapping, all the following results must be queued until the Blob is unwrapped and the result's event can be delivered. The queueing is handled by (sigh) a layer of indirection. Previously, WebIDBCallbacksImpl called straight into IDBRequest::OnSuccess() and IDBRequest::OnError(). This CL routes all callbacks that can be queued into IDBRequest::QueueResult() overloads that match the IDBRequest:OnSuccess() and IDBRequest::OnError() signatures. When a result needs to be queued (either due to Blob-unwrapping, or because it arrives after another queued result), its data is saved in a IDBRequestQueueItem. These queued results are tracked by a per-transaction IDBRequestQueue. When a IDBValue contains a Blob reference, the Blob's content is asynchronously fetched by IDBRequestLoader. Afterwards, IDBValueUnwrapper turns the Blob data into an IDBValue that can be de-serialized by the SerializedScriptValue logic. BUG=713409 ==========
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Patchset #3 (id:180001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:160001) has been deleted
pwnall@chromium.org changed reviewers: + jsbell@chromium.org
jsbell: Can you please review the CL (including architecture / general approach)? dmurph: Can you please look at the Blob handling in IDBValueWrapper, IDBValueUnwrapper, and IDBRequestLoader?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Some initial comments. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:127: function cloningTest(label, descriptors) { descriptors -> valueDescriptors https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (left): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:244: ClearPutOperationBlobs(); Would we put these in the QueueResult callsa as well? We just need error and onsuccess(key). https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (left): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:178: void ClearPutOperationBlobs() { transit_blob_handles_.clear(); } Requests can take forever for a GC, so please try to actively clean them up (see the places this was called that you removed). https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:199: HashMap<String, RefPtr<BlobDataHandle>> transit_blob_handles_; Any reason for the name change? I like 'transit' better as it specifically means the transit system (mojo/IPC/whatever), outgoing is a little less specific. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:139: inline void ExtractOutgoingBlobHandlesFrom(IDBValueWrapper* value_wrapper) { I'd prefer we don't include and depend on the value wrapper api for this specific method call - what if we just made the blob handles accessible and mutable? *HashMap<String, RefPtr<BlobDataHandle>> OutgoingBlobHandles() https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.cpp:84: loader_ = new IDBRequestLoader(this, &values_); The loader will definitely be only loading one blob, right? or, one value, right? Or I guess it could be wrapping a list of values - is that more accurate? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp:90: bool IDBValueUnwrapper::ReadVarint(unsigned& value) { There must be a library method for this... maybe it's only here? https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_le... https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:77: if (wire_bytes_.size() <= max_bytes) Check the accessible buffer instead of wire bytes, before serializing to wire bytes. Then we don't have to do that for every value :P https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:82: Blob* wrapper = Add a TODO & bug to use CreateBuilder in WebBlobRegistry https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebBl... To create the blob, as this does another memcpy. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:86: wrapper_handle_ = std::move(wrapper->GetBlobDataHandle()); Do we need this to be a separate handle? Can we just add this to blob_info_? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:93: request_->OnSuccess(string_list); Can you dcheck that we don't have any queued results on the request? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:117: } Can you dcheck that we don't have any queued results on the request? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:152: request_->OnSuccess(value); Can you dcheck that we don't have any queued results on the request? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:160: request_->OnSuccess(); Can you dcheck that we don't have any queued results on the request? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:176: return; Can you dcheck that we don't have any queued results on the request? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:187: std::unique_ptr<WebIDBDatabase> db = WTF::WrapUnique(database); Can you dcheck that we don't have any queued results on the request?
Sorry for an incomplete/scattered review. Initial notes, mostly nits. I'll continue the review tomorrow. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:112: virtual void OnError(DOMException*); WDYT about making most of the OnXXXX methods protected/private and friend the QueueItem? (Also, I don't think these all need to be virtual any more?) https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:122: virtual void OnSuccess(); Doesn't this one need a QueueResult() ? if not, can we delete it? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:173: // i.e. delete requests and unsuccessful open requests. And completed open requests, IIRC? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp:97: DOMException::Create(kWebIDBDatabaseExceptionDataError, This should just use kDataError (the kWebIDB... is just for stuff crossing the blink public API boundary) https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h:29: // to perform post-processing on the results (e.g., large value unwrapping). nit: no , https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h:33: class IDBRequestQueueItem Filename should be IDBRequestQueueItem ? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h:34: : public GarbageCollectedFinalized<IDBRequestQueueItem> { Why GC'd? I would imagine this would be owned by the queue. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h:63: // complete synchronously, in which case IDBtransaction::OnResultReady() will nit: IDBTransaction https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:381: void IDBTransaction::UnregisterRequest(IDBRequest* request) { Seems like we could have a DCHECK here that the request doesn't have a queued result.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Next batch of comments https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:383: IDBValueWrapper value_wrapper(isolate, value.V8Value(), write_wasm_to_stream, Is the motivation for introducing the wrapper here -- instead of just before the Backend()->Put() call -- for the future when more detailed serialization options are introduced, e.g. how to treat array buffers, etc? (For this patch on its own it seems weird to wrap at this point and add IDBValueWrapper::Clone) https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:269: if (!transaction_->HasQueuedResult() && !needs_unwrapping) What do you think about making queuing non-conditional? I.e. always queue, and the queue is responsible for deciding whether or not to call OnSuccess synchronously or unwrap and attach/start a loader? Or, if we want to avoid the expense of creating the IDBRequestQueueItem, at least consolidating the lines following it into EnqueueResult() (i.e. pass in needs_unwrapping, and EnqueueResult calls AttachLoader/StartLoading) https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:114: virtual void OnSuccess(const Vector<String>&); As you probably noticed, this overload is for webkitGetDatabaseNames() which is not associated with a transaction. Feel free to add a comment and/or move this line. (Hopefully removing that in M60 but I'm still watching the metrics...) https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp:24: if (!value->data_ || value->data_->size() < 3) Constants/documentation for these magic numbers. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp:29: return header[0] == kVersionTag && header[1] == 17 && header[2] == 1; Ditto. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h:9: #include "platform/wtf/PassRefPtr.h" We prefer std::move() of RefPtr now. (Trade off between matching surrounding code vs. new style. Whatever ends up being more readable for now.) https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h:27: // Wrapper for an IndexedDB value. More details? https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h:29: // The serialization process can throw an exception. The caller is responsible Comment doesn't apply to unwrapper https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h:91: #endif // IDBValueWrapper_h nit: missing "Un" https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:70: << "WrapIfBiggerThan() called on wrapper with serialization exception"; tip: could use __FUNCTION__ to make this more refactor-friendly.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:280001) has been deleted
Patchset #5 (id:300001) has been deleted
Patchset #5 (id:320001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmurph, jsbell: Thank you very much for the quick and thorough feedback! Unfortunately, I wasn't nearly as quick addressing all the points. Can you please take another look? https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:77: Blob* wrapper = On 2017/04/13 18:05:00, dmurph wrote: > Ok so this is how we'll deal with the lifetime. This whole class should know > about ALL blobs, whether they are in the original value or if they are the new > wrapped blob (interesting question about what happens if we have a big value > that also has a blob - but I think it should work out?) > > 1. We temporarily store this object in our wrapper just to keep it from > destructing before this object is destructed. > 2. We add this blob's uuid to blob_info_. > 3. We have a method called AddRefToBlobs() that goes through blob_info_ and adds > a ref using.. probably a new IPC (meh) or just calling webblobregistry_impl's > addref method for each. > 4. In the caller (IDBObjectStore.cpp) we call wrap, then call AddRefToBlobs, > before sending it over the wire. > 5. On the browser side, in DatabaseImpl's Put method > (https://cs.chromium.org/chromium/src/content/browser/indexed_db/database_impl...) > we grab the BlobDispatcherHost for the current renderer (a little tricky - this > will require some wiring) and dec the ref there for each blob that was > transfered AFTER the put operation is scheduled - Or maybe as something to > happen after the transaction? We have to dec the ref after we copy/write the > blob to a file. > > Does this make sense? You implemented a solution for keeping Blobs alive, so I don't need to worry about it too much. Thank you! https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:127: function cloningTest(label, descriptors) { On 2017/05/04 22:27:07, dmurph wrote: > descriptors -> valueDescriptors Done. I also added a comment here that briefly explains what the test does. Should help someone debugging a test failure. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (left): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:244: ClearPutOperationBlobs(); On 2017/05/04 22:27:07, dmurph wrote: > Would we put these in the QueueResult callsa as well? We just need error and > onsuccess(key). I called clear() directly on the vector. It seemed to me that a method name doesn't add semantic information over "outgoing_blob_handles_.clear()" ("transfer_blob_handles_.clear()", if you prefer). https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (left): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:178: void ClearPutOperationBlobs() { transit_blob_handles_.clear(); } On 2017/05/04 22:27:07, dmurph wrote: > Requests can take forever for a GC, so please try to actively clean them up (see > the places this was called that you removed). I think I was doing that. See the "outgoing_blob_handles_.clear();" calls in QueueResult(IDBKey) and QueueResult(DOMException). AFAIK these are the only possible callbacks for put. FWIW, I didn't remove the DCHECK from OnSuccessInternal(), so if I missed something, it isn't covered by tests :) What's the best way to express that the cleanup is still taking place? Put the method back in? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:199: HashMap<String, RefPtr<BlobDataHandle>> transit_blob_handles_; On 2017/05/04 22:27:07, dmurph wrote: > Any reason for the name change? I like 'transit' better as it specifically means > the transit system (mojo/IPC/whatever), outgoing is a little less specific. Done. (switched it back) "transit" seemed generic to me. Its biggest flaw IMO is that it doesn't disclose the direction in which data flows. After my CL, IDBRequest's handling of both outgoing and incoming Blobs will be non-trivial, so I would've liked to clearly distinguish between the two. FWIW. I thought that "outgoing" implies "in transit", so I thought the rename is a strict upgrade (more semantics stuffed in, still one word). I'm not a native speaker though, so ¯\_(ツ)_/¯ https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:112: virtual void OnError(DOMException*); On 2017/05/05 00:29:37, jsbell wrote: > WDYT about making most of the OnXXXX methods protected/private and friend the > QueueItem? > > (Also, I don't think these all need to be virtual any more?) Done. Can't say no to de-virtualizing :) https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:122: virtual void OnSuccess(); On 2017/05/05 00:29:37, jsbell wrote: > Doesn't this one need a QueueResult() ? if not, can we delete it? Done. Thank you very much for catching this!! FWIW, couldn't trigger OnSuccess() by iterating a cursor to the end of an index. I did trigger it by issuing a get() for a non-existing key. OnSuccess(int64_t) also needs to go through the queue, because it's used for count(). I updated the patch. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:139: inline void ExtractOutgoingBlobHandlesFrom(IDBValueWrapper* value_wrapper) { On 2017/05/04 22:27:07, dmurph wrote: > I'd prefer we don't include and depend on the value wrapper api for this > specific method call - what if we just made the blob handles accessible and > mutable? > > *HashMap<String, RefPtr<BlobDataHandle>> OutgoingBlobHandles() Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:173: // i.e. delete requests and unsuccessful open requests. On 2017/05/05 00:29:37, jsbell wrote: > And completed open requests, IIRC? Done. Case added. The "i.e." there was intended to say I'm only providing examples. The list is non-exhaustive, because I don't know all the cases off the top of my head :) https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp:97: DOMException::Create(kWebIDBDatabaseExceptionDataError, On 2017/05/05 00:29:37, jsbell wrote: > This should just use kDataError (the kWebIDB... is just for stuff crossing the > blink public API boundary) Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.cpp:84: loader_ = new IDBRequestLoader(this, &values_); On 2017/05/04 22:27:07, dmurph wrote: > The loader will definitely be only loading one blob, right? or, one value, > right? Or I guess it could be wrapping a list of values - is that more accurate? IDBObjectStore.getAll() returns an array of IDBValues, so the loader needs to potentially unwrap multiple values in that case. To make my life easier, I made the loader work with a vector, and the vector just happens to have exactly one value for many overloads. There are two questions here that you might be implicitly asking. 1) Is the vector overload negligible, compared to blob-unwrapping? I think so, but I haven't checked. 2) Would it make sense to unwrap values in parallel, rather than sequentially? Maybe. We'd need a global queue, to make sure we don't unwrap a million blobs at the same time. The queue might already exist in FileReader, because it needs throttling against script abuse, anyway. Even if we want parallel reading, it seems like a great candidate for a follow-up CL. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h:29: // to perform post-processing on the results (e.g., large value unwrapping). On 2017/05/05 00:29:37, jsbell wrote: > nit: no , Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h:33: class IDBRequestQueueItem On 2017/05/05 00:29:37, jsbell wrote: > Filename should be IDBRequestQueueItem ? Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h:34: : public GarbageCollectedFinalized<IDBRequestQueueItem> { On 2017/05/05 00:29:37, jsbell wrote: > Why GC'd? I would imagine this would be owned by the queue. Done. I was being lazy and avoiding learning about Persistent<>. I switched to std::unique_ptr and Persistent<> here and in IDBRequestLoader. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueue.h:63: // complete synchronously, in which case IDBtransaction::OnResultReady() will On 2017/05/05 00:29:37, jsbell wrote: > nit: IDBTransaction Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:381: void IDBTransaction::UnregisterRequest(IDBRequest* request) { On 2017/05/05 00:29:37, jsbell wrote: > Seems like we could have a DCHECK here that the request doesn't have a queued > result. Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp:90: bool IDBValueUnwrapper::ReadVarint(unsigned& value) { On 2017/05/04 22:27:07, dmurph wrote: > There must be a library method for this... maybe it's only here? > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_le... As far as I know, there isn't. Please correct me if I'm wrong, so I don't have to write tests :) The code you pointed to in the browser. The varint encoding/decoding in the renderer that I know of is in SSV, but we moved it from Blink to V8. This code is lifted & adapted from V8. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:77: if (wire_bytes_.size() <= max_bytes) On 2017/05/04 22:27:07, dmurph wrote: > Check the accessible buffer instead of wire bytes, before serializing to wire > bytes. Then we don't have to do that for every value :P I still need to serialize, even if no wrapping happens. ExtractWireBytes() is called unconditionally, and it needs to return what gets written to LevelDB. I copied & adapted some of the CL's commit message to the IDBValueWrapper class docs. Does this help clarify things? https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:82: Blob* wrapper = On 2017/05/04 22:27:07, dmurph wrote: > Add a TODO & bug to use CreateBuilder in WebBlobRegistry > https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebBl... > > To create the blob, as this does another memcpy. Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:86: wrapper_handle_ = std::move(wrapper->GetBlobDataHandle()); On 2017/05/04 22:27:07, dmurph wrote: > Do we need this to be a separate handle? Can we just add this to blob_info_? IIUC, WebBlobInfo is a struct that wraps the fields that I'm passing in here. It doesn't manage a browser-side reference to the Blob, like BlobDataHandle does. So I need to hang onto the BlobDataHandle here and return it in ExtractBlobDataHandles(), so IDBObjectStore::put() can pass it to the backend. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp (right): https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:93: request_->OnSuccess(string_list); On 2017/05/04 22:27:07, dmurph wrote: > Can you dcheck that we don't have any queued results on the request? Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:117: } On 2017/05/04 22:27:07, dmurph wrote: > Can you dcheck that we don't have any queued results on the request? Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:152: request_->OnSuccess(value); On 2017/05/04 22:27:07, dmurph wrote: > Can you dcheck that we don't have any queued results on the request? Per jsbell's comment, this one's actually going through the queue right now. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:160: request_->OnSuccess(); On 2017/05/04 22:27:07, dmurph wrote: > Can you dcheck that we don't have any queued results on the request? Per jsbell's comment, this one's actually going through the queue right now. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:176: return; On 2017/05/04 22:27:07, dmurph wrote: > Can you dcheck that we don't have any queued results on the request? Done. https://codereview.chromium.org/2822453003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/WebIDBCallbacksImpl.cpp:187: std::unique_ptr<WebIDBDatabase> db = WTF::WrapUnique(database); On 2017/05/04 22:27:07, dmurph wrote: > Can you dcheck that we don't have any queued results on the request? Done. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:383: IDBValueWrapper value_wrapper(isolate, value.V8Value(), write_wasm_to_stream, On 2017/05/08 22:08:01, jsbell wrote: > Is the motivation for introducing the wrapper here -- instead of just before the > Backend()->Put() call -- for the future when more detailed serialization options > are introduced, e.g. how to treat array buffers, etc? > > (For this patch on its own it seems weird to wrap at this point and add > IDBValueWrapper::Clone) Yes. I wanted to pull out all the serialization concerns so this large method doesn't have to change when we do new fancy things with WASM modules and perhaps typed arrays. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:269: if (!transaction_->HasQueuedResult() && !needs_unwrapping) On 2017/05/08 22:08:01, jsbell wrote: > What do you think about making queuing non-conditional? I.e. always queue, and > the queue is responsible for deciding whether or not to call OnSuccess > synchronously or unwrap and attach/start a loader? > > Or, if we want to avoid the expense of creating the IDBRequestQueueItem, at > least consolidating the lines following it into EnqueueResult() (i.e. pass in > needs_unwrapping, and EnqueueResult calls AttachLoader/StartLoading) Done. Per in-person discussion, went for the 2nd option. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:114: virtual void OnSuccess(const Vector<String>&); On 2017/05/08 22:08:01, jsbell wrote: > As you probably noticed, this overload is for webkitGetDatabaseNames() which is > not associated with a transaction. Feel free to add a comment and/or move this > line. > > (Hopefully removing that in M60 but I'm still watching the metrics...) Yes, but thank you for checking! This is a non-trivial behavior change, and I can use all the extra pairs of eyes I can get :) I added a comment regarding webkitGetDatabaseNames() in the renaming CL. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp:24: if (!value->data_ || value->data_->size() < 3) On 2017/05/08 22:08:01, jsbell wrote: > Constants/documentation for these magic numbers. Done. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.cpp:29: return header[0] == kVersionTag && header[1] == 17 && header[2] == 1; On 2017/05/08 22:08:01, jsbell wrote: > Ditto. Done. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h:9: #include "platform/wtf/PassRefPtr.h" On 2017/05/08 22:08:01, jsbell wrote: > We prefer std::move() of RefPtr now. (Trade off between matching surrounding > code vs. new style. Whatever ends up being more readable for now.) Mostly done. I also removed the PassRefPtr instances I had introduced in IDBRequest. I couldn't remove AdoptRef in IDBValueUnwrapper::CreateUnwrapped(). I kept hitting a DCHECK for !adoption_is_required_ at RefCounted.h:46. I skimmed through RefCounted / RefPtr / PassRefPtr, and I couldn't find an adoption mechanism that doesn't rely on PassRefPtr. Sigh. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h:27: // Wrapper for an IndexedDB value. On 2017/05/08 22:08:01, jsbell wrote: > More details? Done. I added more details to the top of the class. I think that explains the constructor. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h:29: // The serialization process can throw an exception. The caller is responsible On 2017/05/08 22:08:01, jsbell wrote: > Comment doesn't apply to unwrapper Done. Thanks for catching! https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueUnwrapper.h:91: #endif // IDBValueWrapper_h On 2017/05/08 22:08:01, jsbell wrote: > nit: missing "Un" Done. https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp (right): https://codereview.chromium.org/2822453003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapper.cpp:70: << "WrapIfBiggerThan() called on wrapper with serialization exception"; On 2017/05/08 22:08:01, jsbell wrote: > tip: could use __FUNCTION__ to make this more refactor-friendly. Done. Thank you very much for the tip!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
This looks really good. I'm going to take another pass this evening, I didn't really see anything glaring. What do you think about moving logic out of the queue item - possibly make it a struct - and into the txn and loader? I'm not sure if this is necessary. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:126: void IDBRequestQueueItem::MarkReady() { Can we rename this NotifyTransactionResultReady? Or something else that says we are basically telling the transaction that the result is ready?
On 2017/05/15 19:32:23, dmurph wrote: > What do you think about moving logic out of the queue item - possibly make it a > struct - and into the txn and loader? I'm not sure if this is necessary. FWIW, the CL started out that way -- the loader had all the callback logic embedded in it. I prefer to have it in the queue item because of SRP. I think of it as bound callback (perhaps similar to base::Callback? i haven't written much browser-side code, so I don't really know for sure), so it's responsibility is to call the right method in IDBRequest. Conversely, the loader's responsibility is to bring up the right requests. From a different angle, the loader is going to grow in complexity, as it'll have to handle WASM situations as well. So I think it's advantageous to keep the accidental complexity (callback logic) out, so we only have to deal with the mandatory complexity of preloading / mmapping everything that needs to be loaded / mmaped. If these arguments aren't appealing enough, please let me know, and I'll do the change. > https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp > (right): > > https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:126: void > IDBRequestQueueItem::MarkReady() { > Can we rename this NotifyTransactionResultReady? Or something else that says we > are basically telling the transaction that the result is ready?
https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:126: void IDBRequestQueueItem::MarkReady() { On 2017/05/15 19:32:23, dmurph wrote: > Can we rename this NotifyTransactionResultReady? Or something else that says we > are basically telling the transaction that the result is ready? I didn't like this name either, and I left it there to see if it's just me. Thanks! I'll think of something better.
https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:3: <title>IndexedDB: large nested objects are cloned correctly</title> aside: I wonder if we should create a subdir for wpt tests that are probing limits (e.g. simultaneous cursor behavior, key/value sizes, etc). possible follow-up for a public discussion. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:9: <script> Since this test involves multiple sequential transactions it's likely to be slow - might need the meta timeout tag. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:12: // Should be large enough to trigger value wrapping in the IndexedDB engines Maybe explain "wrapping" or say "that have special handling for large values" https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:88: for (let property of Object.getOwnPropertyNames(descriptor)) { If you're feeling fancy, you could do this as: return Promise.all(Object.getOwnPropertyNames(descriptor).map(p => checkValue(...))); https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:97: assert_class_string(value, 'Blob', 'incorrect value type'); nit: Word the message in terms of the *expected* outcome, e.g. 'value should be a Blob' or just the property being compared e.g. 'Blob type' https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:133: // The test verifies that the get() values match the arguments to put() and that This tests get() but not getAll() https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:139: const store = database.createObjectStore('test-store', { keyPath: null }); Why specify keyPath: null which is the default? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/request-event-ordering.html (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/request-event-ordering.html:17: function largeValue(size, seed) { Move this to support.js or support-promises.js ? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/request-event-ordering.html:70: continue nextOperation; This label doesn't appear necessary https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:256: transit_blob_handles_.clear(); Why is this cleared here but not in all HandleResponse methods? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:282: bool needs_unwrapping = IDBRequestLoader::NeedsUnwrapping(value.Get()); Maybe use the method on IDBValueWrapper to reduce dependencies on IDBRequestLoader? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:303: bool needs_unwrapping = IDBRequestLoader::NeedUnwrapping(values); Maybe move NeedsUnwrapping to IDBValueWrapper to reduce dependencies on IDBRequestLoader? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:102: void HandleResponse(DOMException*); Please add a comment covering the general behavior of HandleResponse, i.e. these will either queue an event for dispatch immediately if possible, or go into the transaction's queue waiting on request loader work needed for this response or a previous request's response. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:111: void HandleResponse(int64_t); Since it's non-obvious, add the parameter name here. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:112: void HandleResponse(); At one point we had documentation in the .h files about which IDB methods these result types correspond to. I would not object to adding them back. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:166: inline bool TransactionHasQueuedResults() const { Why did you decide to make this debug-only? Seems like it would be useful in the HandleResult methods? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:199: // Calls into OnError() and OnSuccess(). Comment is obsolete https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:256: // Non-null while this request's is queued behind other requests that are nit: grammar: 'request is' https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp:75: << "The reader returned more data than we were prepared for"; nit: maybe log expected/actual ? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:25: // IndexedDB requests result in a single value, getAll() in IDBObjectStore and Should we add a TODO to handle fetches of multiple values in parallel batches? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:42: static bool NeedsUnwrapping(IDBValue*); If we drop the use of these methods then IDBRequestQueueItem is the only user of this class. It could move to being an nested class, but it's big enough that it's own file is probably better. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:75: Vector<RefPtr<IDBValue>>* const values_; Comment on who owns this. (it's implied in the constructor, but wouldn't hurt to say it here too) https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:60: if (attach_loader) Should we DCHECK_EQ(attach_loader, NeedsUnwrapping(value)) ? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:75: if (attach_loader) Should we DCHECK_EQ(attach_loader, NeedUnwrapping(values)) ? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:74: void FireCallback(); Nit about naming: 'Fire' has a specific DOM meaning - synchronous event dispatch. So we should avoid that term here since this really causes an event to be enqueued. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:87: enum Mode { Since 'mode' is a spec term/IDL enum (for transaction mode), maybe ResponseType ? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:88: kBackendKeyPrimaryKeyValue, I'd call this kCursorKeyPrimaryKeyValue https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:90: kInt64Value, kNumber ? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:93: kNoArguments, kVoid ? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:120: std::unique_ptr<WebIDBCursor> backend_; 'cursor' instead of 'backend' here? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:115: // While this is true, new results must be queued using QueueResult(). nit: EnqueueResult https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:228: // The IDBRequest results whose events have not fired yet. nit: enqueued, not fired https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:38: // It follows that SSV will never produce byte sequences starting with 0xFF, Can you run this past jbroman@ as a sanity check? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:49: // 2) varint length-prefixed ASCII string - Blob UUID This is where I unhelpfully wonder if just storing the 16 byte UUID data is better than storing 37 bytes (1 byte length = 0x24 + 36 ASCII char serialization) https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:217: // TODO(pwnall): Blobs seem to get different UUIDs when they're resurrected? Did you verify this or ... ? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h:32: // "Normal" V8 values are stored on disk using the format implemented in nit: this first sentence is true whether we wrap or not. Rephrase? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h:57: // value. Add "... to avoid side effects." (We didn't spec it for fun!)
Description was changed from ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. Gist: IndexedDB values are currently stored in LevelDB according to the serialization logic in SerializedScriptValue (SSV). After this CL, an SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB stores a reference to the Blob. "small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB "large" v8::Value -> SSV -> IDBValue (SSV output) -> Blob w/ SSV output -> IDBValue (Blob reference) -> LevelDB On the write side, the IndexedDB (IDB) value serialization logic is extracted from IDBObjectStore::put() into a separate class, IDBValueWrapper. This is mostly to avoid further swelling IDBObjectStore::put(), which is already rather large. IDBValueWrapper knows how to turn an IDBValue into a SerializedScriptValue (SSV), and how to wrap the SSV into a Blob, if necessary. The read-side changes are more complex due to the following: 1) IDB request results are laziliy deserialized from IDBValue, because the SSV deserialization logic is expensive. The deserialization is done synchronously, but Blob data can only be fetched asynchronously. So, IDB values must be unwrapped from Blobs before the SSV logic is invoked. 2) The events for IDB request results must be dispatched in the order in which the requests were issued. If an IDB result's SSV requires Blob-unwrapping, all the following results must be queued until the Blob is unwrapped and the result's event can be delivered. The queueing is handled by (sigh) a layer of indirection. Previously, WebIDBCallbacksImpl called straight into IDBRequest::OnSuccess() and IDBRequest::OnError(). This CL routes all callbacks that can be queued into IDBRequest::QueueResult() overloads that match the IDBRequest:OnSuccess() and IDBRequest::OnError() signatures. When a result needs to be queued (either due to Blob-unwrapping, or because it arrives after another queued result), its data is saved in a IDBRequestQueueItem. These queued results are tracked by a per-transaction IDBRequestQueue. When a IDBValue contains a Blob reference, the Blob's content is asynchronously fetched by IDBRequestLoader. Afterwards, IDBValueUnwrapper turns the Blob data into an IDBValue that can be de-serialized by the SerializedScriptValue logic. BUG=713409 ========== to ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. Gist: IndexedDB values are currently stored in LevelDB according to the serialization logic in SerializedScriptValue (SSV). After this CL, an SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB stores a reference to the Blob. "small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB "large" v8::Value -> SSV -> IDBValue (SSV output) -> Blob w/ SSV output -> IDBValue (Blob reference) -> LevelDB On the write side, the IndexedDB (IDB) value serialization logic is extracted from IDBObjectStore::put() into a separate class, IDBValueWrapper. This is mostly to avoid further swelling IDBObjectStore::put(), which is already rather large. IDBValueWrapper knows how to turn an IDBValue into a SerializedScriptValue (SSV), and how to wrap the SSV into a Blob, if necessary. The read-side changes are more complex due to the following: 1) IDB request results are laziliy deserialized from IDBValue, because the SSV deserialization logic is expensive. The deserialization is done synchronously, but Blob data can only be fetched asynchronously. So, IDB values must be unwrapped from Blobs before the SSV logic is invoked. 2) The events for IDB request results must be dispatched in the order in which the requests were issued. If an IDB result's SSV requires Blob-unwrapping, all the following results must be queued until the Blob is unwrapped and the result's event can be delivered. The queueing is handled by (sigh) a layer of indirection. Previously, WebIDBCallbacksImpl called straight into IDBRequest::EnqueueResult(). This CL routes all callbacks that can be queued into IDBRequest::HandleResult() overloads that match the IDBRequest::EnqueueResult() signatures. When a result needs to be queued (either due to Blob-unwrapping, or because it arrives after another queued result), its data is saved in a IDBRequestQueueItem. These queued results are tracked by a per-transaction IDBRequestQueue. When a IDBValue contains a Blob reference, the Blob's content is asynchronously fetched by IDBRequestLoader. Afterwards, IDBValueUnwrapper turns the Blob data into an IDBValue that can be de-serialized by the SerializedScriptValue logic. BUG=713409 ==========
Patchset #8 (id:400001) has been deleted
Patchset #8 (id:420001) has been deleted
Patchset #8 (id:440001) has been deleted
jsbell, dmurph: PTAL? I'll make sure all tests pass after the following CLs land: https://chromium-review.googlesource.com/c/509150/ https://chromium-review.googlesource.com/c/508479/ https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:3: <title>IndexedDB: large nested objects are cloned correctly</title> On 2017/05/15 23:37:37, jsbell wrote: > aside: I wonder if we should create a subdir for wpt tests that are probing > limits (e.g. simultaneous cursor behavior, key/value sizes, etc). possible > follow-up for a public discussion. That sounds like a very reasonable proposal. So, we'd have something like conformance tests that cover each piece of the spec (99% of the current tests), and quality tests that cover scaling or bugs found and fixed in existing engines? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:9: <script> On 2017/05/15 23:37:37, jsbell wrote: > Since this test involves multiple sequential transactions it's likely to be slow > - might need the meta timeout tag. Done. Both tests can probably use that tag. Thank you! https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:12: // Should be large enough to trigger value wrapping in the IndexedDB engines On 2017/05/15 23:37:37, jsbell wrote: > Maybe explain "wrapping" or say "that have special handling for large values" Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:88: for (let property of Object.getOwnPropertyNames(descriptor)) { On 2017/05/15 23:37:37, jsbell wrote: > If you're feeling fancy, you could do this as: > > return Promise.all(Object.getOwnPropertyNames(descriptor).map(p => > checkValue(...))); Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:97: assert_class_string(value, 'Blob', 'incorrect value type'); On 2017/05/15 23:37:37, jsbell wrote: > nit: Word the message in terms of the *expected* outcome, e.g. 'value should be > a Blob' or just the property being compared e.g. 'Blob type' > Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:133: // The test verifies that the get() values match the arguments to put() and that On 2017/05/15 23:37:37, jsbell wrote: > This tests get() but not getAll() Done. Thanks, I don't know how I managed to miss that. I added getAll() coverage to both tests, and fixed a tiny bug that they found :( https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:139: const store = database.createObjectStore('test-store', { keyPath: null }); On 2017/05/15 23:37:37, jsbell wrote: > Why specify keyPath: null which is the default? Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/request-event-ordering.html (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/request-event-ordering.html:17: function largeValue(size, seed) { On 2017/05/15 23:37:37, jsbell wrote: > Move this to support.js or support-promises.js ? Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/request-event-ordering.html:70: continue nextOperation; On 2017/05/15 23:37:37, jsbell wrote: > This label doesn't appear necessary Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:256: transit_blob_handles_.clear(); On 2017/05/15 23:37:37, jsbell wrote: > Why is this cleared here but not in all HandleResponse methods? This is only set in put() requests, which (IIUC) can result in a key or an error. So I only clear the handles here and in HandleResponse(DOMException). Also, QueueResultInternal() DCHECKs that transit_blob_handles_ is empty. Would things be clearer if I moved the DCHECK to all HandleResponse instances that don't call transit_blob_handles_.clear()? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:282: bool needs_unwrapping = IDBRequestLoader::NeedsUnwrapping(value.Get()); On 2017/05/15 23:37:37, jsbell wrote: > Maybe use the method on IDBValueWrapper to reduce dependencies on > IDBRequestLoader? Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:303: bool needs_unwrapping = IDBRequestLoader::NeedUnwrapping(values); On 2017/05/15 23:37:38, jsbell wrote: > Maybe move NeedsUnwrapping to IDBValueWrapper to reduce dependencies on > IDBRequestLoader? Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:102: void HandleResponse(DOMException*); On 2017/05/15 23:37:38, jsbell wrote: > Please add a comment covering the general behavior of HandleResponse, i.e. these > will either queue an event for dispatch immediately if possible, or go into the > transaction's queue waiting on request loader work needed for this response or a > previous request's response. Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:111: void HandleResponse(int64_t); On 2017/05/15 23:37:38, jsbell wrote: > Since it's non-obvious, add the parameter name here. Done. I didn't add the parameter name before because it's ambiguous -- it can either be a result for count() or an old database version number for openDatabase(). (there might be other meanings, I haven't fully absorbed the browser-side IndexedDB implementation) https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:112: void HandleResponse(); On 2017/05/15 23:37:38, jsbell wrote: > At one point we had documentation in the .h files about which IDB methods these > result types correspond to. I would not object to adding them back. I agree! Can I do that in a follow-up CL? Making sure that I cover every callback requires a bit of work, and I'd rather not hold this CL up (and having to keep rebasing it). https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:166: inline bool TransactionHasQueuedResults() const { On 2017/05/15 23:37:38, jsbell wrote: > Why did you decide to make this debug-only? Seems like it would be useful in the > HandleResult methods? This method does an extra check -- transaction_ != nullptr. After some struggles getting IDBRequestTest.EventsAfterStopping to pass, I'm starting to think that all the methods might need to handle the nullptr case. If that's the case, I'll switch to using this method. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:199: // Calls into OnError() and OnSuccess(). On 2017/05/15 23:37:38, jsbell wrote: > Comment is obsolete Done. Updated the method name. Thank you for catching this! https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:256: // Non-null while this request's is queued behind other requests that are On 2017/05/15 23:37:38, jsbell wrote: > nit: grammar: 'request is' Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp:75: << "The reader returned more data than we were prepared for"; On 2017/05/15 23:37:38, jsbell wrote: > nit: maybe log expected/actual ? AFAIK the DCHECK_xx variants log their arguments. Example: STDERR: [1:1:0515/193007.830163:1049509374697:FATAL:IDBRequestLoader.cpp(74)] Check failed: wrapped_data_.size() + data_length + 100000 <= wrapped_data_.capacity() (165536 vs. 147424)The reader returned more data than we were prepared for Is this what you were looking for, or should I log something else? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:25: // IndexedDB requests result in a single value, getAll() in IDBObjectStore and On 2017/05/15 23:37:38, jsbell wrote: > Should we add a TODO to handle fetches of multiple values in parallel batches? Done. I added it in the .cpp file. The loader API will not change, so readers who only intend to cover the header shouldn't see the TODO. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:42: static bool NeedsUnwrapping(IDBValue*); On 2017/05/15 23:37:38, jsbell wrote: > If we drop the use of these methods then IDBRequestQueueItem is the only user of > this class. It could move to being an nested class, but it's big enough that > it's own file is probably better. Ack. Also, the loader will grow in size and complexity when we implement special treatment for WASM chunks. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:75: Vector<RefPtr<IDBValue>>* const values_; On 2017/05/15 23:37:38, jsbell wrote: > Comment on who owns this. (it's implied in the constructor, but wouldn't hurt to > say it here too) Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:60: if (attach_loader) On 2017/05/15 23:37:38, jsbell wrote: > Should we DCHECK_EQ(attach_loader, NeedsUnwrapping(value)) ? The loader DCHECKs that the values given to it need unwrapping. Is that sufficient? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:75: if (attach_loader) On 2017/05/15 23:37:38, jsbell wrote: > Should we DCHECK_EQ(attach_loader, NeedUnwrapping(values)) ? Same as above :) https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:126: void IDBRequestQueueItem::MarkReady() { On 2017/05/15 23:15:00, pwnall wrote: > On 2017/05/15 19:32:23, dmurph wrote: > > Can we rename this NotifyTransactionResultReady? Or something else that says > we > > are basically telling the transaction that the result is ready? > > I didn't like this name either, and I left it there to see if it's just me. > > Thanks! I'll think of something better. How about OnResultLoadComplete()? https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:74: void FireCallback(); On 2017/05/15 23:37:38, jsbell wrote: > Nit about naming: 'Fire' has a specific DOM meaning - synchronous event > dispatch. So we should avoid that term here since this really causes an event to > be enqueued. Done. Renamed to EnqueueResponse(), to match the method name in IDBRequest. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:87: enum Mode { On 2017/05/15 23:37:38, jsbell wrote: > Since 'mode' is a spec term/IDL enum (for transaction mode), maybe ResponseType > ? Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:88: kBackendKeyPrimaryKeyValue, On 2017/05/15 23:37:38, jsbell wrote: > I'd call this kCursorKeyPrimaryKeyValue Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:90: kInt64Value, On 2017/05/15 23:37:38, jsbell wrote: > kNumber ? Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:93: kNoArguments, On 2017/05/15 23:37:38, jsbell wrote: > kVoid ? Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:120: std::unique_ptr<WebIDBCursor> backend_; On 2017/05/15 23:37:38, jsbell wrote: > 'cursor' instead of 'backend' here? Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:115: // While this is true, new results must be queued using QueueResult(). On 2017/05/15 23:37:38, jsbell wrote: > nit: EnqueueResult Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:228: // The IDBRequest results whose events have not fired yet. On 2017/05/15 23:37:38, jsbell wrote: > nit: enqueued, not fired Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:38: // It follows that SSV will never produce byte sequences starting with 0xFF, On 2017/05/15 23:37:38, jsbell wrote: > Can you run this past jbroman@ as a sanity check? Will do. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:49: // 2) varint length-prefixed ASCII string - Blob UUID On 2017/05/15 23:37:39, jsbell wrote: > This is where I unhelpfully wonder if just storing the 16 byte UUID data is > better than storing 37 bytes (1 byte length = 0x24 + 36 ASCII char > serialization) FWIW, I think this (serialization format) is the most important area of the CL. After realizing that Blobs in IndexedDB don't come back with the same UUID, I dropped the UUID from the serialization format. Instead, I'm adding the index in the list of Blobs. This is redundant (as was the UUID), because the Blob that holds theSSV is always the last Blob in the list, and the current unwrapping code takes advantage of this observation. Justification for the pieces I have: 1) Blob size -- we may want to use different unwrapping methods / schedule things differently based on the Blob size 2) Blob index -- I imagine it might come in handy in the future. For example, we could tack on more Blobs (and more data to the wrapper Blob) without changing the kBlobWrappedValue tag. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:217: // TODO(pwnall): Blobs seem to get different UUIDs when they're resurrected? On 2017/05/15 23:37:38, jsbell wrote: > Did you verify this or ... ? Done. I just finished verifying this claim. Details in http://crbug.com/719007#c5 https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h:32: // "Normal" V8 values are stored on disk using the format implemented in On 2017/05/15 23:37:39, jsbell wrote: > nit: this first sentence is true whether we wrap or not. Rephrase? Done. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h:57: // value. On 2017/05/15 23:37:39, jsbell wrote: > Add "... to avoid side effects." > > (We didn't spec it for fun!) Done. Yikes, I didn't mean to criticize the spec, I only intended to justify myself (w/o knowing the spec, cloning functionality seems unnecessary in serialization logic). FWIW, I think that the structured clone solution is fairly elegant for spec-land, especially as (IIUC) there's room for an optimized implementation to skip creating the clone.
pwnall@chromium.org changed reviewers: + jbroman@chromium.org
jbroman: Can you please review the accuracy of the comment on lines 22-39 of third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp?
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
overall l*g*t*m https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:256: transit_blob_handles_.clear(); On 2017/05/19 18:27:33, pwnall wrote: > Would things be clearer if I moved the DCHECK to all HandleResponse instances > that don't call transit_blob_handles_.clear()? Yes, that would have answered my question before I asked it. :) https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:112: void HandleResponse(); On 2017/05/19 18:27:34, pwnall wrote: > Can I do that in a follow-up CL? sgtm https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.cpp:75: << "The reader returned more data than we were prepared for"; On 2017/05/19 18:27:34, pwnall wrote: > Is this what you were looking for, or should I log something else? That's fine. https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right): https://codereview.chromium.org/2822453003/diff/360001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:49: // 2) varint length-prefixed ASCII string - Blob UUID sgtm... On 2017/05/19 18:27:35, pwnall wrote: > the Blob that > holds theSSV is always the last Blob in the list, and the current unwrapping > code takes advantage of this observation. I didn't actually see this documented anywhere? https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:53: 'IndexedDB result should match put() argument'); In theory we should also verify that add() and update() work with large values. I don't think we need to duplicate every case, though. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:143: // Some types of requests, such as indexedDB.openDatabase(), cannot be issued How about: "IDBOpenDBRequests are not associated with transactions and never need Blob processing, so their results are handled..." https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:39: // IDBRequestQueueItem::MarkReady(). nit: MarkReady was renamed https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:47: // When the unwrapping completes, the loader will call MarkReady() on the nit: MarkReady was renamed https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:62: // If no more wrapped IDBValues are found, this calls the IDBRequest nit: only calls the IDBRequest callbacks indirectly. queue_item_'s MarkReady (but with new name) ? https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp:77: scope.GetExecutionContext()->NotifyContextDestroyed(); We should have a similar unit test where the context is stopped while there are queued IDBRequestQueueItems, e.g. one with an active IDBRequestLoader, one that's pending. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:231: // the results that arrive during the post-processing phase are queued up. nit: The first result requiring post-processing is also added to the queue, so "results that arrive during the post-processing phase..." is not quite correct, is it? https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:22: // V8 values are stored on disk by IndexedDB using the format implemented in Docs should probably mention that the blob is pushed into the value's blob_info vector, and thus if the unwrapped value has N blob references, the wrapped value has N+1 https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:44: // Identifies IndexedDB values that were wrapped in Blobs. The wrapper has the Can you explicitly write out the wrapped blob format header? i.e. 1) 0xFF - kVersionTag 2) 0x11 - kRequiresProcessingSSVPseudoVersion 3) 0x01 - kBlobWrappedValue Followed by... https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:152: bool IDBValueUnwrapper::IsWrapped(IDBValue* value) { Verify that blob_info size is >= 1 ?
Some arch suggestions around the queue item, and test suggestion. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:15: const wrapThreshold = 128 * 1024; Point to the code location where this is held https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:133: request_->transaction()->OnResultReady(); This would be calling a callback - so we don't depend on IDBTransaction.h https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:38: IDBRequestQueueItem(IDBRequest*, DOMException*); I'd love to remove IDBRequest from the class all together and instead use callbacks - I'm not sure if that's possible though. But I think it's doable. There would be two callbacks, one for the transactions OnResultsReady - which I would rename to SendAvailableResponses (something a little more descriptive of what it does). The second would be to enqueue the response. Couldn't we wrap the values we're giving in the constructor in a closure that calls EnqueueReponse on the request? Then we remove the dependencies on the request and the transaction (yes we remove the dcheck - I'm fine with that, or we can do checking some other way - like in one of the callbacks). https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp:77: scope.GetExecutionContext()->NotifyContextDestroyed(); On 2017/05/22 21:54:19, jsbell wrote: > We should have a similar unit test where the context is stopped while there are > queued IDBRequestQueueItems, e.g. one with an active IDBRequestLoader, one > that's pending. +1 https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp:94: Can we add testing for the case where we fail to create the blob when sending the 'put' request to the browser (for any reason - maybe easiest to create is where we don't have enough quota). One way to do this is with a browsertest where you manually limit the blob quota, and then craft a specific html page where the value is just above the limit. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:407: void IDBTransaction::OnResultReady() { Can you rename this to SendPendingReadyResponses()? Or something else that described this a little more directly? https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:413: result->EnqueueResponse(); If you change to callbacks.... you can make this just grab the callback and run it (so less logic methods on that item). Or, make it something like TakeEnqueueResponseCallback(), where the transaction owns it and that guarentees it can't be called twice (trying to replicate your constraints with the dchecks).
https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:39: // will have to be bumped. Yes, I believe this is true. It saddens me somewhat to have so many clever tricks used here, basically arising from the fact that we didn't originally have a magic number. It might be simpler to just grab and reserve a tag that was never used (0xFE or something), and just use that to mark the value. Anything current should begin with 0xFF, and it seems to me that this requires slightly less of a history lesson. Am I forgetting something? https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h:97: "application/x-blink-idb-value-wrapper"; IIUC, IANA no longer recommends deploying unregistered MIME types with the x- prefix. https://tools.ietf.org/html/rfc6838 Dunno if it's worth registering this, using x. instead of x-, or just deciding this is never going to be visible and thus we don't care. (Or we could use application/octet-stream, I suppose.)
Patchset #10 (id:500001) has been deleted
Patchset #10 (id:520001) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Patchset #11 (id:560001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:580001) has been deleted
Patchset #11 (id:600001) has been deleted
Patchset #10 (id:540001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Patchset #12 (id:660001) has been deleted
Patchset #10 (id:620001) has been deleted
pwnall@chromium.org changed reviewers: + foolip@chromium.org
foolip@chromium.org: Can you please review the changes in third_party/WebKit/public/platform/WebURLLoaderMockFactory.h? https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:15: const wrapThreshold = 128 * 1024; On 2017/05/23 18:16:39, dmurph wrote: > Point to the code location where this is held This is a WPT test, so it should in theory be browser-agnostic. It's not quite agnostic at this point, as the limit above is clearly based on the Chromium implementation but it can be easily tweaked once someone learns about other browsers' thresholds. I think that adding a link to Chrome's implementation would make it a bit less browser-agnostic. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/IndexedDB/large-nested-cloning.html:53: 'IndexedDB result should match put() argument'); On 2017/05/22 21:54:19, jsbell wrote: > In theory we should also verify that add() and update() work with large values. > I don't think we need to duplicate every case, though. Done (for now). I added coverage for IDBObjectStore.add() in two of the tests. I'd prefer to cover IDBCursor.continue() and the fancier cloning cases for IDBCursor.add() in a follow-up. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:143: // Some types of requests, such as indexedDB.openDatabase(), cannot be issued On 2017/05/22 21:54:19, jsbell wrote: > How about: "IDBOpenDBRequests are not associated with transactions and > never need Blob processing, so their results are handled..." I like your explanation, because it's simpler, but I don't think it works for the openDatabase response. AFAIK openDatabase is associated with a transaction, and we can get away without queueing because it's guaranteed to be the first request in the transaction. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:39: // IDBRequestQueueItem::MarkReady(). On 2017/05/22 21:54:19, jsbell wrote: > nit: MarkReady was renamed Done. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:47: // When the unwrapping completes, the loader will call MarkReady() on the On 2017/05/22 21:54:19, jsbell wrote: > nit: MarkReady was renamed Done. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestLoader.h:62: // If no more wrapped IDBValues are found, this calls the IDBRequest On 2017/05/22 21:54:19, jsbell wrote: > nit: only calls the IDBRequest callbacks indirectly. queue_item_'s MarkReady > (but with new name) ? Done. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:133: request_->transaction()->OnResultReady(); On 2017/05/23 18:16:39, dmurph wrote: > This would be calling a callback - so we don't depend on IDBTransaction.h I don't think there's general callback support in Blink -- the closest I could find is VoidCallback [1], which is garbage-collected, and seems overkill for this case. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/Void... https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:38: IDBRequestQueueItem(IDBRequest*, DOMException*); On 2017/05/23 18:16:39, dmurph wrote: > I'd love to remove IDBRequest from the class all together and instead use > callbacks - I'm not sure if that's possible though. But I think it's doable. > > There would be two callbacks, one for the transactions OnResultsReady - which I > would rename to SendAvailableResponses (something a little more descriptive of > what it does). > > The second would be to enqueue the response. Couldn't we wrap the values we're > giving in the constructor in a closure that calls EnqueueReponse on the request? > > Then we remove the dependencies on the request and the transaction (yes we > remove the dcheck - I'm fine with that, or we can do checking some other way - > like in one of the callbacks). This is not straightforward because some of the values in the callback are replaced by post-processing. Specifically, the IDBValues in PassRefPtr<IDBValue> and Vector<RefPtr<IDBValue>> can change if they're How about we refactor this after it lands successfully? It'll be easier to reason about the code impact of callbacks. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp:77: scope.GetExecutionContext()->NotifyContextDestroyed(); On 2017/05/22 21:54:19, jsbell wrote: > We should have a similar unit test where the context is stopped while there are > queued IDBRequestQueueItems, e.g. one with an active IDBRequestLoader, one > that's pending. Done. I wrote these tests for IDBRequest and IDBTransaction and fixed the bugs that they uncovered. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp:77: scope.GetExecutionContext()->NotifyContextDestroyed(); On 2017/05/23 18:16:39, dmurph wrote: > On 2017/05/22 21:54:19, jsbell wrote: > > We should have a similar unit test where the context is stopped while there > are > > queued IDBRequestQueueItems, e.g. one with an active IDBRequestLoader, one > > that's pending. > > +1 Acknowledged. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp:94: On 2017/05/23 18:16:39, dmurph wrote: > Can we add testing for the case where we fail to create the blob when sending > the 'put' request to the browser (for any reason - maybe easiest to create is > where we don't have enough quota). > > One way to do this is with a browsertest where you manually limit the blob > quota, and then craft a specific html page where the value is just above the > limit. Ack. Future work? According to [1-9], the transaction should not commit if it can't fetch and write blobs. So broken blobs should never be written to IndexedDB. [1] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... [2] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... [3] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... [4] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... [5] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... [6] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... [7] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... [8] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... [9] https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_tr... https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:407: void IDBTransaction::OnResultReady() { On 2017/05/23 18:16:39, dmurph wrote: > Can you rename this to SendPendingReadyResponses()? Or something else that > described this a little more directly? Let's figure out a good name in the next pass. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:413: result->EnqueueResponse(); On 2017/05/23 18:16:39, dmurph wrote: > If you change to callbacks.... you can make this just grab the callback and run > it (so less logic methods on that item). Or, make it something like > TakeEnqueueResponseCallback(), where the transaction owns it and that guarentees > it can't be called twice (trying to replicate your constraints with the > dchecks). Per in person discussion, I've introduced a WTF::Closure to decouple IDBRequestQueueItem from IDBTransaction. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBTransaction.h:231: // the results that arrive during the post-processing phase are queued up. On 2017/05/22 21:54:20, jsbell wrote: > nit: The first result requiring post-processing is also added to the queue, so > "results that arrive during the post-processing phase..." is not quite correct, > is it? Done. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:22: // V8 values are stored on disk by IndexedDB using the format implemented in On 2017/05/22 21:54:20, jsbell wrote: > Docs should probably mention that the blob is pushed into the value's blob_info > vector, and thus if the unwrapped value has N blob references, the wrapped value > has N+1 I tried to add the details to the documentation in IDBValueWrapper.h. Is this clearer? https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:39: // will have to be bumped. On 2017/05/23 20:13:11, jbroman wrote: > Yes, I believe this is true. It saddens me somewhat to have so many clever > tricks used here, basically arising from the fact that we didn't originally have > a magic number. > > It might be simpler to just grab and reserve a tag that was never used (0xFE or > something), and just use that to mark the value. Anything current should begin > with 0xFF, and it seems to me that this requires slightly less of a history > lesson. Am I forgetting something? I'm not sure I understand your proposal. Did you mean I could use a 0xFE prefix instead of the current 0xFF 0x11 [anything but 0xFF]? If I'd go that route, I'd need a list of forbidden follow-up bytes, similar to SSV's list of burned versions, in order to make sure I don't conflict with byte-swapped v0 SSVs. Using 0xFE later on (0xFF 0x11 0xFE or 0xFF 0x11 0xFF 0x0D 0xFE) seems to result in longer prefixes. I don't see any benefit in return for the longer prefix length. Specifically, SSV should fail on the prefix I'm proposing, and this would be used outside the SSV framework, so there's no benefit in getting a tag that would integrate well with SSV. Can you please explain in more detail? Thanks for putting up with me! https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:44: // Identifies IndexedDB values that were wrapped in Blobs. The wrapper has the On 2017/05/22 21:54:20, jsbell wrote: > Can you explicitly write out the wrapped blob format header? i.e. > > 1) 0xFF - kVersionTag > 2) 0x11 - kRequiresProcessingSSVPseudoVersion > 3) 0x01 - kBlobWrappedValue > > Followed by... Done. Ah, good call. That was really unclear. Thank you so much for catching the confusion! https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:152: bool IDBValueUnwrapper::IsWrapped(IDBValue* value) { On 2017/05/22 21:54:20, jsbell wrote: > Verify that blob_info size is >= 1 ? Done. I added a check IDBValueUnwrapper::Parse(), because it already does a size-related check. This is only relevant in case of disk corruption, so I hope not failing fast is acceptable. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.h:97: "application/x-blink-idb-value-wrapper"; On 2017/05/23 20:13:11, jbroman wrote: > IIUC, IANA no longer recommends deploying unregistered MIME types with the x- > prefix. > https://tools.ietf.org/html/rfc6838 > > Dunno if it's worth registering this, using x. instead of x-, or just deciding > this is never going to be visible and thus we don't care. (Or we could use > application/octet-stream, I suppose.) I submitted an application for "application/vnd.blink-idb-value-wrapper". Thank you very much for pointing this out! I had no idea that things have changed :) While I don't expect this to be user-visible, I see no harm in using a registered MIME type to tag our Blobs. This might help developers if the Blobs ever end up in a developer-visible UI, whether on purpose or by accident.
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pwnall@chromium.org changed reviewers: + dcheng@chromium.org - foolip@chromium.org
dcheng: Can you please review the changes in third_party/WebKit/public/platform/WebURLLoaderMockFactory.h? jsbell, dmurph: PTAL?
https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrapping.cpp:39: // will have to be bumped. On 2017/05/25 at 13:27:12, pwnall wrote: > On 2017/05/23 20:13:11, jbroman wrote: > > Yes, I believe this is true. It saddens me somewhat to have so many clever > > tricks used here, basically arising from the fact that we didn't originally have > > a magic number. > > > > It might be simpler to just grab and reserve a tag that was never used (0xFE or > > something), and just use that to mark the value. Anything current should begin > > with 0xFF, and it seems to me that this requires slightly less of a history > > lesson. Am I forgetting something? > > I'm not sure I understand your proposal. > > Did you mean I could use a 0xFE prefix instead of the current 0xFF 0x11 [anything but 0xFF]? If I'd go that route, I'd need a list of forbidden follow-up bytes, similar to SSV's list of burned versions, in order to make sure I don't conflict with byte-swapped v0 SSVs. > > Using 0xFE later on (0xFF 0x11 0xFE or 0xFF 0x11 0xFF 0x0D 0xFE) seems to result in longer prefixes. I don't see any benefit in return for the longer prefix length. Specifically, SSV should fail on the prefix I'm proposing, and this would be used outside the SSV framework, so there's no benefit in getting a tag that would integrate well with SSV. > > Can you please explain in more detail? Thanks for putting up with me! Yeah, okay. You're right that it's slightly more complicated than I'd thought (byte-swapping, yaaay). This sequence sgtm. (I just wish we didn't have to have to many of these tricks around, but it's hard to avoid.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nit. Context for other reviewers: We're trying really hard to get this in before m60 branch, as WASM really depends on this. https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.h:38: IDBRequestQueueItem(IDBRequest*, DOMException*); On 2017/05/25 13:27:11, pwnall wrote: > On 2017/05/23 18:16:39, dmurph wrote: > > I'd love to remove IDBRequest from the class all together and instead use > > callbacks - I'm not sure if that's possible though. But I think it's doable. > > > > There would be two callbacks, one for the transactions OnResultsReady - which > I > > would rename to SendAvailableResponses (something a little more descriptive of > > what it does). > > > > The second would be to enqueue the response. Couldn't we wrap the values we're > > giving in the constructor in a closure that calls EnqueueReponse on the > request? > > > > Then we remove the dependencies on the request and the transaction (yes we > > remove the dcheck - I'm fine with that, or we can do checking some other way - > > like in one of the callbacks). > > This is not straightforward because some of the values in the callback are > replaced by post-processing. Specifically, the IDBValues in PassRefPtr<IDBValue> > and Vector<RefPtr<IDBValue>> can change if they're > > How about we refactor this after it lands successfully? It'll be easier to > reason about the code impact of callbacks. sgtm https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp (right): https://codereview.chromium.org/2822453003/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestTest.cpp:94: On 2017/05/25 13:27:11, pwnall wrote: > On 2017/05/23 18:16:39, dmurph wrote: > > Can we add testing for the case where we fail to create the blob when sending > > the 'put' request to the browser (for any reason - maybe easiest to create is > > where we don't have enough quota). > > > > One way to do this is with a browsertest where you manually limit the blob > > quota, and then craft a specific html page where the value is just above the > > limit. > > Ack. > Future work? According to [1-9], the transaction should not commit if it can't > fetch and write blobs. So broken blobs should never be written to IndexedDB. > > [1] > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... > [2] > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... > [3] > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... > [4] > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... > [5] > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... > [6] > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... > [7] > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... > [8] > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_ba... > [9] > https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_tr... Acknowledged. https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:329: WrapPersistent(transaction_.Get())))); nit: I'm not super sure how oilpan works, I want to verify that this isn't creating a memory leak - transaction holds something that has a persistant pointer to itself. I'm guessing me make sure we clear this callback eagerly on all edge cases.
LGTM. I think the comments are fine to address in followups. https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:329: WrapPersistent(transaction_.Get())))); On 2017/05/25 17:28:06, dmurph wrote: > nit: I'm not super sure how oilpan works, I want to verify that this isn't > creating a memory leak - transaction holds something that has a persistant > pointer to itself. I'm guessing me make sure we clear this callback eagerly on > all edge cases. It might be good to document how we guarantee this not to be a cycle in a followup. https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp:92: std::unique_ptr<Vector<RefPtr<BlobDataHandle>>> blob_data, No need to fix in this CL, but: 1. Isn't Vector movable? It looks a bit odd to wrap it in a unique_ptr. 2. I think we're trying to get rid of PassRefPtr and just use RefPtr. https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h:50: const WebString& file_path = WebString()) override; I know we violate this today, but we actually prohibit default args on virtual methods since they don't work as expected: https://google.github.io/styleguide/cppguide.html#Default_Arguments
lgtm with a couple nits https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:1: Nit: remove blank line https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: 2017 https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp:65: EXPECT_FALSE(IDBValueUnwrapper::IsWrapped(mutant_value.Get())); All of the IsWrapped tests are either TRUE because it is wrapped, or false because the value is "invalid" (artificial and too short, or corrupted). It seems like we're missing the case where it's valid but not wrapped?
The CQ bit was checked by pwnall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org, jsbell@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2822453003/#ps740001 (title: "Addressed last round of feedback.")
dmurph, dcheng, jsbell: Thank you very much for the super-timely reviews! https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:329: WrapPersistent(transaction_.Get())))); On 2017/05/25 17:28:06, dmurph wrote: > nit: I'm not super sure how oilpan works, I want to verify that this isn't > creating a memory leak - transaction holds something that has a persistant > pointer to itself. I'm guessing me make sure we clear this callback eagerly on > all edge cases. dmurph: You seem to understand how OilPan works perfectly :) I added a comment in IDBRequestQueueItem that describes the cycle being created, and explains how we avoid turning that into a leak. We have tests for it now! https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:329: WrapPersistent(transaction_.Get())))); On 2017/05/25 18:34:04, dcheng wrote: > On 2017/05/25 17:28:06, dmurph wrote: > > nit: I'm not super sure how oilpan works, I want to verify that this isn't > > creating a memory leak - transaction holds something that has a persistant > > pointer to itself. I'm guessing me make sure we clear this callback eagerly on > > all edge cases. > > It might be good to document how we guarantee this not to be a cycle in a > followup. Done. I added a comment in this CL. https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBRequestQueueItem.cpp:1: On 2017/05/25 18:49:42, jsbell wrote: > Nit: remove blank line Done. Thank you for noticing! https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValue.cpp:92: std::unique_ptr<Vector<RefPtr<BlobDataHandle>>> blob_data, On 2017/05/25 18:34:04, dcheng wrote: > No need to fix in this CL, but: > 1. Isn't Vector movable? It looks a bit odd to wrap it in a unique_ptr. > 2. I think we're trying to get rid of PassRefPtr and just use RefPtr. Acknowledged. I will look to fix both of these in a follow-up. Regarding the 2nd issue... I tried to minimize introducing new PassRefPtrs, subject to the constraint that I didn't want to change IDBValue. I would like to do that in a follow-up, and remove the mental overhead of having to learn/remember how PassRefPtr works. https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/05/25 18:49:42, jsbell wrote: > nit: 2017 Done. Thank you for catching! https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/indexeddb/IDBValueWrappingTest.cpp:65: EXPECT_FALSE(IDBValueUnwrapper::IsWrapped(mutant_value.Get())); On 2017/05/25 18:49:42, jsbell wrote: > All of the IsWrapped tests are either TRUE because it is wrapped, or false > because the value is "invalid" (artificial and too short, or corrupted). It > seems like we're missing the case where it's valid but not wrapped? Yes, I need more tests. I prioritized these tests because I was most worried about accessing memory out of bounds, and the layout tests have decent coverage for valid unwrapped values. I plan to add more tests, and I'd much rather do that in follow-ups. This CL is already too big (to fail?) https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h (right): https://codereview.chromium.org/2822453003/diff/720001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h:50: const WebString& file_path = WebString()) override; On 2017/05/25 18:34:04, dcheng wrote: > I know we violate this today, but we actually prohibit default args on virtual > methods since they don't work as expected: > https://google.github.io/styleguide/cppguide.html#Default_Arguments Done. Thank you very much for pointing out! I did not know this! (how did you guys manage to build a browser with such a sketch language?!) I drew my inspiration from RegisterURL above. I fixed up the method I introduced, and I'll try to clean up RegisterURL in a follow-up.
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. Gist: IndexedDB values are currently stored in LevelDB according to the serialization logic in SerializedScriptValue (SSV). After this CL, an SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB stores a reference to the Blob. "small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB "large" v8::Value -> SSV -> IDBValue (SSV output) -> Blob w/ SSV output -> IDBValue (Blob reference) -> LevelDB On the write side, the IndexedDB (IDB) value serialization logic is extracted from IDBObjectStore::put() into a separate class, IDBValueWrapper. This is mostly to avoid further swelling IDBObjectStore::put(), which is already rather large. IDBValueWrapper knows how to turn an IDBValue into a SerializedScriptValue (SSV), and how to wrap the SSV into a Blob, if necessary. The read-side changes are more complex due to the following: 1) IDB request results are laziliy deserialized from IDBValue, because the SSV deserialization logic is expensive. The deserialization is done synchronously, but Blob data can only be fetched asynchronously. So, IDB values must be unwrapped from Blobs before the SSV logic is invoked. 2) The events for IDB request results must be dispatched in the order in which the requests were issued. If an IDB result's SSV requires Blob-unwrapping, all the following results must be queued until the Blob is unwrapped and the result's event can be delivered. The queueing is handled by (sigh) a layer of indirection. Previously, WebIDBCallbacksImpl called straight into IDBRequest::EnqueueResult(). This CL routes all callbacks that can be queued into IDBRequest::HandleResult() overloads that match the IDBRequest::EnqueueResult() signatures. When a result needs to be queued (either due to Blob-unwrapping, or because it arrives after another queued result), its data is saved in a IDBRequestQueueItem. These queued results are tracked by a per-transaction IDBRequestQueue. When a IDBValue contains a Blob reference, the Blob's content is asynchronously fetched by IDBRequestLoader. Afterwards, IDBValueUnwrapper turns the Blob data into an IDBValue that can be de-serialized by the SerializedScriptValue logic. BUG=713409 ========== to ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. Gist: IndexedDB values are currently stored in LevelDB according to the serialization logic in SerializedScriptValue (SSV). After this CL, an SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB stores a reference to the Blob. "small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB "large" v8::Value -> SSV -> IDBValue (SSV output) -> Blob w/ SSV output -> IDBValue (Blob reference) -> LevelDB On the write side, the IndexedDB (IDB) value serialization logic is extracted from IDBObjectStore::put() into a separate class, IDBValueWrapper. This is mostly to avoid further swelling IDBObjectStore::put(), which is already rather large. IDBValueWrapper knows how to turn an IDBValue into a SerializedScriptValue (SSV), and how to wrap the SSV into a Blob, if necessary. The read-side changes are more complex due to the following: 1) IDB request results are laziliy deserialized from IDBValue, because the SSV deserialization logic is expensive. The deserialization is done synchronously, but Blob data can only be fetched asynchronously. So, IDB values must be unwrapped from Blobs before the SSV logic is invoked. 2) The events for IDB request results must be dispatched in the order in which the requests were issued. If an IDB result's SSV requires Blob-unwrapping, all the following results must be queued until the Blob is unwrapped and the result's event can be delivered. The queueing is handled by (sigh) a layer of indirection. Previously, WebIDBCallbacksImpl called straight into IDBRequest::EnqueueResult(). This CL routes all callbacks that can be queued into IDBRequest::HandleResult() overloads that match the IDBRequest::EnqueueResult() signatures. When a result needs to be queued (either due to Blob-unwrapping, or because it arrives after another queued result), its data is saved in a IDBRequestQueueItem. These queued results are tracked by a per-transaction IDBRequestQueue. When a IDBValue contains a Blob reference, the Blob's content is asynchronously fetched by IDBRequestLoader. Afterwards, IDBValueUnwrapper turns the Blob data into an IDBValue that can be de-serialized by the SerializedScriptValue logic. BUG=713409 TBR=haraken ==========
pwnall@chromium.org changed reviewers: + haraken@chromium.org
TBR=haraken for the changes to the files below: * third_party/WebKit/Source/modules/BUILD.gn -- add IDBWrappingTest.cpp to list of tests * third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.cc -- implement changes in WebURLLoaderMockFactory * third_party/WebKit/Source/platform/testing/weburl_loader_mock_factory_impl.h Sorry for the TBR. The extension I'm using made me think that I had OWNERS coverage. Happy to address comments in follow-ups.
The CQ bit was checked by pwnall@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
rs LGTM
CQ is committing da patch. Bot data: {"patchset_id": 740001, "attempt_start_ts": 1495743645388890, "parent_rev": "09692cdc03a2723afadf7109b2b8404a9eb497ed", "commit_rev": "9f25693ac827e8ea87699023e640760fd09ef9bb"}
Message was sent while issue was closed.
Description was changed from ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. Gist: IndexedDB values are currently stored in LevelDB according to the serialization logic in SerializedScriptValue (SSV). After this CL, an SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB stores a reference to the Blob. "small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB "large" v8::Value -> SSV -> IDBValue (SSV output) -> Blob w/ SSV output -> IDBValue (Blob reference) -> LevelDB On the write side, the IndexedDB (IDB) value serialization logic is extracted from IDBObjectStore::put() into a separate class, IDBValueWrapper. This is mostly to avoid further swelling IDBObjectStore::put(), which is already rather large. IDBValueWrapper knows how to turn an IDBValue into a SerializedScriptValue (SSV), and how to wrap the SSV into a Blob, if necessary. The read-side changes are more complex due to the following: 1) IDB request results are laziliy deserialized from IDBValue, because the SSV deserialization logic is expensive. The deserialization is done synchronously, but Blob data can only be fetched asynchronously. So, IDB values must be unwrapped from Blobs before the SSV logic is invoked. 2) The events for IDB request results must be dispatched in the order in which the requests were issued. If an IDB result's SSV requires Blob-unwrapping, all the following results must be queued until the Blob is unwrapped and the result's event can be delivered. The queueing is handled by (sigh) a layer of indirection. Previously, WebIDBCallbacksImpl called straight into IDBRequest::EnqueueResult(). This CL routes all callbacks that can be queued into IDBRequest::HandleResult() overloads that match the IDBRequest::EnqueueResult() signatures. When a result needs to be queued (either due to Blob-unwrapping, or because it arrives after another queued result), its data is saved in a IDBRequestQueueItem. These queued results are tracked by a per-transaction IDBRequestQueue. When a IDBValue contains a Blob reference, the Blob's content is asynchronously fetched by IDBRequestLoader. Afterwards, IDBValueUnwrapper turns the Blob data into an IDBValue that can be de-serialized by the SerializedScriptValue logic. BUG=713409 TBR=haraken ========== to ========== Wrap large IndexedDB values into Blobs before writing to LevelDB. Gist: IndexedDB values are currently stored in LevelDB according to the serialization logic in SerializedScriptValue (SSV). After this CL, an SSV output that exceeds a threshold gets wrapped in a Blob, and LevelDB stores a reference to the Blob. "small" v8::Value -> SSV -> IDBValue (SSV output) -> LevelDB "large" v8::Value -> SSV -> IDBValue (SSV output) -> Blob w/ SSV output -> IDBValue (Blob reference) -> LevelDB On the write side, the IndexedDB (IDB) value serialization logic is extracted from IDBObjectStore::put() into a separate class, IDBValueWrapper. This is mostly to avoid further swelling IDBObjectStore::put(), which is already rather large. IDBValueWrapper knows how to turn an IDBValue into a SerializedScriptValue (SSV), and how to wrap the SSV into a Blob, if necessary. The read-side changes are more complex due to the following: 1) IDB request results are laziliy deserialized from IDBValue, because the SSV deserialization logic is expensive. The deserialization is done synchronously, but Blob data can only be fetched asynchronously. So, IDB values must be unwrapped from Blobs before the SSV logic is invoked. 2) The events for IDB request results must be dispatched in the order in which the requests were issued. If an IDB result's SSV requires Blob-unwrapping, all the following results must be queued until the Blob is unwrapped and the result's event can be delivered. The queueing is handled by (sigh) a layer of indirection. Previously, WebIDBCallbacksImpl called straight into IDBRequest::EnqueueResult(). This CL routes all callbacks that can be queued into IDBRequest::HandleResult() overloads that match the IDBRequest::EnqueueResult() signatures. When a result needs to be queued (either due to Blob-unwrapping, or because it arrives after another queued result), its data is saved in a IDBRequestQueueItem. These queued results are tracked by a per-transaction IDBRequestQueue. When a IDBValue contains a Blob reference, the Blob's content is asynchronously fetched by IDBRequestLoader. Afterwards, IDBValueUnwrapper turns the Blob data into an IDBValue that can be de-serialized by the SerializedScriptValue logic. BUG=713409 TBR=haraken Review-Url: https://codereview.chromium.org/2822453003 Cr-Commit-Position: refs/heads/master@{#474874} Committed: https://chromium.googlesource.com/chromium/src/+/9f25693ac827e8ea87699023e640... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:740001) as https://chromium.googlesource.com/chromium/src/+/9f25693ac827e8ea87699023e640...
Message was sent while issue was closed.
On 2017/05/25 23:23:56, haraken wrote: > rs LGTM Thank you very much for the quick review! |