Bug 185869 – IndexedDB: breaks if binary data (Uint8Array) and autoIncrement key in store

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Maxime Réty]

Created attachment 340983 [details]
Test case

There's a bug in Safari 11.1 when you try to add a record with binary data (Uint8Array) into an indexedDB store having an autoIncrement key.

The triggered error is "UnknownError: An internal error was encountered in the Indexed Database server", and then the whole database is broken and can't be reopened/reused.

This issue neither happens if you have binary data and a store without autoIncrement, nor non-binary data and a store with autoIncrement. Only having both is an issue.

Breaks (at least) on Safari Version 11.1 (13605.1.33.1.4) on macOS High Sierra 10.13.4.

Full test case: https://gist.github.com/maximerety/2332ed4660652f57ef6e12e3194e28e4

Related issue: https://github.com/dfahlander/Dexie.js/issues/656

[Radar WebKit Bug Importer]

<rdar://problem/40453623>

[Sihui Liu]

Created attachment 355704 [details]
Patch

[Geoffrey Garen]

Comment on attachment 355704 [details]
Patch

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

r=me

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:955
> +    static NeverDestroyed<Strong<JSDOMGlobalObject>> domGlobalObject(databaseThreadVM(), JSDOMGlobalObject::create(databaseThreadVM(), JSDOMGlobalObject::createStructure(databaseThreadVM(), jsNull()), normalWorld(databaseThreadVM())));

It's not necessary in this patch, but I think it would be nice to write a follow-up patch that eventually deletes this global object and its VM. We use this global object as a temporary to enable serializing and deserializing JavaScript values. It's wasteful to keep it and its VM allocated forever.

One strategy is to create an object that owns the VM and global object, and make that object reference counted and owned by each UniqueIDBDatabase. That way, when the last UniqueIDBDatabase is closed, we can free the VM.

So you would have something like:

class IDBSerializationContext : public WeakPtrFactory<IDBSerializationContext>, public RefCounted<IDBSerializationContext> {
    ...
    private:
        std::unique_ptr<VM> m_vm;
        Strong<JSDOMGlobalObject> m_globalObject;
};

IDBSerializationContext& sharedIDBSerializationContext()
{
    static WeakPtr<IDBSerializationContext> s_context;
    if (!s_context) {
        ...
    }
    return s_context;
}

> Source/WebCore/bindings/js/JSDOMWrapper.cpp:44
> +    ASSERT(globalObject.classInfo() == JSDOMGlobalObject::info() || scriptExecutionContext() || globalObject.classInfo() == JSRemoteDOMWindow::info());

Probably best to write this as:

    ASSERT(scriptExecutionContext() || globalObject.classInfo() == JSRemoteDOMWindow::info() || globalObject.classInfo() == JSDOMGlobalObject::info());

The primary thing we're trying to assert is that we have a script execution context. After that come two exceptions to the general rule. Best to put the general rule first.

[Sihui Liu]

Comment on attachment 355704 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:955
>> +    static NeverDestroyed<Strong<JSDOMGlobalObject>> domGlobalObject(databaseThreadVM(), JSDOMGlobalObject::create(databaseThreadVM(), JSDOMGlobalObject::createStructure(databaseThreadVM(), jsNull()), normalWorld(databaseThreadVM())));
> 
> It's not necessary in this patch, but I think it would be nice to write a follow-up patch that eventually deletes this global object and its VM. We use this global object as a temporary to enable serializing and deserializing JavaScript values. It's wasteful to keep it and its VM allocated forever.
> 
> One strategy is to create an object that owns the VM and global object, and make that object reference counted and owned by each UniqueIDBDatabase. That way, when the last UniqueIDBDatabase is closed, we can free the VM.
> 
> So you would have something like:
> 
> class IDBSerializationContext : public WeakPtrFactory<IDBSerializationContext>, public RefCounted<IDBSerializationContext> {
>     ...
>     private:
>         std::unique_ptr<VM> m_vm;
>         Strong<JSDOMGlobalObject> m_globalObject;
> };
> 
> IDBSerializationContext& sharedIDBSerializationContext()
> {
>     static WeakPtr<IDBSerializationContext> s_context;
>     if (!s_context) {
>         ...
>     }
>     return s_context;
> }

Sure, will do.

>> Source/WebCore/bindings/js/JSDOMWrapper.cpp:44
>> +    ASSERT(globalObject.classInfo() == JSDOMGlobalObject::info() || scriptExecutionContext() || globalObject.classInfo() == JSRemoteDOMWindow::info());
> 
> Probably best to write this as:
> 
>     ASSERT(scriptExecutionContext() || globalObject.classInfo() == JSRemoteDOMWindow::info() || globalObject.classInfo() == JSDOMGlobalObject::info());
> 
> The primary thing we're trying to assert is that we have a script execution context. After that come two exceptions to the general rule. Best to put the general rule first.

JSDOMObject invokes JSDOMGlobalObject::scriptExecutionContext(), which checks if JSDOMGlobalObject inherits from JS*Window or JSWorker* or JSWorker* or JSWorklet* with release assert.  I put globalObject.classInfo() == JSDOMGlobalObject::info() before scriptExecutionContext() to avoid removing the release assertion, as creating a JSDOMGlobalObject without window or worker is a special case that should not happen elsewhere. I can change the order here and remove the assertion in scriptExecutionContext().

[Geoffrey Garen]

> JSDOMObject invokes JSDOMGlobalObject::scriptExecutionContext(), which
> checks if JSDOMGlobalObject inherits from JS*Window or JSWorker* or
> JSWorker* or JSWorklet* with release assert.  I put globalObject.classInfo()
> == JSDOMGlobalObject::info() before scriptExecutionContext() to avoid
> removing the release assertion, as creating a JSDOMGlobalObject without
> window or worker is a special case that should not happen elsewhere. I can
> change the order here and remove the assertion in scriptExecutionContext().

OK, let's leave this as is for now.

Perhaps later we'll decide that IDB should really use JSWorkerGlobalScope instead of JSDOMGlobalObject.

[WebKit Commit Bot]

Comment on attachment 355704 [details]
Patch

Clearing flags on attachment: 355704

Committed r238677: <https://trac.webkit.org/changeset/238677>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.