Closed Bug 819475 Opened 11 years ago Closed 6 years ago

Should we stop returning null from document.domain?

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: bzbarsky, Assigned: qdot)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Spec seems to have this not nullable.  We return null for various non-hierarchical things...  What do other UAs do?
FWIW, data:text/html,<script>alert(document.domain=="")</script> gives me true in Opera/Chrome/Safari. I remember it was the same for XMLHttpRequest.responseXML.domain when I tested that.
Hm.  So in Gecko, at this point, a document with a null principal ends up with a nsNullPrincipalURI for the domainURI, which claims to have a host of "".

If I could produce a principal whose URI is an nsSimpleURI, we could get a null here.  That means a non-hierarchical URI that we render but which does not inherit the principal from parent.

Looks like something like about:buildconfig or about:neterror (try putting in "http://255.255.255.255/" in the url bar) and such are the main cases where we get null.

I suppose it's not a problem to return "" in the currently-null cases...
See also bug 417021.
Blocks: 1015945
The test from comment 2 returns true. Not sure if it always did, the code seems to suggest that it did...

So what remains here, changing IDL?
> The test from comment 2 returns true. Not sure if it always did

It certainly has for a while.

Again, the main cases in which null is returned are not really web-accessible.

We could change the idl (and our impl).
Was looking at fixing this up as part of the document.domain work I'm doing about bug 1459671. It doesn't look like changing from DOMString? to DOMString should be too hard, but it also looks like we have domain in HTMLDocument instead of Document, which means we're failing a WPT (testing/web-platform/tests/html/browsers/origin/relaxing-the-same-origin-restriction/document_domain.html).

I could get that moved as part of this and kill 2 bugs with one stone, but as this is fairly core functionality for a bunch of things, are there any cavaets for this move, or is it just one of those "needed someone to do it" things? The domain handling code is fairly self contained and easy to move, but getting up into how nsIDocument spreads through other document types gets a bit confusing, so I want to make sure I'm not missing anything.
Assignee: nobody → kyle
Flags: needinfo?(bzbarsky)
There are a bunch of things we still need to move from HTMLDocument to Document.  See bug 897815 and the things it tracks.

I would rather we kept the two behavior changes here (not returning null and moving to Document) separate.  That said, I don't think there's a problem with it other than "no one has done it yet" and I would be happy to review a fix to that effect.
Flags: needinfo?(bzbarsky)
Comment on attachment 8984317 [details]
Bug 819475 - Make document.domain non-nullable;

https://reviewboard.mozilla.org/r/250128/#review256460

::: dom/html/nsHTMLDocument.cpp:893
(Diff revision 1)
>  nsHTMLDocument::GetDomain(nsAString& aDomain)
>  {
>    nsCOMPtr<nsIURI> uri = GetDomainURI();
>  
>    if (!uri) {
> -    SetDOMStringToNull(aDomain);
> +    aDomain.SetLength(0);

.Truncate(), please.

::: dom/html/nsHTMLDocument.cpp:903
(Diff revision 1)
>    nsresult rv = nsContentUtils::GetHostOrIPv6WithBrackets(uri, hostName);
>    if (NS_SUCCEEDED(rv)) {
>      CopyUTF8toUTF16(hostName, aDomain);
>    } else {
>      // If we can't get the host from the URI (e.g. about:, javascript:,
> -    // etc), just return an null string.
> +    // etc), just set length to zero.

"just return an empty string"

::: dom/html/nsHTMLDocument.cpp:904
(Diff revision 1)
>    if (NS_SUCCEEDED(rv)) {
>      CopyUTF8toUTF16(hostName, aDomain);
>    } else {
>      // If we can't get the host from the URI (e.g. about:, javascript:,
> -    // etc), just return an null string.
> -    SetDOMStringToNull(aDomain);
> +    // etc), just set length to zero.
> +    aDomain.SetLength(0);

Again, .Truncate()
Attachment #8984317 - Flags: review?(bzbarsky) → review+
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5edda305eaf0
Make document.domain non-nullable; r=bz
https://hg.mozilla.org/mozilla-central/rev/5edda305eaf0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Documentation added.

Updated description at ref page:
https://developer.mozilla.org/en-US/docs/Web/API/Document/domain

Updated browser compat data with a note:
https://github.com/mdn/browser-compat-data/pull/2506

Add note to Fx62 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/62#DOM

Let me know if this looks sufficient. Many thanks!
Flags: needinfo?(kyle)
devdoc LGTM
Flags: needinfo?(kyle)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.