Bug 152201 – Modern IDB: storage/indexeddb/index-duplicate-keypaths.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Modern IDB: storage/indexeddb/index-duplicate-keypaths.html fails

[Brady Eidson]

Created attachment 267203 [details]
Patch v1

[Alex Christensen]

Comment on attachment 267203 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:366
> +    if (newKeyNumber < objectStore->currentKeyGeneratorValue())
> +        return { };
> +
> +    uint64_t newKeyInteger(newKeyNumber);
> +    if (newKeyInteger <= newKeyNumber)
> +        ++newKeyInteger;
> +
> +    ASSERT(newKeyInteger > newKeyNumber);

I think newKeyInteger will always be <= newKeyNumber if newKeyNumber is >= currentKeyGeneratorValue, which is a uint64_t,  which can never be negative.

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:370
> +    return IDBError();

I think if you use return { }; before you should use it here, too.  Also elsewhere in this file.

[Alex Christensen]

Comment on attachment 267203 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:349
> +IDBError MemoryIDBBackingStore::maybeUpdateKeyGeneratorNumber(const IDBResourceIdentifier& transactionIdentifier, uint64_t objectStoreIdentifier, double newKeyNumber)

This could also return void.  That would be different than all the other IDBBackingStore methods, but this can't generate an error.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 267203 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267203&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:366
> > +    if (newKeyNumber < objectStore->currentKeyGeneratorValue())
> > +        return { };
> > +
> > +    uint64_t newKeyInteger(newKeyNumber);
> > +    if (newKeyInteger <= newKeyNumber)
> > +        ++newKeyInteger;
> > +
> > +    ASSERT(newKeyInteger > newKeyNumber);
> 
> I think newKeyInteger will always be <= newKeyNumber if newKeyNumber is >=
> currentKeyGeneratorValue, which is a uint64_t,  which can never be negative.

I agree. So does the ASSERT!

> 
> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:370
> > +    return IDBError();
> 
> I think if you use return { }; before you should use it here, too.  Also
> elsewhere in this file.

Yup in this new code, you're right.

Not going to go change the rest of the file right now.

(In reply to comment #3)
> Comment on attachment 267203 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=267203&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:349
> > +IDBError MemoryIDBBackingStore::maybeUpdateKeyGeneratorNumber(const IDBResourceIdentifier& transactionIdentifier, uint64_t objectStoreIdentifier, double newKeyNumber)
> 
> This could also return void.  That would be different than all the other
> IDBBackingStore methods, but this can't generate an error.

This can generate an error in the upcoming SQLite backend, so no need to make it be void now just to change it again later.

[Brady Eidson]

https://trac.webkit.org/changeset/194010

[Brady Eidson]

(In reply to comment #4)
> (In reply to comment #2)
> > Comment on attachment 267203 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=267203&action=review
> > 
> > > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:366
> > > +    if (newKeyNumber < objectStore->currentKeyGeneratorValue())
> > > +        return { };
> > > +
> > > +    uint64_t newKeyInteger(newKeyNumber);
> > > +    if (newKeyInteger <= newKeyNumber)
> > > +        ++newKeyInteger;
> > > +
> > > +    ASSERT(newKeyInteger > newKeyNumber);
> > 
> > I think newKeyInteger will always be <= newKeyNumber if newKeyNumber is >=
> > currentKeyGeneratorValue, which is a uint64_t,  which can never be negative.
> 
> I agree. So does the ASSERT!
> 
Wait, I completely misread your comment. Your comment is the opposite of the ASSERT and opposite of the truth.]

newKeyInteger (a uint64) will always be greater-than-or-equal-to newKeyNumber (a double)

That's what the spec calls for and what the code here does.

The value of newKeyNumber vs currentKeyGeneratorValue is irrelevant on these lines, because the method already bails earlier on if the newKeyNumber is already less than currentKeyGeneratorValue