Bug 127866

Summary: IDB: ObjectStore cursor advance() support
Product: WebKit Reporter: Jon Lee <jonlee>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, commit-queue, jsbell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 124521    
Attachments:
Description Flags
Patch v1
none
Patch v2 - build fix sam: review+

Description Jon Lee 2014-01-29 16:27:12 PST
IDB: ObjectStore cursor advance() support

<rdar://problem/15779645>
Comment 1 Brady Eidson 2014-01-30 12:54:57 PST
Created attachment 222714 [details]
Patch v1
Comment 2 Brady Eidson 2014-01-30 13:31:18 PST
Created attachment 222725 [details]
Patch v2 - build fix
Comment 3 Martin Hock 2014-01-30 13:59:51 PST
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
Comment 4 Sam Weinig 2014-01-30 14:04:07 PST
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?
Comment 5 Brady Eidson 2014-01-30 14:19:06 PST
(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.
Comment 6 Brady Eidson 2014-01-30 14:19:13 PST
http://trac.webkit.org/changeset/163113
Comment 7 Brady Eidson 2014-01-30 14:22:54 PST
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!