Bug 60623 - IndexedDB: Fix integer comparison bug in LevelDB coding routines
Summary: IndexedDB: Fix integer comparison bug in LevelDB coding routines
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-11 06:34 PDT by Hans Wennborg
Modified: 2011-05-13 06:30 PDT (History)
2 users (show)

See Also:


Attachments
Patch (10.76 KB, patch)
2011-05-11 06:39 PDT, Hans Wennborg
tonyg: review+
tonyg: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-05-11 06:34:13 PDT
IndexedDB: Fix integer comparison bug in LevelDB coding routines
Comment 1 Hans Wennborg 2011-05-11 06:39:00 PDT
Created attachment 93114 [details]
Patch
Comment 2 David Grogan 2011-05-11 12:28:40 PDT
Comment on attachment 93114 [details]
Patch

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

> Source/WebCore/storage/IDBLevelDBCoding.cpp:-663
> -        return m_databaseId - other.m_databaseId;

What bad things were happening with these subtractions?
Comment 3 Hans Wennborg 2011-05-12 03:33:26 PDT
(In reply to comment #2)
> (From update of attachment 93114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93114&action=review
> 
> > Source/WebCore/storage/IDBLevelDBCoding.cpp:-663
> > -        return m_databaseId - other.m_databaseId;
> 
> What bad things were happening with these subtractions?

The subtractions work fine, the trouble is when the result is truncated from int64_t to int.

In the case that Greg found, one number was 1 and the other INT64_MAX. The difference is some large negative number, but after truncation it came out as "2", a.k.a. fail. This caused us to fail to read back object store meta-data from a database.

Not sure why I ever thought that code would work.
Comment 4 Tony Gentilcore 2011-05-13 05:47:00 PDT
Comment on attachment 93114 [details]
Patch

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

> Source/WebCore/storage/IDBLevelDBCoding.cpp:202
> +static int compareInts(int64_t a, int64_t b)

Does this end up getting inlined still? How perf-sensitive are the callers?

> Source/WebCore/storage/IDBLevelDBCoding.cpp:-1049
> -    ret.append(encodeByte(kSchemaVersionTypeByte));

This looks like a stray diff. If it is intentional, please explain in the ChangeLog.

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:38
> +// FIXME: We shouldn't need to rely on these macros.

This is also in IDBLevelDBCoding.cpp and IDBLevelDBBackingStore.cpp. Would be nice to consolidate at some point.
Comment 5 Hans Wennborg 2011-05-13 06:25:55 PDT
(In reply to comment #4)
> (From update of attachment 93114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93114&action=review
> 
> > Source/WebCore/storage/IDBLevelDBCoding.cpp:202
> > +static int compareInts(int64_t a, int64_t b)
> 
> Does this end up getting inlined still? How perf-sensitive are the callers?
Yes, it gets inlined and I think the asm looks decent.

> 
> > Source/WebCore/storage/IDBLevelDBCoding.cpp:-1049
> > -    ret.append(encodeByte(kSchemaVersionTypeByte));
> 
> This looks like a stray diff. If it is intentional, please explain in the ChangeLog.
Will do. The unit test uncovered this; that line shouldn't have been there.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:38
> > +// FIXME: We shouldn't need to rely on these macros.
> 
> This is also in IDBLevelDBCoding.cpp and IDBLevelDBBackingStore.cpp. Would be nice to consolidate at some point.
I know, it's definitely on my list.
Comment 6 Hans Wennborg 2011-05-13 06:30:22 PDT
Committed r86425: <http://trac.webkit.org/changeset/86425>