IndexedDB: ObjectStoreMetaDataKey::m_metaDataType should use byte type

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Xingnan Wang]

Use unsigned char instead of integer.

[Xingnan Wang]

Created attachment 155461 [details]
Patch

[Kentaro Hara]

Comment on attachment 155461 [details]
Patch

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

Looks good.

> Source/WebCore/ChangeLog:8
> +        No new tests - Covered by existing tests.

Would you list up a couple of existing tests here?

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:1089
> +    result->m_metaDataType = *p++;

Shall we implement decodeByte()? (For consistency with existing encodeByte().)

[Xingnan Wang]

Comment on attachment 155461 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests - Covered by existing tests.
> 
> Would you list up a couple of existing tests here?

All right. It`s in the encoding/decoding level of object store,I think most of the test cases could cover this.

>> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:1089
>> +    result->m_metaDataType = *p++;
> 
> Shall we implement decodeByte()? (For consistency with existing encodeByte().)

Good suggestion.

[Joshua Bell]

Comment on attachment 155461 [details]
Patch

This looks like it will fail for existing stored data on disk.  Some back compat mechanism is necessary. 

I'm on vacation at the moment. Please CC dgrogan and alecflett for feedback.

[Joshua Bell]

(Ah, they already are CC'd. I'm reading this on my phone, sorry.)

[David Grogan]

Comment on attachment 155461 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:1089
>>> +    result->m_metaDataType = *p++;
>> 
>> Shall we implement decodeByte()? (For consistency with existing encodeByte().)
> 
> Good suggestion.

For compatibility with existing databases, you can maybe detect here whether it's a byte or var int.  I don't know if that's bulletproof though.  Or you can look into doing a schema upgrade in IDBLevelDBBackingStore::setUpMetadata.  That might be slow.  Some experimentation might be needed.

[Xingnan Wang]

(In reply to comment #6)
> (From update of attachment 155461 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155461&action=review
> 
> >>> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:1089
> >>> +    result->m_metaDataType = *p++;
> >> 
> >> Shall we implement decodeByte()? (For consistency with existing encodeByte().)
> > 
> > Good suggestion.
> 
> For compatibility with existing databases, you can maybe detect here whether it's a byte or var int.  I don't know if that's bulletproof though.  Or you can look into doing a schema upgrade in IDBLevelDBBackingStore::setUpMetadata.  That might be slow.  Some experimentation might be needed.

m_metaDataType is always less than 8, which is encoded by encodeVarInt() with only one byte. Also encodeByte() uses one byte to store the data with the same way. So could we think it would not fail for the existing data in disk?

[Joshua Bell]

Comment on attachment 155461 [details]
Patch

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

You're right - the range of existing values that are used for the ObjectMetaDataType should encode identically for VarInt and Byte since they are <= 127. So this should be safe. (A webkit_unit_test case could be added for that, validating that the encodings are the same for the current roster of values.) Removing my paranoid objection.

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:67
> +//     The prefix is followed by a type byte, then a variable-length integer, and then another variable-length integer.

That should end with "and then another type byte."

[Xingnan Wang]

Created attachment 155729 [details]
Patch

[Xingnan Wang]

(In reply to comment #9)
> Created an attachment (id=155729) [details]
> Patch

Updated the patch as your comments, thanks.

[Kentaro Hara]

Comment on attachment 155729 [details]
Patch

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

LGTM. If jsbell@ says OK, I'm happy to r+ it.

> Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:180
> +    ASSERT(p < limit);
> +    if (p >= limit)

This looks strange. We don't need both. (Although some code in IDBLevelDBCoding.cpp already use both.)

If 'p >= limit' must not happen, please remove 'if (p >= limit)'. If it can happen, please remove ASSERT(p < limit). (I think that removing ASSERT() looks reasonable in this case.)

[Xingnan Wang]

Created attachment 155750 [details]
Patch

[Joshua Bell]

(In reply to comment #11)
> (From update of attachment 155729 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155729&action=review
> 
> LGTM. If jsbell@ says OK, I'm happy to r+ it.

I'm going to build the Chromium tests locally with this patch first. Working on that now.
 
> > Source/WebCore/Modules/indexeddb/IDBLevelDBCoding.cpp:180
> > +    ASSERT(p < limit);
> > +    if (p >= limit)
> 
> This looks strange. We don't need both. (Although some code in IDBLevelDBCoding.cpp already use both.)
> 
> If 'p >= limit' must not happen, please remove 'if (p >= limit)'. If it can happen, please remove ASSERT(p < limit). (I think that removing ASSERT() looks reasonable in this case.)

As an FYI:

Although much of this code predates me, I believe the intent of these redundant tests is to serve two different purposes. The ASSERTs are present to catch errors during development, e.g. when we're refactoring code, modifying constants, etc. The non-debug tests are in there to catch runtime errors due to corrupt data (e.g. actual on-disk corruption, uncaught bugs that write bad data, etc). 

I agree that's not a very good style, and I'm fine with leaving out the ASSERT here, as the error handling in this file needs a cleanup pass. We should probably switch the ASSERTs over to something like a custom macro that does ASSERT in Debug but also does a LOG_ERROR and stats reporting in Release builds, in addition to returning the error code.

[Joshua Bell]

Comment on attachment 155750 [details]
Patch

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

LGTM with one change suggestion.

Also, I think there should be a Source/WebKit/chromium/ChangeLog entry. Either just add it manually, or remove the Source/WebCore/ChangeLog entry, run webkit-patch prepare 92725, then re-apply the Source/WebCore/ChangeLog entry (I don't know if there's a faster way to prepare just the missing ChangeLogs)

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:93
> +#ifdef NDEBUG

These NDEBUG guards should not be necessary here. 

We use them in the "Int" tests due to ASSERTs that guard against using data with the high bit set (since the methods are not correctly using signed/unsigned types), but wanted to ensure the correct behavior in the test. (I can dig up the bug number if desired, there was a long discussion.)

The new decodeByte function correctly takes an unsigned char, though, so it doesn't matter here.

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:708
> +TEST(IDBLevelDBCodingTest, EncodeVarIntVSEncodeByteTest)

Thanks a bunch for adding this.

[Xingnan Wang]

Created attachment 155949 [details]
Patch

[Xingnan Wang]

Comment on attachment 155750 [details]
Patch

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

>> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:93
>> +#ifdef NDEBUG
> 
> These NDEBUG guards should not be necessary here. 
> 
> We use them in the "Int" tests due to ASSERTs that guard against using data with the high bit set (since the methods are not correctly using signed/unsigned types), but wanted to ensure the correct behavior in the test. (I can dig up the bug number if desired, there was a long discussion.)
> 
> The new decodeByte function correctly takes an unsigned char, though, so it doesn't matter here.

Removed this one.

>> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:708
>> +TEST(IDBLevelDBCodingTest, EncodeVarIntVSEncodeByteTest)
> 
> Thanks a bunch for adding this.

Thanks for your review, updated the patch.

[Kentaro Hara]

Comment on attachment 155949 [details]
Patch

Looks OK

[WebKit Review Bot]

Comment on attachment 155949 [details]
Patch

Clearing flags on attachment: 155949

Committed r124402: <http://trac.webkit.org/changeset/124402>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Chromium Android build fix landed in http://trac.webkit.org/changeset/124411.