Bug 60623 – IndexedDB: Fix integer comparison bug in LevelDB coding routines

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

IndexedDB: Fix integer comparison bug in LevelDB coding routines

[Hans Wennborg]

Created attachment 93114 [details]
Patch

[David Grogan]

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?

[Hans Wennborg]

(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.

[Tony Gentilcore]

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.

[Hans Wennborg]

(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.

[Hans Wennborg]

Committed r86425: <http://trac.webkit.org/changeset/86425>