Bug 104135 – JSObjectCopyPropertyNames returns non-enumerable properties.

• Status: ASSIGNED
• Component: JavaScriptCore

[Michael Brüning]

It seems that JSObjectCopyPropertyNames(JSContextRef ctx, JSObjectRef object) also returns non-enumerable properties even though the documentation states otherwise.

Came across this issue when running QtWebKit 1 evaluateJavaScript calls from the FancyBrowser QtWebKit example and the Qt bridge's convertValueToQVariantMap processed all properties instead of just the enumerable ones.

Might be caused by the void JSObject::getOwnPropertyNames(JSObject* object, ExecState* exec, PropertyNameArray& propertyNames, EnumerationMode mode) implementation which does not check the EnumerationMode for numeric properties retrieved from Butterfly, but I am not an expert on this.

[Simon Hausmann]

Interesting. There's a unit test in Source/JavaScriptCore/API/tests/testapi.c that should cover this. Hmm.

[Michael Brüning]

It might be because the test uses two strings and for those, the enumeration mode is enforced. I'll dig a bit more, for I might also be wrong here...

[Michael Brüning]

We (Allan and I) were just discussing whether this might be the cause of the performance degradation reported in [1], as the QtWebKit 2.3 implementation filtered out non-enumerable properties when converting QVariant maps.

I did a bit of reading in the ECMAScript 5 standard [2] to see how getOwnPropertyNames was meant to behave. Section 15.2.3.4 states that getOwnPropertyNames should return _all_ named properties, not only the enumerable ones. The property names in the array that is returned should all be enumerable.

The question is whether the documentation for JSObjectCopyPropertyNames is wrong and it should just return all property names or whether it's the JSObjectCopyPropertyNames implementation that needs fixing. Should the former be the case, then we'd need to re-add the checks for enumerable properties to the JS bridge IMO.

[1] https://bugs.webkit.org/show_bug.cgi?id=104540
[2] http://www.ecma-international.org/publications/files/ECMA-ST/ECMA-262.pdf

[Michael Brüning]

Tried filtering the non-enumerable properties in the Qt JS bridge's convertValueToQVariantMap, but this leads to another ASSERT that seems to have to do with another thread holding the lock on the JS heap. 

I suppose this needs to be corrected in the JS API anyway as at least JSObjectCopyPropertyNames should not return non-enumerable properties according to the documentation.

[Michael Brüning]

Created attachment 183003 [details]
Patch

[Darin Adler]

Comment on attachment 183003 [details]
Patch

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

Is there any way to have test code to cover this? Is this code path only used in JSObjectCopyPropertyNames? Isn’t it also used in JavaScript execution for the "for in" statement? We need test coverage.

> Source/WebCore/ChangeLog:11
> +        Also adds and additional filter in the Qt JS bridge that was there before.   

Typo: and

> Source/WebCore/bindings/js/JSDOMStringMapCustom.cpp:56
> +        Identifier propertyIdentifier = Identifier(exec, names[i]);

This should just use normal constructor syntax:

    Identifier propertyName(exec, names[i]);

> Source/WebCore/bindings/js/JSStorageCustom.cpp:93
> +        Identifier propertyIdentifier = Identifier(exec, thisObject->m_impl->key(i, ec));

This should just use normal constructor syntax:

    Identifier propertyName(exec, thisObject->m_impl->key(i, ec));

> Source/WebCore/bindings/js/JSStorageCustom.cpp:98
> +            setDOMException(exec, ec);
> +            if (exec->hadException())
> +                return;

These lines of code need to be outside the if statement; if we execute the code to get a key and get an exception, we need to detect that and propagate the exception even if the property is non-enumerable.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2222
> +            push(@implContent, "        Identifier propertyIdentifier = Identifier::from(exec, i);\n");

I think the local should be named propertyName, not propertyIdentifier.

[Michael Brüning]

Thanks for the input! 

I'll correct the things you mentioned and look into adding some test coverage for the next version.