Bug 127866 – IDB: ObjectStore cursor advance() support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jon Lee]

IDB: ObjectStore cursor advance() support

<rdar://problem/15779645>

[Brady Eidson]

Created attachment 222714 [details]
Patch v1

[Brady Eidson]

Created attachment 222725 [details]
Patch v2 - build fix

[Martin Hock]

Comment on attachment 222725 [details]
Patch v2 - build fix

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

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:887
> +        // There's no way to indicate an error to SQLite - we have to return a sorting decision.
> +        // We arbitrarily choose "A > B"
> +        return 1;

I would suggest that even if the data is not deserializable, you still write a collation function that obeys the properties here: http://www.sqlite.org/c3ref/create_collation.html
Otherwise, "the behavior of SQLite is undefined".
I think it would be best if you put all un-deserializable things together, maybe before everything else.  So you would always try to deserialize both things, even if the first one can't be deserialized.  When comparing two un-deserializable things, just call memcmp.

You may also want to consider performing sqlite3_interrupte():
http://sqlite.1065341.n5.nabble.com/Signaling-errors-from-collation-functions-td43419.html

[Sam Weinig]

Comment on attachment 222725 [details]
Patch v2 - build fix

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

> Source/WebKit2/DatabaseProcess/IndexedDB/IDBIdentifier.h:84
> +    DatabaseProcessIDBConnection& connection() const { return *m_connection; }

You should ASSERT(m_connection) here.

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBCursor.cpp:43
> +    std::unique_ptr<SQLiteIDBCursor> cursor = std::unique_ptr<SQLiteIDBCursor>(new SQLiteIDBCursor(transaction, cursorIdentifier, objectStoreID, indexID, cursorDirection, cursorType, taskType, keyRange));

This should use auto and make_unique<>

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBTransaction.cpp:41
> +SQLiteIDBTransaction::SQLiteIDBTransaction(UniqueIDBDatabaseBackingStoreSQLite* backingStore, const IDBIdentifier& transactionIdentifier, IndexedDB::TransactionMode mode)

Could backingStore be a reference?

[Brady Eidson]

(In reply to comment #4)
> (From update of attachment 222725 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222725&action=review
> 
> > Source/WebKit2/DatabaseProcess/IndexedDB/IDBIdentifier.h:84
> > +    DatabaseProcessIDBConnection& connection() const { return *m_connection; }
> 
> You should ASSERT(m_connection) here.

Fixed.

> > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBCursor.cpp:43
> > +    std::unique_ptr<SQLiteIDBCursor> cursor = std::unique_ptr<SQLiteIDBCursor>(new SQLiteIDBCursor(transaction, cursorIdentifier, objectStoreID, indexID, cursorDirection, cursorType, taskType, keyRange));
> 
> This should use auto and make_unique<>

Using auto.

To use make_unique<> requires a public constructor or finding the right syntax for friending std::make_unique.

I can't figure out that syntax...  Will skip this for now.

> 
> > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBTransaction.cpp:41
> > +SQLiteIDBTransaction::SQLiteIDBTransaction(UniqueIDBDatabaseBackingStoreSQLite* backingStore, const IDBIdentifier& transactionIdentifier, IndexedDB::TransactionMode mode)
> 
> Could backingStore be a reference?

Yup.

[Brady Eidson]

http://trac.webkit.org/changeset/163113

[Brady Eidson]

Sorry I'd missed this comment before landing.

(In reply to comment #3)
> (From update of attachment 222725 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222725&action=review
> 
> > Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:887
> > +        // There's no way to indicate an error to SQLite - we have to return a sorting decision.
> > +        // We arbitrarily choose "A > B"
> > +        return 1;
> 
> I would suggest that even if the data is not deserializable, you still write a collation function that obeys the properties here: http://www.sqlite.org/c3ref/create_collation.html
> Otherwise, "the behavior of SQLite is undefined".

If the data coming in is bogus, then the behavior of the Indexed Database API is "undefined", so we're not losing anything by being undefined in SQLite terms as well.

> I think it would be best if you put all un-deserializable things together, maybe before everything else.  So you would always try to deserialize both things, even if the first one can't be deserialized.  When comparing two un-deserializable things, just call memcmp.

I think memcmp on blobs that are supposed to represent a complex object would be just as much of an undefined "lie" as this arbitrary decision.

> You may also want to consider performing sqlite3_interrupte():
> http://sqlite.1065341.n5.nabble.com/Signaling-errors-from-collation-functions-td43419.html

This is an interesting option that I wasn't aware of!  Definitely outside the scope of this patch (as bogus data already means IDB is acting in an undefined manner) but an interesting improvement in the future to give better error reporting.

Thanks for suggesting it!