Bug 65187 - Make document.documentURI readonly
Summary: Make document.documentURI readonly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2011-07-26 08:15 PDT by Anne van Kesteren
Modified: 2012-06-12 11:03 PDT (History)
11 users (show)

See Also:


Attachments
Patch (14.27 KB, patch)
2012-06-07 13:22 PDT, Mike West
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (927.87 KB, application/zip)
2012-06-07 18:56 PDT, WebKit Review Bot
no flags Details
Patch (46.83 KB, patch)
2012-06-11 08:29 PDT, Mike West
no flags Details | Formatted Diff | Diff
Adding ObjC bindings. (47.95 KB, patch)
2012-06-11 09:18 PDT, Mike West
no flags Details | Formatted Diff | Diff
ifdeffing the IDL for ObjC (46.03 KB, patch)
2012-06-11 11:21 PDT, Mike West
no flags Details | Formatted Diff | Diff
Adding TestWebKitAPI test. (58.00 KB, patch)
2012-06-11 14:23 PDT, Mike West
no flags Details | Formatted Diff | Diff
Bringing back W3C tests. (49.03 KB, patch)
2012-06-11 16:13 PDT, Mike West
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (672.14 KB, application/zip)
2012-06-11 21:04 PDT, WebKit Review Bot
no flags Details
Trying again, with a platform-specific Chromium test. (49.75 KB, patch)
2012-06-12 00:27 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anne van Kesteren 2011-07-26 08:15:16 PDT
When you set documentURI it throws in both Firefox and Opera. It should at least be made readonly and maybe be removed altogether if that is feasible.
Comment 1 Mike West 2012-06-07 13:07:49 PDT
The latest version of the DOM4 working draft marks this attribute as readonly (http://www.w3.org/TR/dom/#document). Firefox 12 isn't throwing an error, but is disallowing writes to the attribute's value. I think it might be worthwhile to match that behavior.

I have a trivial patch to remove write support for the attribute, and I'm skimming through tests to see what breaks. So far, it's only breaking tests that were testing that assignment to documentURI didn't break a feature or create a security hole.

I'll upload the patch as-is, and wait for a decision regarding the change before working through the remaining tests.
Comment 2 Mike West 2012-06-07 13:22:25 PDT
Created attachment 146367 [details]
Patch
Comment 3 Build Bot 2012-06-07 13:37:31 PDT
Comment on attachment 146367 [details]
Patch

Attachment 146367 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12907726
Comment 4 Alexey Proskuryakov 2012-06-07 15:51:39 PDT
Mike, thank you for the research you performed. Some additional relevant questions to answer are:

- Does IE support this attribute, and if it does, does it match the proposed spec?
- What made Firefox disagree with the spec, and silently ignore writes instead of raising an exception?
- If you search for source code on the Web, what uses of documentURI do show up?
Comment 5 Adam Barth 2012-06-07 17:03:34 PDT
Comment on attachment 146367 [details]
Patch

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

> Source/WebCore/dom/Document.h:-430
>      String documentURI() const { return m_documentURI; }
> -    void setDocumentURI(const String&);

Note: Without this setter, I believe we can remove m_documentURI and a bunch of complexity in calculating the base URI.
Comment 6 WebKit Review Bot 2012-06-07 18:56:05 PDT
Comment on attachment 146367 [details]
Patch

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

New failing tests:
dom/xhtml/level3/core/documentsetdocumenturi02.xhtml
dom/xhtml/level3/core/documentsetdocumenturi01.xhtml
http/tests/security/xss-DENIED-document-baseURI-javascript.html
dom/xhtml/level3/core/nodegetbaseuri02.xhtml
http/tests/security/xss-DENIED-document-baseURI-javascript-with-spaces.html
dom/xhtml/level3/core/documentsetdocumenturi03.xhtml
Comment 7 WebKit Review Bot 2012-06-07 18:56:10 PDT
Created attachment 146450 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 Ms2ger (he/him; ⌚ UTC+1/+2) 2012-06-07 23:30:39 PDT
Do you still need 'TreatNullAs=NullString'?
Comment 9 Ryosuke Niwa 2012-06-07 23:34:23 PDT
(In reply to comment #4)
> Mike, thank you for the research you performed. Some additional relevant questions to answer are:
> 
> - Does IE support this attribute, and if it does, does it match the proposed spec?

IE9 doesn't seem to support this properly. So user can set it to whatever value (it doesn't do anything special since it's an unsupported JS property).

We should definitely not throw here.
Comment 10 Mike West 2012-06-10 09:01:13 PDT
(In reply to comment #4)
> Mike, thank you for the research you performed. Some additional relevant questions to answer are:
> 
> - Does IE support this attribute, and if it does, does it match the proposed spec?

Answered above (thanks!).

> - What made Firefox disagree with the spec, and silently ignore writes instead of raising an exception?

IE's behavior is probably as good a reason as any to silently fail rather than throwing an error.

> - If you search for source code on the Web, what uses of documentURI do show up?

Searching for "document.documentURI" on my favorite search engine returns an MDN article, which says that the property is read-only, the bugzilla bug that made the property read only in Mozilla, three WHATWG mailing list conversations expressing surprise that the property wasn't read-only in WebKit, and asking whether it can be merged completely with the URL property.. The remaining results refer to a similarly-named Silverlight property.

In short, developers are being advised to use `document.URI`, and I haven't been able to find anyone relying on the writable behavior in a good half-hour of searching. I'm happy to dig deeper if you think it's necessary, but hopefully not too much deeper. My impression is that this is a low-risk change. :)

Based on Adam's comments, I took a quick look at the rest of the code that uses `m_documentURI`, and I agree with the thrust of his comment. If we drop writes, we can drop this property entirely, which would specifically let us drop a little-used case from Document::updateBaseURL. Since a good chunk of the (non w3c) LayoutTests that exercise this aspect of the property are security-related, I think that's a good enough reason to proceed.
Comment 11 Mike West 2012-06-10 09:11:10 PDT
(In reply to comment #10)
> (In reply to comment #4)
> In short, developers are being advised to use `document.URI`, and I haven't been able to find anyone relying on the writable behavior in a good half-hour of searching.

I took a quick look at StackOverflow as well: 6 questions refer to "document.documentURI" and not to Silverlight (http://google.com/search?q=%22document.documentURI%22+-silverlight+site:stackoverflow.com%2Fquestions). Of those, none set the property.
Comment 12 Ms2ger (he/him; ⌚ UTC+1/+2) 2012-06-10 09:33:43 PDT
(In reply to comment #4)
> - What made Firefox disagree with the spec, and silently ignore writes instead of raising an exception?

Not sure what spec you refer to; readonly attributes in IDL always silently ignore writes. (Unless possibly in ES5 strict mode; interoperability here wasn't great when I last tested.)
Comment 13 Alexey Proskuryakov 2012-06-10 12:32:14 PDT
> Searching for "document.documentURI" on my favorite search engine returns an MDN article <...>

I think that the proper way to search for actual use (now that Google Code Search no longer exists) is <http://www.google.com/search?client=safari&q=documentURI+filetype:js>.
Comment 14 Mike West 2012-06-10 12:45:25 PDT
(In reply to comment #13)
> > Searching for "document.documentURI" on my favorite search engine returns an MDN article <...>
> 
> I think that the proper way to search for actual use (now that Google Code Search no longer exists) is <http://www.google.com/search?client=safari&q=documentURI+filetype:js>.

Clever!

Limiting that search to "document.documentURI" supposedly yields 3,710 results. It doesn't scroll past page 3, however. It's unscientific in the extreme, but I cmd-G'd my way through those 27 results; one did an assignment to a variable named `documentURI` (https://github.com/RequestPolicy/requestpolicy/blob/master/src/content/overlay.js#L687), the rest were reading and manipulating (mostly with slice, like http://code.google.com/r/leo-asd/source/browse/examples/demo/chrome/content/lib/loadDeps.js?r=70fe31a19ed89e463352ed8923f484bd3387b7ba&spec=svn22094f03a0ac81fa6e9092710b1701674affa98d).
Comment 15 Mike West 2012-06-11 08:29:08 PDT
Created attachment 146859 [details]
Patch
Comment 16 Mike West 2012-06-11 08:35:50 PDT
I also took a quick look at Opera, just to round out the discussion here: as Anne noted in the initial comment, Opera throws an error ("Unhandled DOMException: NO_MODIFICATION_ALLOWED_ERR"). I'm not convinced that that's the correct behavior: silently ignoring the write, as per Firefox, seems like a better solution. Regardless, it's clear that WebKit is the only browser that supports writing to this value. I'd like to change that.

Attached is what I think a complete patch would look like, removing m_documentURI entirely, and simplifying the baseURL calculation down to using m_url instead. I've removed all the tests that relied on the old behavior, and added a single test to check the property's readonlyness. The baseURL and document.URL behaviors are covered by existing tests. I'm hoping the bot run will back that up. :)

Thanks for taking another look!
Comment 17 Build Bot 2012-06-11 09:08:22 PDT
Comment on attachment 146859 [details]
Patch

Attachment 146859 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12943280
Comment 18 Mike West 2012-06-11 09:18:32 PDT
Created attachment 146864 [details]
Adding ObjC bindings.
Comment 19 WebKit Review Bot 2012-06-11 09:21:11 PDT
Please wait for approval from timothy@apple.com (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.
Comment 20 Timothy Hatcher 2012-06-11 09:30:16 PDT
Comment on attachment 146864 [details]
Adding ObjC bindings.

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

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:98
> +@property(readonly, copy) NSString *documentURI AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER;

This change is fine.
Comment 21 Timothy Hatcher 2012-06-11 09:32:01 PDT
(In reply to comment #20)
> (From update of attachment 146864 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146864&action=review
> 
> > Source/WebCore/bindings/objc/PublicDOMInterfaces.h:98
> > +@property(readonly, copy) NSString *documentURI AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER;
> 
> This change is fine.

Actually, it isn't OK. We need to support setDocumentURI: in ObjC. There are clients using it.
Comment 22 Mike West 2012-06-11 09:58:14 PDT
(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 146864 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146864&action=review
> > 
> > > Source/WebCore/bindings/objc/PublicDOMInterfaces.h:98
> > > +@property(readonly, copy) NSString *documentURI AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER;
> > 
> > This change is fine.
> 
> Actually, it isn't OK. We need to support setDocumentURI: in ObjC. There are clients using it.

Alright, that's unfortunate.

Forgive the n00b question: is there a mechanism to deprecate/remove the functionality on the web while retaining an ObjC interface? Or would you simply prefer that WebKit's current behavior remain in place?
Comment 23 Ryosuke Niwa 2012-06-11 10:04:09 PDT
(In reply to comment #22)
> Forgive the n00b question: is there a mechanism to deprecate/remove the functionality on the web while retaining an ObjC interface? Or would you simply prefer that WebKit's current behavior remain in place?

Yes, you wrap the IDL line in
#if !defined(LANGUAGE_JAVASCRIPT) || !LANGUAGE_JAVASCRIPT
        CSSStyleDeclaration createCSSStyleDeclaration();
#endif

or

#if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C
        // FIXME: remove the these two versions once [Optional] is implemented for Objective-C.
        boolean            execCommand(in DOMString command,
                                       in boolean userInterface);
        boolean            execCommand(in DOMString command);
#endif
Comment 24 Mike West 2012-06-11 10:28:13 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > Forgive the n00b question: is there a mechanism to deprecate/remove the functionality on the web while retaining an ObjC interface? Or would you simply prefer that WebKit's current behavior remain in place?
> 
> Yes, you wrap the IDL line in

Brilliant, thank you.

This means I need to put the m_documentURI property back in place. Do we need to retain the baseURL behavior as well?

If so, I'm a little worried about how to test this; if the behavior under the covers breaks, we won't have any LayoutTests to catch the issue.
Comment 25 Mike West 2012-06-11 11:21:34 PDT
Created attachment 146879 [details]
ifdeffing the IDL for ObjC
Comment 26 Alexey Proskuryakov 2012-06-11 12:19:41 PDT
> If so, I'm a little worried about how to test this; if the behavior under the covers breaks, we won't have any LayoutTests to catch the issue.

That's a very good question. Yes, it would be bad to lose coverage, and a test can be added as an "API Test" - it will only run on Macs, but that's better than nothing. The code is in Tools/TestWebKitAPI, and it's pretty easy to add a new (WebKit1 only) test.
Comment 27 Mike West 2012-06-11 14:23:58 PDT
Created attachment 146910 [details]
Adding TestWebKitAPI test.
Comment 28 Mike West 2012-06-11 14:26:09 PDT
(In reply to comment #26)
> > If so, I'm a little worried about how to test this; if the behavior under the covers breaks, we won't have any LayoutTests to catch the issue.
> 
> That's a very good question. Yes, it would be bad to lose coverage, and a test can be added as an "API Test" - it will only run on Macs, but that's better than nothing. The code is in Tools/TestWebKitAPI, and it's pretty easy to add a new (WebKit1 only) test.

Thanks for the tip. I've added a few tests in the attached patch.
Comment 29 Alexey Proskuryakov 2012-06-11 14:38:51 PDT
Comment on attachment 146910 [details]
Adding TestWebKitAPI test.

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

> Source/WebCore/dom/Document.cpp:1318
> +    // This property is read-only from JavaScript, but writable from ObjC.

We generally don't like abbreviations, so "Objective-C".

> Source/WebCore/dom/Document.cpp:2691
> +        // The documentURI attribute is read-only from JavaScript, but writable from ObjC, so we need to retain

Ditto.

> Source/WebCore/dom/Document.h:1232
> +    // This property is read-only from JavaScript, but writable from ObjC.

Ditto.

> Source/WebCore/dom/Document.idl:82
> +        // DOM Level 4 style interface.

Is it actually "DOM Level 4"? I don't think that it's an accurate name.

Also, perhaps you could explain this in plain language, like "documentURI used to be read/write in DOM 3 Core and in WebKit, and became read-only in DOM4".

> LayoutTests/ChangeLog:10
> +        * dom/xhtml/level3/core/documentsetdocumenturi01.xhtml: Removed.

I'm not sure if this is the best thing to do with these tests. I'd probably have just landed failure results - we like to keep running tests, because they may catch future unexpected changes or crashers.
Comment 30 Alexey Proskuryakov 2012-06-11 14:40:17 PDT
Looks good to me otherwise, would r+ if tests were not deleted.
Comment 31 Mike West 2012-06-11 16:13:42 PDT
Created attachment 146950 [details]
Bringing back W3C tests.
Comment 32 Mike West 2012-06-11 16:17:21 PDT
Comment on attachment 146950 [details]
Bringing back W3C tests.

Thanks for working with me to get this patch in shape. I've improved comments as you suggested, and I've brought back the W3C tests. As we discussed in IRC, I've left the remaining tests on the cutting room floor, as they don't add anything to our coverage.

Two of the W3C tests write out the value of documentURI in the failure message: I've disabled both rather than deleting them, and updated the results for the other two.

Thanks!
Comment 33 WebKit Review Bot 2012-06-11 21:04:19 PDT
Comment on attachment 146950 [details]
Bringing back W3C tests.

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

New failing tests:
dom/xhtml/level3/core/nodegetbaseuri02.xhtml
Comment 34 WebKit Review Bot 2012-06-11 21:04:29 PDT
Created attachment 147004 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 35 Mike West 2012-06-12 00:27:59 PDT
Created attachment 147027 [details]
Trying again, with a platform-specific Chromium test.
Comment 36 Mike West 2012-06-12 02:25:56 PDT
Comment on attachment 147027 [details]
Trying again, with a platform-specific Chromium test.

Bots are happy this time. CQ? :)
Comment 37 WebKit Review Bot 2012-06-12 11:03:14 PDT
Comment on attachment 147027 [details]
Trying again, with a platform-specific Chromium test.

Clearing flags on attachment: 147027

Committed r120093: <http://trac.webkit.org/changeset/120093>
Comment 38 WebKit Review Bot 2012-06-12 11:03:21 PDT
All reviewed patches have been landed.  Closing bug.