Bug 152981 – Modern IDB: storage/indexeddb/key-generator.html fails

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

[Brady Eidson]

Modern IDB: storage/indexeddb/key-generator.html fails

It fails for two reasons:
1 - Once the key generator value is above 2^53 we don't handle errors attempting to generate a value.
2 - If a put/add operation fails after a key value is generated (e.g. because of Index constraints), the key generator value needs to be reset.

[Brady Eidson]

Created attachment 268701 [details]
Patch v1

[Brady Eidson]

Ugh. It doesn't apply because it relies on https://bugs.webkit.org/show_bug.cgi?id=152976

[Anders Carlsson]

Comment on attachment 268701 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:656
> +class ScopeExitFunction {

I think this should be called ScopeGuard.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:659
> +        : m_function(std::make_unique<std::function<void()>>(function))

This should use WTFMove.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:675
> +    std::unique_ptr<std::function<void()>> m_function;

This doesn't have to be an unique_ptr, it can just be an std::function. functions can be null.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:697
> +    std::unique_ptr<ScopeExitFunction> generatedKeyResetter;

If you add a default constructor to ScopeExitFunction, there's no need to make this an unique_ptr.

[Alex Christensen]

Comment on attachment 268701 [details]
Patch v1

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

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:41
> +static uint64_t maxGeneratedKeyValue = 9007199254740992;

As much as I hate to admit it, this is a number that I don't recognize just by looking at it.  Please write it in hex or 1 << 52.

[Brady Eidson]

(In reply to comment #4)
> Comment on attachment 268701 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268701&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:41
> > +static uint64_t maxGeneratedKeyValue = 9007199254740992;
> 
> As much as I hate to admit it, this is a number that I don't recognize just
> by looking at it.  Please write it in hex or 1 << 52.

I think a comment will be more important to understanding the value at a glance than either of those, but I'll do one of them with a comment.

[Brady Eidson]

Created attachment 268708 [details]
Patch v1

[Alex Christensen]

Comment on attachment 268708 [details]
Patch v1

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

r=me

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:350
> +        return { IDBDatabaseException::ConstraintError, "Cannot generate new key value over 2^53 for object store operation" };

Do we really need a message here that contains "2^53"? In C++ and JavaScript, that means 2 XOR 53, which evaluates to 55.  I guess this does appear in the spec...

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:656
> +class ScopeGuard {

This is pretty cool.  Don't we already have anything like this?

[Brady Eidson]

(In reply to comment #7)
> Comment on attachment 268708 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268708&action=review
> 
> r=me
> 
> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:350
> > +        return { IDBDatabaseException::ConstraintError, "Cannot generate new key value over 2^53 for object store operation" };
> 
> Do we really need a message here that contains "2^53"? In C++ and
> JavaScript, that means 2 XOR 53, which evaluates to 55.  I guess this does
> appear in the spec...

Yeah, since it's a string that can be searched for in the spec, I figure that's more useful for web developers who are actually looking at these error messages and diagnosing them. In context it's unlikely to be confused for C++/JS code.

> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:656
> > +class ScopeGuard {
> 
> This is pretty cool.  Don't we already have anything like this?

As I confirmed with Anders, no. :(

[Brady Eidson]

http://trac.webkit.org/changeset/194864