Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2786463002:
[IndexedDB] Fix cursor pooling IsValid(). (Closed)
Description[IndexedDB] Fix cursor pooling IsValid().
We accidentally didn't handle the case where the iterator was evicted
and invalid. This is now much more explicit and tested.
R=pwnall,cmumford
BUG=705837
Review-Url: https://codereview.chromium.org/2786463002
Cr-Commit-Position: refs/heads/master@{#460526}
Committed: https://chromium.googlesource.com/chromium/src/+/c785f2a0d65bbac2f5f896862e1ad21b38acb3ad
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments #
Messages
Total messages: 27 (15 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...
Hey Chris & Victor, I made a mistake in the last patch - this fixes IsValid calls. Daniel
On 2017/03/28 21:49:34, dmurph wrote: > Hey Chris & Victor, > > I made a mistake in the last patch - this fixes IsValid calls. > > Daniel LGTM. I should be able to write a WPT test that catches this, right?
On 2017/03/28 23:00:45, pwnall wrote: > On 2017/03/28 21:49:34, dmurph wrote: > > Hey Chris & Victor, > > > > I made a mistake in the last patch - this fixes IsValid calls. > > > > Daniel > > LGTM. > > I should be able to write a WPT test that catches this, right? Yes - see the bug for a repro.
The CQ bit was unchecked by dmurph@chromium.org
The CQ bit was checked by dmurph@chromium.org
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...)
dmurph@chromium.org changed reviewers: + jsbell@chromium.org
Ah, I need an owner. Chris or Josh, can you ptal?
lgtm % two nits. https://codereview.chromium.org/2786463002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2786463002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:47: DCHECK(iterator_); nit: Suggest removing the DCHECK because the program will crash on the next line, and the cause will be obvious. Don't think this adds anything of value. https://codereview.chromium.org/2786463002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:50: NOTREACHED(); Remove the default because the compile will fail if not all enumeration values are handled.
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...
https://codereview.chromium.org/2786463002/diff/1/content/browser/indexed_db/... File content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc (right): https://codereview.chromium.org/2786463002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:47: DCHECK(iterator_); On 2017/03/29 17:00:20, cmumford wrote: > nit: Suggest removing the DCHECK because the program will crash on the next > line, and the cause will be obvious. Don't think this adds anything of value. Done. https://codereview.chromium.org/2786463002/diff/1/content/browser/indexed_db/... content/browser/indexed_db/leveldb/leveldb_iterator_impl.cc:50: NOTREACHED(); On 2017/03/29 17:00:20, cmumford wrote: > Remove the default because the compile will fail if not all enumeration values > are handled. 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 cmumford@chromium.org, pwnall@chromium.org Link to the patchset: https://codereview.chromium.org/2786463002/#ps20001 (title: "comments")
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dmurph@chromium.org
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": 20001, "attempt_start_ts": 1490817476908330,
"parent_rev": "2d3c8ab9800e2eebc6a379a78f5afba38bd4f640", "commit_rev":
"c785f2a0d65bbac2f5f896862e1ad21b38acb3ad"}
Message was sent while issue was closed.
Description was changed from ========== [IndexedDB] Fix cursor pooling IsValid(). We accidentally didn't handle the case where the iterator was evicted and invalid. This is now much more explicit and tested. R=pwnall,cmumford BUG=705837 ========== to ========== [IndexedDB] Fix cursor pooling IsValid(). We accidentally didn't handle the case where the iterator was evicted and invalid. This is now much more explicit and tested. R=pwnall,cmumford BUG=705837 Review-Url: https://codereview.chromium.org/2786463002 Cr-Commit-Position: refs/heads/master@{#460526} Committed: https://chromium.googlesource.com/chromium/src/+/c785f2a0d65bbac2f5f896862e1a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c785f2a0d65bbac2f5f896862e1a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
