[JSC] Don't create a JSValue if it's not going to be used for nullable attributes

• Component: New Bugs
• Status: REOPENED ---

[Peter Beverloo]

[JSC] Don't create a JSValue if it's not going to be used for nullable attributes

[Peter Beverloo]

Created attachment 193850 [details]
Patch

[Peter Beverloo]

Comment on attachment 193850 [details]
Patch

Ack -- let's check out the Gtk build error.

[Peter Beverloo]

Created attachment 193873 [details]
Patch

[Kentaro Hara]

Comment on attachment 193873 [details]
Patch

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

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:746
> +    TestObj* nativeResult = impl->withScriptExecutionContextAttributeRaises(scriptContext, ec);
>      setDOMException(exec, ec);
> -    return result;
> +    return toJS(exec, castedThis->globalObject(), WTF::getPtr(nativeResult));

TestObj* is already a raw pointer. Do we need WTF::getPtr() ?

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:772
> +    TestObj* nativeResult = impl->withScriptExecutionContextAndScriptStateAttributeRaises(exec, scriptContext, ec);
>      setDOMException(exec, ec);
> -    return result;
> +    return toJS(exec, castedThis->globalObject(), WTF::getPtr(nativeResult));

Ditto.

[Peter Beverloo]

This caused the failure on the GTK bot (which uses JSC bindings and enables IndexedDB).  The IDBRequest object returns a PassRefPtr<> instead of raw pointers:

http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/indexeddb/IDBRequest.h#L59

Previously this was caught by the WebCore impl call being encapsulated by WTF::getPtr(), but now that we're storing the value to a local variable this isn't the case anymore.

All other bindings seem to return raw pointers. Should I modify IDBRequest (and potential other places) to do this instead, which makes the call to WTF::getPtr() obsolute?

[Kentaro Hara]

(In reply to comment #5)
> All other bindings seem to return raw pointers. Should I modify IDBRequest (and potential other places) to do this instead, which makes the call to WTF::getPtr() obsolute?

Sounds reasonable to me, as long as it does not have side effects.

[Peter Beverloo]

This paradigm seems to be quite frequently used within IndexedDB and I'm not sure how to fix it.  I'll give an example.

The IDBDatabase.version attribute[1] is defined in the IDL file as a readonly IDBAny, as it can return an integer or a string.  The implementation allocates a new instance of IDBAny containing the return value (in a PassRefPtr) and immediately returns this to the generated binding code.  The V8 bindings generator special-cases readonly attributes[2] and creates the following call:

RefPtr<IDBAny> result = imp->version();

The JSC code generator doesn't do this, and gets the raw pointer value by calling WTF::getPtr(PassRefPtr<>), which comes down to PassRefPtr<>.get().  The call to IDBDatabase::version() created by JSC therefore looks like this:

JSValue result = toJS(exec, castedThis->globalObject(), WTF::getPtr(impl->version()));

This causes us to loose the PassRefPtr<> instance, which is therefore destroyed and frees up the pointer it owned, as this was the only reference to it.  Unless the PassRefPtr<> somehow leaks instead of gets destroyed, this seems to be a use after free?  Or do we get "lucky" and is the PassRefPtr<> destroyed after the call to toJS()?

I won't have time next week to deeply look in to this, and it seems to be rather IDB specific as it is.

[1] http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp?rev=146636#L134
[2] http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm?rev=146636#L1113

-----

I just uploaded the third patch, as I realized results in the second patch weren't properly rebased.

I'm not happy with needing a NativeValueToLocal() method at all, which uses the heuristic of "pointers end with a star" to determine whether WTF::getPtr() should be used.  In toJS() we then call it again.  While this is a no-op that the compiler will optimize away, I'm not sure what the proper way is to get rid of this.  Any advice from the bindings experts here would be appreciated!

[Peter Beverloo]

Created attachment 194619 [details]
Patch

[Kentaro Hara]

sam: would you take a look?

[Peter Beverloo]

Committed r147464: <http://trac.webkit.org/changeset/147464>

[WebKit Review Bot]

Re-opened since this is blocked by bug 113812