[webkit-reviews] review granted: [Bug 189435] IndexedDB tests leak documents : [Attachment 361403] 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 189435: IndexedDB tests leak documents
https://bugs.webkit.org/show_bug.cgi?id=189435

Attachment 361403: Patch

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




--- Comment #9 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 361403
  --> https://bugs.webkit.org/attachment.cgi?id=361403
Patch

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

r=me

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:131
> +    JSValue key = m_cachedPrimaryKey ? JSValue(m_cachedPrimaryKey) :
primaryKey(state);

JSValueInWrappedObject's operator JSC::JSValue() is not explicit, so I think
you can change JSValue(m_cachedPrimaryKey) to just m_cachedPrimaryKey.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:303
> +    JSValue key = m_cachedPrimaryKey ? JSValue(m_cachedPrimaryKey) :
primaryKey(state);

Ditto.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:358
> +JSValue IDBCursor::key(ExecState& state)
> +{
> +    return toJS(state, *state.lexicalGlobalObject(), m_currentKey.get());
> +}
> +
> +JSValue IDBCursor::primaryKey(ExecState& state)
> +{
> +    return toJS(state, *state.lexicalGlobalObject(),
m_currentPrimaryKey.get());
> +}
> +
> +JSC::JSValue IDBCursor::value(ExecState& state)
> +{
> +    return deserializeIDBValueToJSValue(state, m_currentValue);
> +}

We usually put these wrapper functions that call toJS() into our JSXXX.cpp or
JSXXXCustom.cpp files. We believe these functions are only invoked by
JavaScript bindings code. And if we autogenerated these functions, the
autogenerator would put them in the JSXXX.cpp file.

So, I'd suggest removing these functions from the IDBCursor class, and moving
their code into JSIDBCursorCustom.cpp.

In general, it is "bad code smell" to see significant use of JavaScript
interfaces inside a C++ DOM object. We usually think of JavaScript as something
that wraps C++ DOM objects, and is not part of their internal implementation.
Once JavaScript interfaces become part of the internal implementation of a C++
DOM object, we risk the kind of memory leak you're fixing here.

> Source/WebCore/Modules/indexeddb/IDBCursor.h:63
> +    JSValueInWrappedObject& cachedKey() { return m_cachedKey; }
> +    JSValueInWrappedObject& cachedPrimaryKey() { return m_cachedPrimaryKey;
}
> +    JSValueInWrappedObject& cachedValue() { return m_cachedValue; }

I would call these currentKeyWrapper, currentPrimaryKeyWrapper, and
currentValueWrapper -- because these data members/accessors are always null or
JavaScript wrappers for currentKey, currentPrimaryKey, and currentValue.

Maybe we can also remove the word "current" from all of these names. Any state,
at the moment you examine it, is current.

> Source/WebCore/Modules/indexeddb/IDBCursor.idl:35
> +    [CustomGetter] readonly attribute any key;
> +    [CustomGetter] readonly attribute any primaryKey;

It would be nice, in a future patch, to teach our auto generator how to
generate a getter for a JSValueInWrappedObject automatically. But it's OK that
these are custom for now.

> Source/WebCore/Modules/indexeddb/IDBRequest.h:63
> +    enum class OtherResultType {

Maybe we should call this NullResultType? Empty and Undefined are different
ways to express absence, which we usually call "null".

> Source/WebCore/Modules/indexeddb/IDBRequest.h:80
> +    JSValueInWrappedObject& cachedResult() { return m_cachedResult; }

I would call this resultWrapper.

One thing I don't like about the word "cache" is that a cache is usually an
optimization that you can delete at any time without changing correctness. But
in our case, once we allocate our result and its JS wrapper, it is essential
for correctness that they remain in sync.

> Source/WebCore/Modules/indexeddb/IDBRequest.h:-83
> -    

Please revert whitespace change to keep svn history clean.