Should we stop returning null from document.domain?

RESOLVED FIXED in Firefox 62

Status

()

defect
RESOLVED FIXED
7 years ago
2 months ago

People

(Reporter: bzbarsky, Assigned: qdot)

Tracking

({dev-doc-complete})

unspecified
mozilla62
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Spec seems to have this not nullable.  We return null for various non-hierarchical things...  What do other UAs do?

Comment 1

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

Comment 3

5 years ago
See also bug 417021.

Updated

5 years ago
Blocks: 1015945

Comment 4

5 years ago
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 hidden (mozreview-request)
Reporter

Comment 9

a year ago
mozreview-review
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+
Comment hidden (mozreview-request)

Comment 11

a year ago
Pushed by kmachulis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5edda305eaf0
Make document.domain non-nullable; r=bz

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5edda305eaf0
Status: NEW → RESOLVED
Last Resolved: a year 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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.