Bug 74213 – IndexedDB: Throw exception if IDBCursor.continue() called with lower key than current

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Throw exception if IDBCursor.continue() called with lower key than current

[Joshua Bell]

Created attachment 118628 [details]
Patch

[Joshua Bell]

Seems easy -- too easy. I think I'm missing something.

[Joshua Bell]

Need to defend against the backing store's key being null.

[David Grogan]

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?

[Joshua Bell]

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.

[Joshua Bell]

Hrm, this fails the cursor prefetch browser test. Digging...

[Joshua Bell]

Created attachment 118656 [details]
Patch

[Joshua Bell]

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?

[Hans Wennborg]

(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.

[Joshua Bell]

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?

[Joshua Bell]

Comment on attachment 118656 [details]
Patch

Oops, forgot to cq? it. tony@ ?

[Tony Chang]

Comment on attachment 118656 [details]
Patch

Oh, I thought you had commit access?

[WebKit Review Bot]

Comment on attachment 118656 [details]
Patch

Clearing flags on attachment: 118656

Committed r104506: <http://trac.webkit.org/changeset/104506>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.