IndexedDB: Throw exception if IDBCursor.continue() called with lower key than current
Created attachment 118628 [details] Patch
Seems easy -- too easy. I think I'm missing something.
Need to defend against the backing store's key being null.
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 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.
Hrm, this fails the cursor prefetch browser test. Digging...
Created attachment 118656 [details] Patch
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?
(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.
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 on attachment 118656 [details] Patch Oops, forgot to cq? it. tony@ ?
Comment on attachment 118656 [details] Patch Oh, I thought you had commit access?
Comment on attachment 118656 [details] Patch Clearing flags on attachment: 118656 Committed r104506: <http://trac.webkit.org/changeset/104506>
All reviewed patches have been landed. Closing bug.