Bug 152201

Created attachment 267203 [details] Patch v1

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.

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.

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

(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