Bug 128316 - IDB: storage/indexeddb/create-index-with-integer-keys.html fails
Summary: IDB: storage/indexeddb/create-index-with-integer-keys.html fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-02-06 10:24 PST by Brady Eidson
Modified: 2014-02-07 10:22 PST (History)
5 users (show)

See Also:


Attachments
Patch v1 - Figure out the index keys to write in the DatabaseProcess as part of the createIndex operation itself. (46.17 KB, patch)
2014-02-06 23:25 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch v2 - Fix style (46.15 KB, patch)
2014-02-06 23:26 PST, Brady Eidson
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2014-02-06 10:24:08 PST
IDB: storage/indexeddb/create-index-with-integer-keys.html fails

The test creates an object store, puts a record in the object store, creates an index, then opens an index cursor.

The index cursor is supposed to represent that record in the object store, but comes up undefined.

We don't write index records for indexes that are created *after* a record is put in its referenced object store.  Doh!
Comment 1 Radar WebKit Bug Importer 2014-02-06 10:24:42 PST
<rdar://problem/16002857>
Comment 2 Brady Eidson 2014-02-06 12:28:09 PST
Populating an index if it is created after an object store already has records is what "IndexPopulator" is about.

That mechanism is still nuts, though.  We're going to do this way better.
Comment 3 Brady Eidson 2014-02-06 17:06:21 PST
Have this working in a sane manner with a little JSC in the DatabaseProcess.

Working out a kink in the patch (the test still has one wrong result) then I'll post for review.
Comment 4 Brady Eidson 2014-02-06 23:25:13 PST
Created attachment 223428 [details]
Patch v1 - Figure out the index keys to write in the DatabaseProcess as part of the createIndex operation itself.
Comment 5 Brady Eidson 2014-02-06 23:26:43 PST
Created attachment 223429 [details]
Patch v2 - Fix style
Comment 6 Geoffrey Garen 2014-02-07 10:14:32 PST
Comment on attachment 223429 [details]
Patch v2 - Fix style  

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

r=me

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:208
> +        for (auto i : keyDatas)

This should be "auto&" to avoid an apparent copy.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:209
> +            keys.append(i.maybeCreateIDBKey());

Let's null check before append to avoid a nullptr dereference later.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:329
> +                indexKeys.append(i.maybeCreateIDBKey());

Let's null check before append to avoid a nullptr dereference later.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:355
> +        if (!indexKey->isValid())
> +            return;

Maybe we should check isValid in both cases -- maybe up at the top, by the nullptr check? It seems that an array key can be invalid, so I guess we should check it.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:363
> +        for (auto i : indexKey->array())

auto&?

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:646
> +        for (auto indexKey : indexKeys) {

auto&?

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.h:117
> +    OwnPtr<JSC::JSGlobalObject> m_globalObject;

Please use JSC::Strong. It's like OwnPtr, but for GC instead of malloc. Otherwise, crash-o.
Comment 7 Brady Eidson 2014-02-07 10:22:36 PST
http://trac.webkit.org/changeset/163627