Make document.documentURI readonly

• Component: DOM
• Status: RESOLVED FIXED

[Anne van Kesteren]

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.

[Mike West]

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.

[Mike West]

Created attachment 146367 [details]
Patch

[Build Bot]

Comment on attachment 146367 [details]
Patch

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

[Alexey Proskuryakov]

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?

[Adam Barth]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Do you still need 'TreatNullAs=NullString'?

[Ryosuke Niwa]

(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.

[Mike West]

(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.

[Mike West]

(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.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.)

[Alexey Proskuryakov]

> 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>.

[Mike West]

(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).

[Mike West]

Created attachment 146859 [details]
Patch

[Mike West]

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!

[Build Bot]

Comment on attachment 146859 [details]
Patch

Attachment 146859 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12943280

[Mike West]

Created attachment 146864 [details]
Adding ObjC bindings.

[WebKit Review Bot]

Please wait for approval from [e-mail] (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.

[Timothy Hatcher]

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.

[Timothy Hatcher]

(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.

[Mike West]

(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?

[Ryosuke Niwa]

(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

[Mike West]

(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.

[Mike West]

Created attachment 146879 [details]
ifdeffing the IDL for ObjC

[Alexey Proskuryakov]

> 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.

[Mike West]

Created attachment 146910 [details]
Adding TestWebKitAPI test.

[Mike West]

(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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Looks good to me otherwise, would r+ if tests were not deleted.

[Mike West]

Created attachment 146950 [details]
Bringing back W3C tests.

[Mike West]

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!

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Mike West]

Created attachment 147027 [details]
Trying again, with a platform-specific Chromium test.

[Mike West]

Comment on attachment 147027 [details]
Trying again, with a platform-specific Chromium test.

Bots are happy this time. CQ? :)

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.