Bug 109789 – [V8] Remove reload, assign and replace attributes from Location

• Component: WebCore JavaScript
• Status: RESOLVED WONTFIX

[Kentaro Hara]

We should remove hard-coded implementation for Location::reload, Location::replace, and Location::assign in CodeGeneratorV8.pm.

[Kentaro Hara]

Created attachment 188275 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188275 [details]
Patch

Attachment 188275 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16580248

New failing tests:
http/tests/security/cross-frame-access-location-get.html
http/tests/inspector/console-cross-origin-iframe-logging.html
http/tests/security/cross-frame-access-location-get-override.html
http/tests/security/xss-DENIED-defineProperty.html
http/tests/misc/location-replace-crossdomain.html

[Kentaro Hara]

Created attachment 188296 [details]
Patch

[Adam Barth]

Comment on attachment 188296 [details]
Patch

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

> Source/WebCore/page/Location.idl:54
>      [Custom, V8Unforgeable] void assign(in [Optional=DefaultIsUndefined] DOMString url);
>      [Custom, V8Unforgeable] void replace(in [Optional=DefaultIsUndefined] DOMString url);
>      [Custom, V8Unforgeable] void reload();
> +#if defined(V8_BINDING) && V8_BINDING
> +    [Custom, V8Unforgeable, DoNotCheckSecurity] readonly attribute any assign;
> +    [Custom, V8Unforgeable, DoNotCheckSecurity] readonly attribute any replace;
> +    [Custom, V8Unforgeable, DoNotCheckSecurity] readonly attribute any reload;
> +#endif

It's really strange to have both functions and attributes with the same name....

[Adam Barth]

I'm not sure having both functions and attributes with the same name is the right thing to do.  If you think it's the right thing, can you add more explanation as to why?

[Kentaro Hara]

(In reply to comment #5)
> I'm not sure having both functions and attributes with the same name is the right thing to do.  If you think it's the right thing, can you add more explanation as to why?

I don't think it's a right thing to do, but V8 currently support it (i.e. my patch doesn't change the current behavior of V8). For example, http/tests/security/cross-frame-access-location-get-override.html depends on the fact that assign/reload/replace work as attributes.

JSC has only functions for these names, which looks correct to me.

[Adam Barth]

What is the difference between having an attribute and having a function with the same name?  I though that they both are implemented as properties...  Maybe one is on the prototype and one is on the instance?

[Kentaro Hara]

(In reply to comment #7)
> What is the difference between having an attribute and having a function with the same name? Maybe one is on the prototype and one is on the instance?

No. Given that [V8Unforgeable] is set on the reload() method, both the method and the attribute are defined on the instance. (In terms of implementation, a method is registered by Set(), and an attribute is registered by SetAccessor().)

I have no idea about why we need both. Let me try to remove the attribute and see what test fails.

[Kentaro Hara]

Created attachment 189161 [details]
Patch

[Kentaro Hara]

dglazkov, mike: Attributes for Location::reload, Location::assign and Location::replace were introduced in http://trac.webkit.org/changeset/41693. It looks strange to have both attributes and methods for these Location properties. Would it make sense to remove the attributes?

[WebKit Review Bot]

Comment on attachment 189161 [details]
Patch

Attachment 189161 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16640014

New failing tests:
http/tests/inspector/console-cross-origin-iframe-logging.html
http/tests/security/cross-frame-access-location-put.html
http/tests/security/cross-frame-access-location-get.html
http/tests/security/xss-DENIED-defineProperty.html
http/tests/misc/location-replace-crossdomain.html
http/tests/security/cross-frame-access-location-get-override.html

[Kentaro Hara]

Comment on attachment 189161 [details]
Patch

Looks like I need to work through more.

[Anders Carlsson]

V8 is gone from WebKit.