Bug 74213 - IndexedDB: Throw exception if IDBCursor.continue() called with lower key than current
Summary: IndexedDB: Throw exception if IDBCursor.continue() called with lower key than...
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: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-09 13:48 PST by Joshua Bell
Modified: 2012-01-09 16:13 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.37 KB, patch)
2011-12-09 13:49 PST, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (9.31 KB, patch)
2011-12-09 15:22 PST, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2011-12-09 13:48:46 PST
IndexedDB: Throw exception if IDBCursor.continue() called with lower key than current
Comment 1 Joshua Bell 2011-12-09 13:49:20 PST
Created attachment 118628 [details]
Patch
Comment 2 Joshua Bell 2011-12-09 13:50:50 PST
Seems easy -- too easy. I think I'm missing something.
Comment 3 Joshua Bell 2011-12-09 13:53:52 PST
Need to defend against the backing store's key being null.
Comment 4 David Grogan 2011-12-09 14:18:29 PST
Comment on attachment 118628 [details]
Patch

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

LGTM

> LayoutTests/storage/indexeddb/cursor-continue.html:-165
> -        evalAndLog("event.target.result.continue(1)");

What happens if this 1 stays?
Comment 5 Joshua Bell 2011-12-09 14:20:24 PST
Comment on attachment 118628 [details]
Patch

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

>> LayoutTests/storage/indexeddb/cursor-continue.html:-165
>> -        evalAndLog("event.target.result.continue(1)");
> 
> What happens if this 1 stays?

1 > 0 and this is descending, so it trips the newly added check and generates a DATA_ERR. To run off the end of the index like this test is expecting, either a lower key (e.g. -1) or no key should be specified.
Comment 6 Joshua Bell 2011-12-09 14:34:55 PST
Hrm, this fails the cursor prefetch browser test. Digging...
Comment 7 Joshua Bell 2011-12-09 15:22:04 PST
Created attachment 118656 [details]
Patch
Comment 8 Joshua Bell 2011-12-09 15:25:29 PST
Comment on attachment 118656 [details]
Patch

So this version passes tests. I believe with prefetching, m_cursor can end up becoming null even though we haven't reported back that the cursor has hit the end. Hans?
Comment 9 Hans Wennborg 2011-12-12 06:53:46 PST
(In reply to comment #8)
> (From update of attachment 118656 [details])
> So this version passes tests. I believe with prefetching, m_cursor can end up becoming null even though we haven't reported back that the cursor has hit the end. Hans?

Yes, prefetching can cause m_cursor to end up becoming null, even though the cursor on the front-end side hasn't reached the end yet.

However, as soon as any other operation is performed, such as calling continue() with a key, the prefetch cache is flushed and the cursor in the back-end is reset to match the front-end one.

I tried applying your first patch, but couldn't reproduce any failing browser test.

Anyway, the patch LGTM.
Comment 10 Joshua Bell 2012-01-09 15:33:28 PST
After studying the code further, I'm satisfied with the second patch. Prefetching implies that m_cursor may be null before the continue has run (what my first patch was erroneously erroring on). But when a key is passed in to continue() - the only case this patch is concerned with - the prefetching is reset. (The prefetch and reset logic lives in the chromium port.) Normally, that will reset m_cursor so we could write if(key) { ASSERT(m_cursor); ... }, However, m_cursor may be null for a cursor that has run to the end but is still being held on to by script, so we need the tests as present in the second patch.

tony@ - r? cq?
Comment 11 Joshua Bell 2012-01-09 15:44:30 PST
Comment on attachment 118656 [details]
Patch

Oops, forgot to cq? it. tony@ ?
Comment 12 Tony Chang 2012-01-09 15:48:35 PST
Comment on attachment 118656 [details]
Patch

Oh, I thought you had commit access?
Comment 13 WebKit Review Bot 2012-01-09 16:13:31 PST
Comment on attachment 118656 [details]
Patch

Clearing flags on attachment: 118656

Committed r104506: <http://trac.webkit.org/changeset/104506>
Comment 14 WebKit Review Bot 2012-01-09 16:13:36 PST
All reviewed patches have been landed.  Closing bug.