[webkit-reviews] review granted: [Bug 185869] IndexedDB: breaks if binary data (Uint8Array) and autoIncrement key in store : [Attachment 355704] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 185869: IndexedDB: breaks if binary data (Uint8Array) and autoIncrement key
in store
https://bugs.webkit.org/show_bug.cgi?id=185869

Attachment 355704: Patch

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




--- Comment #3 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 355704
  --> https://bugs.webkit.org/attachment.cgi?id=355704
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.