Bug 150939 - Modern IDB: Make indexes actually index
Summary: Modern IDB: Make indexes actually index
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on: 150910 151014
Blocks: 149117
  Show dependency treegraph
 
Reported: 2015-11-05 11:30 PST by Brady Eidson
Modified: 2015-11-19 13:59 PST (History)
5 users (show)

See Also:


Attachments
Patch v1 (91.95 KB, patch)
2015-11-10 16:01 PST, Brady Eidson
achristensen: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (942.05 KB, application/zip)
2015-11-10 16:50 PST, Build Bot
no flags Details
Patch for landing (87.71 KB, patch)
2015-11-10 20:35 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2015-11-05 11:30:04 PST
Modern IDB: Make indexes actually index
Comment 1 Brady Eidson 2015-11-10 16:01:31 PST
Created attachment 265241 [details]
Patch v1
Comment 2 Build Bot 2015-11-10 16:50:11 PST
Comment on attachment 265241 [details]
Patch v1

Attachment 265241 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/412198

New failing tests:
fast/hidpi/image-set-border-image-simple.html
Comment 3 Build Bot 2015-11-10 16:50:13 PST
Created attachment 265245 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 4 Alex Christensen 2015-11-10 17:58:09 PST
Comment on attachment 265241 [details]
Patch v1

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

r=me

> Source/WebCore/Modules/indexeddb/shared/IndexKey.cpp:45
> +    Vector<IDBKeyData> keys;
> +    keys.reserveCapacity(m_keys.size());

reserveInitialCapacity
Comment 5 Brady Eidson 2015-11-10 20:28:37 PST
(In reply to comment #4)
> Comment on attachment 265241 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265241&action=review
> 
> r=me
> 
> > Source/WebCore/Modules/indexeddb/shared/IndexKey.cpp:45
> > +    Vector<IDBKeyData> keys;
> > +    keys.reserveCapacity(m_keys.size());
> 
> reserveInitialCapacity

Good call. Thx
Comment 6 Brady Eidson 2015-11-10 20:35:05 PST
Created attachment 265263 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2015-11-10 22:07:04 PST
Comment on attachment 265263 [details]
Patch for landing

Clearing flags on attachment: 265263

Committed r192294: <http://trac.webkit.org/changeset/192294>
Comment 8 Darin Adler 2015-11-19 13:44:34 PST
Comment on attachment 265241 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp:64
> +    if (m_unique && m_key && *m_key == key) {

This is wrong when m_unique is true but m_key is null or not equal. It will fall into the code below and deterrence the pointer as the wrong type!
Comment 9 Darin Adler 2015-11-19 13:45:51 PST
Comment on attachment 265263 [details]
Patch for landing

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

> Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp:64
> +    if (m_unique && m_key && *m_key == key) {

Still wrong here. Need a test case for this and need to fix this by using a nested if rather than just a single one.
Comment 10 Darin Adler 2015-11-19 13:52:32 PST
deterrence -> dereference
Comment 11 Brady Eidson 2015-11-19 13:59:28 PST
(In reply to comment #9)
> Comment on attachment 265263 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265263&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/IndexValueEntry.cpp:64
> > +    if (m_unique && m_key && *m_key == key) {
> 
> Still wrong here. Need a test case for this and need to fix this by using a
> nested if rather than just a single one.

This patch landed with this bug as you pointed out.

*BUT* it was caught in a new test and fixed in https://trac.webkit.org/changeset/192610