Bug 161457 – Align cross-Origin Object.getOwnPropertyNames() with the HTML specification

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Align cross-Origin Object.getOwnPropertyNames() with the HTML specification:
- https://html.spec.whatwg.org/#windowproxy-ownpropertykeys
- https://html.spec.whatwg.org/#location-ownpropertykeys
- https://html.spec.whatwg.org/#crossoriginproperties-(-o-)

We should list cross origin properties.

Firefox complies with the specification. However, WebKit returns an empty array and logs a security error message.

[Chris Dumez]

Created attachment 287547 [details]
Patch

[Chris Dumez]

Created attachment 287791 [details]
Patch

[Chris Dumez]

Created attachment 287795 [details]
Patch

[Chris Dumez]

Created attachment 287798 [details]
Patch

[Darin Adler]

Comment on attachment 287798 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:322
> +static void addCrossOriginPropertyNames(ExecState* exec, PropertyNameArray& propertyNames)

In new code we normally want to write "ExecState& state" rather than "ExecState* exec", or "VM& vm". It’s a bit costly to go from ExecState to VM each time, and so it’s a better idiom to either take a VM& or put the VM& in a local variable and use that.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:337
> +    propertyNames.add(Identifier::fromString(exec, "window"));
> +    propertyNames.add(Identifier::fromString(exec, "self"));
> +    propertyNames.add(Identifier::fromString(exec, "location"));
> +    propertyNames.add(Identifier::fromString(exec, "close"));
> +    propertyNames.add(Identifier::fromString(exec, "closed"));
> +    propertyNames.add(Identifier::fromString(exec, "focus"));
> +    propertyNames.add(Identifier::fromString(exec, "blur"));
> +    propertyNames.add(Identifier::fromString(exec, "frames"));
> +    propertyNames.add(Identifier::fromString(exec, "length"));
> +    propertyNames.add(Identifier::fromString(exec, "top"));
> +    propertyNames.add(Identifier::fromString(exec, "opener"));
> +    propertyNames.add(Identifier::fromString(exec, "parent"));
> +    propertyNames.add(Identifier::fromString(exec, "postMessage"));

It’s pretty inefficient to look up strings in the identifier table every time this function is called. Maybe this is not performance critical?

These are listed in what looks like arbitrary order. Should they be sorted in some way?

This looks like an unrolled loop. Could we use an actual loop instead? Typically makes smaller code.

> Source/WebCore/bindings/js/JSLocationCustom.cpp:111
> +static void addCrossOriginPropertyNames(ExecState* exec, PropertyNameArray& propertyNames)

Same comment as above.

On Windows we do "all in one" compiles, so this function conflicts with the one in JSDOMWindowCustom. We can work around that by giving them different names.

[Chris Dumez]

Comment on attachment 287798 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:337
>> +    propertyNames.add(Identifier::fromString(exec, "postMessage"));
> 
> It’s pretty inefficient to look up strings in the identifier table every time this function is called. Maybe this is not performance critical?
> 
> These are listed in what looks like arbitrary order. Should they be sorted in some way?
> 
> This looks like an unrolled loop. Could we use an actual loop instead? Typically makes smaller code.

Ok, I'll sort them and use a loop. FYI, I used the order that is in the spec but it wasn't all that arbitrary :)

[Chris Dumez]

Created attachment 287869 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 287869 [details]
Patch

Clearing flags on attachment: 287869

Committed r205409: <http://trac.webkit.org/changeset/205409>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.