802560 - DOMImplementation.createDocument() should have [TreatNullAs=EmptyString] for second arg

Status: RESOLVED FIXED

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

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The DOM spec defines DOMImplementation.createDocument as:

  XMLDocument createDocument(DOMString? namespace, [TreatNullAs=EmptyString] DOMString qualifiedName, DocumentType? doctype);
<http://dom.spec.whatwg.org/#domimplementation>

We have it as

  nsIDOMDocument            createDocument(in DOMString namespaceURI,
                                           in DOMString qualifiedName,
                                           in nsIDOMDocumentType doctype)
                                  raises(DOMException);

"in DOMString" for us seems to be the same as "DOMString?" in WebIDL.  Our code handles a null qualifiedName differently from an empty string, namely that it throws DOM_NAMESPACE_ERR if qualifiedName is null but namespaceURI is not; and if both are null, it just passes qualifiedName to nsContentUtils::CreateDocument(), which I *think* ultimately treats it as empty.  (It calls .IsEmpty() -- which is true for null strings, right?)

So I think the only difference here is our behavior of throwing.  This causes us to fail one of Ms2ger's test.  WebKit and Opera seem to agree with the spec; IE seems to throw like us.  The spec's behavior seems more useful, so we may as well align on it.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I just realized this conflicts with bug 796903, so I'll wait for that to land before poking DOMImplementation stuff.

Comment 2

[Clochix]

What's the status of this ticket ?

Starting with the new stable Firefox 19, calling DOMImplementation.createDocument with an undefined second argument throws an error : 

[Exception... "An attempt was made to create or change an object in a way which is incorrect with regard to namespaces" code: "14" nsresult: "0x8053000e (NamespaceError)"]

This worked just fine before.

I've updated my code to pass an empty string instead of undefined, but this may break some website now that Firefox 19 is out.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Ms2ger, could you take a look at this?

Comment 4

[Alex Keybl [:akeybl]]

We don't know of any major website breakage from this change yet (nor over the past 18 weeks), so we'll leave this on the FF19 nom list for now.

Comment 5

[Alex Keybl [:akeybl]]

If we don't see any more fallout in the next couple of weeks, we may decide to untrack for 20 and up.

Comment 6

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

Attachment: Patch v1

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 720011 [details] [diff] [review]
Patch v1

r=me

Is there a bug on those remaining failures?

Comment 8

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

(In reply to Boris Zbarsky (:bz) from comment #7)
> Comment on attachment 720011 [details] [diff] [review]
> Patch v1
> 
> r=me
> 
> Is there a bug on those remaining failures?

Thanks for reminding me to look at them... These tests look like they're wrong.

Fixed upstream: https://dvcs.w3.org/hg/webapps/rev/80701dd9ac79

Comment 9

[Alex Keybl [:akeybl]]

(In reply to Alex Keybl [:akeybl] from comment #5)
> If we don't see any more fallout in the next couple of weeks, we may decide
> to untrack for 20 and up.

Untracking for release, but we'll consider a low risk uplift if found.

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/50da21920e0c

Comment 11

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

Comment on attachment 720011 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 796903
User impact if declined: some pages could be broken (exception thrown)
Testing completed (on m-c, etc.): on m-c for a bit
Risk to taking this patch (and alternatives if risky): pretty low. Alternative is a backout of bug 796903.
String or UUID changes made by this patch: none

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 720011 [details] [diff] [review]
Patch v1

low risk uplift for a Fx19 regression.Approving for aurora

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 720011 [details] [diff] [review]
Patch v1

This has been in product for a while and it's now too late for the fourth week beta which already went to build so let's leave it at Aurora and ship this fix first in FF21.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b2ed9e2a49ab