Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2828953002:
[IndexedDB] Hold referenced to IDB::put blobs in the IDBRequest (Closed)
|
Created:
3 years, 8 months ago by dmurph Modified:
3 years, 7 months ago CC:
chromium-reviews, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, blink-reviews, darin-cc_chromium.org, cmumford, haraken, jsbell+idb_chromium.org, michaeln Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[IndexedDB] Hold referenced to IDB::put blobs in the IDBRequest
Since blobs can die during transit (pending mojo change), we can prevent
this by holding onto the blob handles in the IDBRequest until we get the
response back from the browser.
R=pwnall,jsbell
BUG=351753, 710736, 713409
Review-Url: https://codereview.chromium.org/2828953002
Cr-Commit-Position: refs/heads/master@{#467836}
Committed: https://chromium.googlesource.com/chromium/src/+/ca74aee3773bb77867cf02d5f81725d10b33e4ab
Patch Set 1 #
Total comments: 3
Patch Set 2 : Implemented much better idea by Michael #Patch Set 3 : compile fix #
Total comments: 4
Patch Set 4 : Name change, and added dcheck #
Messages
Total messages: 29 (21 generated)
The CQ bit was checked by dmurph@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...
Victor & Josh, can you PTAL at how I do this? Is this worth the extra code / complexity (especially if I clean it up a bit)? Pros: * It makes auto-blobing easier (necessary to refcount the auto-blob blob so it survives transfer) * It makes all blobs sent to IDB survive transfer Cons: * We will remove this when we make blobs a mojo service I vote yes - although I can make it a little better w/ supporting ref'ing a list of uuids.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
LGTM. It seems to me that there isn't that much extra complexity (am I missing something?), and this CL is fixing a lifetime bug. https://codereview.chromium.org/2828953002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right): https://codereview.chromium.org/2828953002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_dispatcher_host.cc:116: blob_dispatcher_host_ = std::move(host); Would it make sense to DCHECK that the host_ is nullptr? My intuition is that this gets called exactly once for a host. https://codereview.chromium.org/2828953002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/indexed_db_dispatcher_host.h (right): https://codereview.chromium.org/2828953002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/indexed_db_dispatcher_host.h:118: // TODO(dmurph): Remove once blobs are mojo'd. Would it be worth pointing to this CL in the TODO? IIUC, all the plumbing here could go away if mojo can hang onto the Blobs.
some drive by comments... https://codereview.chromium.org/2828953002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp (right): https://codereview.chromium.org/2828953002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.cpp:517: registry->AddBlobDataRef(info.Uuid()); Manual refcounting always makes me a little uneasy (are their any cases where the balancing decref is not called and where is that balancing decref) I think the serialized_value created on line 386 contains a map of BlobDataHandles, so long as the handles in that map are retained the blobs will not be deleted. Does the 'request' or 'callbacks' object outlive put() completion? If so, maybe one of those could retain the references w/o manual refcounting. Also, is this change needed? At the time put() is called, there are valid refs on any blobs in the value being put. Is there any chance the release msg of those refs could beat the put msg sent on line 520? Maybe we can't depend on the ordering of put vs release messages.
Description was changed from ========== [BlobStorage & IndexedDB] Manually refcount in-transit blobs for IDB put messages This is a proof-of-concept, let me know if you like this direction. R=pwnall,jsbell BUG=351753,710736,713409 ========== to ========== [BlobStorage & IndexedDB] Manually refcount in-transit blobs for IDB put messages This is a proof-of-concept, let me know if you like this direction. R=pwnall,jsbell BUG=351753,710736,713409 ==========
The CQ bit was checked by dmurph@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...
Description was changed from ========== [BlobStorage & IndexedDB] Manually refcount in-transit blobs for IDB put messages This is a proof-of-concept, let me know if you like this direction. R=pwnall,jsbell BUG=351753,710736,713409 ========== to ========== [IndexedDB] Hold referenced to IDB::put blobs in the IDBRequest Since blobs can die during transit (pending mojo change), we can prevent this by holding onto the blob handles in the IDBRequest until we get the response back from the browser. R=pwnall,jsbell BUG=351753,710736,713409 ==========
Thanks for the idea Michael! That's way better. I need to add a test to the IDBRequest to verify that the handles are cleared when OnSuccess(key) or OnError() are called.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dmurph@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: This issue passed the CQ dry run.
michaeln@chromium.org changed reviewers: + michaeln@chromium.org
thnx! lgtm https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:293: ClearTransitBlobHandles(); There are many OnSuccess variants, no doubt this is the one that get's called for Put(), maybe to ensure and make clear this method isn't needed for the other variants, add a DCHECK(!transit_blobs) to OnSucessInternal. https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:146: void StorePutOperationBlobs( nit: consider naming the StoreXXX and ClearXXX methods with the same noun at the end
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (right): https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:293: ClearTransitBlobHandles(); On 2017/04/25 23:37:00, michaeln wrote: > There are many OnSuccess variants, no doubt this is the one that get's called > for Put(), maybe to ensure and make clear this method isn't needed for the other > variants, add a DCHECK(!transit_blobs) to OnSucessInternal. Done. https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/indexeddb/IDBRequest.h (right): https://codereview.chromium.org/2828953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/indexeddb/IDBRequest.h:146: void StorePutOperationBlobs( On 2017/04/25 23:37:00, michaeln wrote: > nit: consider naming the StoreXXX and ClearXXX methods with the same noun at the > end Done.
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pwnall@chromium.org, michaeln@chromium.org Link to the patchset: https://codereview.chromium.org/2828953002/#ps60001 (title: "Name change, and added dcheck")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493330663150790,
"parent_rev": "58bde56a7498151dccab27864c6beb20daf7ff45", "commit_rev":
"ca74aee3773bb77867cf02d5f81725d10b33e4ab"}
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Hold referenced to IDB::put blobs in the IDBRequest Since blobs can die during transit (pending mojo change), we can prevent this by holding onto the blob handles in the IDBRequest until we get the response back from the browser. R=pwnall,jsbell BUG=351753,710736,713409 ========== to ========== [IndexedDB] Hold referenced to IDB::put blobs in the IDBRequest Since blobs can die during transit (pending mojo change), we can prevent this by holding onto the blob handles in the IDBRequest until we get the response back from the browser. R=pwnall,jsbell BUG=351753,710736,713409 Review-Url: https://codereview.chromium.org/2828953002 Cr-Commit-Position: refs/heads/master@{#467836} Committed: https://chromium.googlesource.com/chromium/src/+/ca74aee3773bb77867cf02d5f817... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ca74aee3773bb77867cf02d5f817... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
