Bug 84467 - IndexedDB: cursor does not correctly iterate over keys added and removed during iteration
Summary: IndexedDB: cursor does not correctly iterate over keys added and removed duri...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-20 10:57 PDT by dstockwell
Modified: 2012-04-26 11:12 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.61 KB, patch)
2012-04-20 11:43 PDT, dstockwell
no flags Details | Formatted Diff | Diff
Patch (10.61 KB, patch)
2012-04-23 15:53 PDT, dstockwell
no flags Details | Formatted Diff | Diff
Patch (10.63 KB, patch)
2012-04-23 16:20 PDT, dstockwell
no flags Details | Formatted Diff | Diff
Patch (10.79 KB, patch)
2012-04-25 20:14 PDT, dstockwell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dstockwell 2012-04-20 10:57:52 PDT
Cursor iteration can fail to yield keys added in an order opposite to the cursor's iteration order. If the cursor iterating forwards at "3", and "6" then "5" are added, "5" will never be hit.

Would fail to suppress many deleted keys, but this is hidden by logic that issues a get() against the transaction on every continue() call.
Comment 1 dstockwell 2012-04-20 11:43:39 PDT
Created attachment 138131 [details]
Patch
Comment 2 Alec Flett 2012-04-23 15:00:01 PDT
Comment on attachment 138131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138131&action=review

> Source/WebCore/platform/leveldb/LevelDBTransaction.cpp:417
> +            && (!m_dbIterator->isValid()

I find the logic of this very difficult to read - can you break at least the direction checks into a separate helper function? these:
    || (m_direction == kForward && m_comparator->compare(m_treeIterator->key(), m_dbIterator->key()) < 0)
    || (m_direction == kReverse && m_comparator->compare(m_treeIterator->key(), m_dbIterator->key()) > 0))
Comment 3 dstockwell 2012-04-23 15:53:49 PDT
Created attachment 138445 [details]
Patch
Comment 4 Alec Flett 2012-04-23 16:08:57 PDT
Comment on attachment 138445 [details]
Patch

Thanks, this makes it much clearer...you can make the helpers const too. LGTM with that.
Comment 5 dstockwell 2012-04-23 16:20:18 PDT
Created attachment 138452 [details]
Patch
Comment 6 Joshua Bell 2012-04-25 14:30:02 PDT
LGTM. ojan@ - can you review?
Comment 7 WebKit Review Bot 2012-04-25 16:51:29 PDT
Comment on attachment 138452 [details]
Patch

Clearing flags on attachment: 138452

Committed r115260: <http://trac.webkit.org/changeset/115260>
Comment 8 WebKit Review Bot 2012-04-25 16:51:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Hin-Chung Lam 2012-04-25 18:17:57 PDT
Committed r115275: <http://trac.webkit.org/changeset/115275>
Comment 10 dstockwell 2012-04-25 18:21:34 PDT
Fix was reverted due to failures in debug build.
Comment 11 dstockwell 2012-04-25 20:14:04 PDT
Created attachment 138926 [details]
Patch
Comment 12 dstockwell 2012-04-25 20:15:44 PDT
Comment on attachment 138926 [details]
Patch

ojan@ - can you review again? I had to remove a stale assert.
Comment 13 WebKit Review Bot 2012-04-26 11:11:57 PDT
Comment on attachment 138926 [details]
Patch

Clearing flags on attachment: 138926

Committed r115333: <http://trac.webkit.org/changeset/115333>
Comment 14 WebKit Review Bot 2012-04-26 11:12:02 PDT
All reviewed patches have been landed.  Closing bug.