Bug 60623

Summary: IndexedDB: Fix integer comparison bug in LevelDB coding routines
Product: WebKit Reporter: Hans Wennborg <hans>
Component: New BugsAssignee: Hans Wennborg <hans>
Status: RESOLVED FIXED    
Severity: Normal CC: andreip, dgrogan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch tonyg: review+, tonyg: commit-queue-

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>