Bug 161457 - Align cross-Origin Object.getOwnPropertyNames() with the HTML specification
Summary: Align cross-Origin Object.getOwnPropertyNames() with the HTML specification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2016-08-31 14:37 PDT by Chris Dumez
Modified: 2016-09-03 15:52 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.19 KB, patch)
2016-08-31 15:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.28 KB, patch)
2016-09-02 11:44 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.30 KB, patch)
2016-09-02 12:10 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.31 KB, patch)
2016-09-02 12:13 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.16 KB, patch)
2016-09-03 15:22 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-08-31 14:37:30 PDT
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.
Comment 1 Chris Dumez 2016-08-31 15:00:56 PDT
Created attachment 287547 [details]
Patch
Comment 2 Chris Dumez 2016-09-02 11:44:07 PDT
Created attachment 287791 [details]
Patch
Comment 3 Chris Dumez 2016-09-02 12:10:48 PDT
Created attachment 287795 [details]
Patch
Comment 4 Chris Dumez 2016-09-02 12:13:20 PDT
Created attachment 287798 [details]
Patch
Comment 5 Darin Adler 2016-09-03 08:21:44 PDT
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.
Comment 6 Chris Dumez 2016-09-03 14:28:08 PDT
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 :)
Comment 7 Chris Dumez 2016-09-03 15:22:15 PDT
Created attachment 287869 [details]
Patch
Comment 8 WebKit Commit Bot 2016-09-03 15:52:50 PDT
Comment on attachment 287869 [details]
Patch

Clearing flags on attachment: 287869

Committed r205409: <http://trac.webkit.org/changeset/205409>
Comment 9 WebKit Commit Bot 2016-09-03 15:52:55 PDT
All reviewed patches have been landed.  Closing bug.