Bug 92562 – IndexedDB: IDBCursor.continue(key) does not throw for key "behind" cursor

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: IDBCursor.continue(key) does not throw for out of range key

[Joshua Bell]

Created attachment 155090 [details]
Patch

[Joshua Bell]

I found this while looking at our exception coverage for http://webkit.org/b/85513

dgrogan@, alecflett@ - please take a look.

[Joshua Bell]

Comment on attachment 155090 [details]
Patch

Whoops, forgot expectations.

[Joshua Bell]

Created attachment 155092 [details]
Patch

[Joshua Bell]

Updated patch has expectations and removes some LOG_ERROR lines I left in.

[Joshua Bell]

(In reply to comment #2)
> I found this while looking at our exception coverage for http://webkit.org/b/85513
> 
> dgrogan@, alecflett@ - please take a look.

Ping?

[David Grogan]

Comment on attachment 155092 [details]
Patch

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

LGTM

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:45
> +IDBCursorBackendImpl::IDBCursorBackendImpl(PassRefPtr<IDBBackingStore::Cursor> cursor, IDBCursor::Direction, CursorType cursorType, IDBTransactionBackendImpl* transaction, IDBObjectStoreBackendImpl* objectStore)

I'm guessing IDBCursor::Direction is still used by IDBCursorBackendProxy?

> LayoutTests/storage/indexeddb/resources/cursor-continue-dir.js:55
> +        request.onsuccess = null;

What's going on with the request.onsuccess = null lines?

[Joshua Bell]

Comment on attachment 155092 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:45
>> +IDBCursorBackendImpl::IDBCursorBackendImpl(PassRefPtr<IDBBackingStore::Cursor> cursor, IDBCursor::Direction, CursorType cursorType, IDBTransactionBackendImpl* transaction, IDBObjectStoreBackendImpl* objectStore)
> 
> I'm guessing IDBCursor::Direction is still used by IDBCursorBackendProxy?

No, it could be removed. (I rushed this patch a bit since it was a regression and I was headed out.) The direction is passed through to the IDBBackingStore::Cursor creation during the openCursor call, and the backend cursor is passed in here. Turns out the IDBCursorBackend* no longer needs to know about the direction at all. I can remove it. (I'll post an updated patch on Wednesday.)

>> LayoutTests/storage/indexeddb/resources/cursor-continue-dir.js:55
>> +        request.onsuccess = null;
> 
> What's going on with the request.onsuccess = null lines?

Debugging residue, I'll remove them. (Before fixing the bug the cursor.continue() call was not throwing, so the test would infinite-loop if the success handler was still around.)

[Alec Flett]

in another patch, I'm making progress towards replacing the 'unsigned short' directions with IDBCursor::Direction, so definitely don't remove it! My intention is that for direction, either you're using the string ("next") or the enum, (and the string quickly gets transformed into the enum) but not the unsigned short.

But this LGTM to me as well - makes me wonder what else we can rip out of the cursor backend, since it sounds like the backing store is doing most of the work.

[Joshua Bell]

(In reply to comment #9)
> in another patch, I'm making progress towards replacing the 'unsigned short' directions with IDBCursor::Direction, so definitely don't remove it! 

We're just talking about removing it from the IDBCursorBackendImpl constructor - that's not a problem, is it?

> But this LGTM to me as well - makes me wonder what else we can rip out of the cursor backend, since it sounds like the backing store is doing most of the work.

It's pretty slim at this point. Most of the code is boilerplate or prefetching.

I think some of the deleteFunction() checks can turn into ASSERTs but I'll leave that for later.

[Alec Flett]

(In reply to comment #10)
> (In reply to comment #9)
> > in another patch, I'm making progress towards replacing the 'unsigned short' directions with IDBCursor::Direction, so definitely don't remove it! 
> 
> We're just talking about removing it from the IDBCursorBackendImpl constructor - that's not a problem, is it?

Nope...I just mean don't remove the enum itself in the process of cleaning this up..

[Joshua Bell]

Created attachment 155832 [details]
Patch

[Joshua Bell]

Latest patch addresses dgrogan@'s comments - the unused direction argument to the IDBCursorBackendImpl is removed, and the unnecessary event handler clearing is removed.

tony@ (or others) - r? cq?

[Joshua Bell]

Oops, tony@ is unavailable.

ojan@, abarth@, eric@ - r? cq?

[Ojan Vafai]

Comment on attachment 155832 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 155832 [details]
Patch

Clearing flags on attachment: 155832

Committed r124361: <http://trac.webkit.org/changeset/124361>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.