Bug 109789 - [V8] Remove reload, assign and replace attributes from Location
Summary: [V8] Remove reload, assign and replace attributes from Location
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-14 00:11 PST by Kentaro Hara
Modified: 2013-05-02 11:29 PDT (History)
7 users (show)

See Also:


Attachments
Patch (3.80 KB, patch)
2013-02-14 00:12 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (3.90 KB, patch)
2013-02-14 03:02 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (7.63 KB, patch)
2013-02-19 14:23 PST, Kentaro Hara
haraken: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2013-02-14 00:11:40 PST
We should remove hard-coded implementation for Location::reload, Location::replace, and Location::assign in CodeGeneratorV8.pm.
Comment 1 Kentaro Hara 2013-02-14 00:12:55 PST
Created attachment 188275 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-14 01:31:42 PST
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
Comment 3 Kentaro Hara 2013-02-14 03:02:14 PST
Created attachment 188296 [details]
Patch
Comment 4 Adam Barth 2013-02-14 09:31:27 PST
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....
Comment 5 Adam Barth 2013-02-14 09:32:08 PST
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?
Comment 6 Kentaro Hara 2013-02-14 16:37:04 PST
(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.
Comment 7 Adam Barth 2013-02-15 09:02:47 PST
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?
Comment 8 Kentaro Hara 2013-02-18 23:55:18 PST
(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.
Comment 9 Kentaro Hara 2013-02-19 14:23:33 PST
Created attachment 189161 [details]
Patch
Comment 10 Kentaro Hara 2013-02-19 14:26:12 PST
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?
Comment 11 WebKit Review Bot 2013-02-19 16:24:30 PST
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
Comment 12 Kentaro Hara 2013-02-19 18:11:41 PST
Comment on attachment 189161 [details]
Patch

Looks like I need to work through more.
Comment 13 Anders Carlsson 2013-05-02 11:29:51 PDT
V8 is gone from WebKit.