Bug 16833 – Acid3 expects createElementNS to throw exception for localname = ":div" (affects Acid3 test 23)

• Component: DOM
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Acid3 expects createElementNS to throw exception for localname = ":div"

We fail to throw exception 14 in the ":div" case.

    function () {
      // test 23: createElementNS() with invalid tag names
      var test = function (name, ns, code) {
        var result;
        try {
          var div = document.createElementNS(ns, name);
        } catch (e) {
          result = e;
        }
        return result && result.code == code;
      }
      assert(test('<div>', null, 5), "no, or wrong, exception for: <div>");
      assert(test('0div', null, 5), "no, or wrong, exception for: 0div");
      assert(test('di v', null, 5), "no, or wrong, exception for: di v");
      assert(test('di<v', null, 5), "no, or wrong, exception for: di<v");
      assert(test('-div', null, 5), "no, or wrong, exception for: -div");
      assert(test('.div', null, 5), "no, or wrong, exception for: .div");
      assert(test('<div>', "http://example.com/", 5), "no, or wrong, exception for: <div>");
      assert(test('0div', "http://example.com/", 5), "no, or wrong, exception for: 0div");
      assert(test('di<v', "http://example.com/", 5), "no, or wrong, exception for: di<v");
      assert(test('-div', "http://example.com/", 5), "no, or wrong, exception for: -div");
      assert(test('.div', "http://example.com/", 5), "no, or wrong, exception for: .div");
      assert(test(':div', null, 14), "no, or wrong, exception for: :div");
      assert(test(':div', "http://example.com/", 14), "no, or wrong, exception for: :div");
      assert(test('d:iv', null, 14), "no, or wrong, exception for: d:iv");
      assert(test('xml:test', "http://example.com/", 14), "no, or wrong, exception for: xml:test");
      assert(test('xmlns:test', "http://example.com/", 14), "no, or wrong, exception for: xmlns:test");
      assert(test('x:test', "http://www.w3.org/2000/xmlns/", 14), "no, or wrong, exception for: x:test");
      document.createElementNS('xmlns:test', "http://www.w3.org/2000/xmlns/");
      return 2;
    },

[Eric Seidel (no email)]

Ok, a few things were wrong with his test.  I've fixed them and sent him a diff.  I've written some additional tests for us, and have prepared a patch, which I will attach shortly.

[Eric Seidel (no email)]

About to attach patches.  They don't fix the test.  But they do make us agree with Moz more or less.  I still need to check IE to make sure all 3 of us agree.  Hixie's test had several problems which I'm still working on communicating to him.

[Eric Seidel (no email)]

Created attachment 18413 [details]
First pass at throwing additional exceptions

 WebCore/dom/Document.cpp |   42 ++++++++++++++++++++++++++++++------------
 1 files changed, 30 insertions(+), 12 deletions(-)

[Eric Seidel (no email)]

Created attachment 18414 [details]
A more complete fix

 WebCore/dom/Document.cpp |   63 +++++++++++++++++++++++++++++++++++-----------
 WebCore/dom/Document.idl |    4 +-
 2 files changed, 50 insertions(+), 17 deletions(-)

[Eric Seidel (no email)]

Created attachment 18415 [details]
Final fixes and adding test cases

 .../documentsetstricterrorchecking02-expected.txt  |    3 +-
 .../createAttributeNS-namespace-err-expected.txt   |   38 ++++++++
 .../Document/createAttributeNS-namespace-err.html  |   13 +++
 .../createElementNS-namespace-err-expected.txt     |   37 ++++++++
 .../Document/createElementNS-namespace-err.html    |   13 +++
 .../fast/dom/Document/resources/TEMPLATE.html      |   13 +++
 .../resources/createAttributeNS-namespace-err.js   |   89 ++++++++++++++++++++
 .../resources/createElementNS-namespace-err.js     |   88 +++++++++++++++++++
 WebCore/dom/Document.cpp                           |   42 ++++------
 9 files changed, 307 insertions(+), 29 deletions(-)

[Eric Seidel (no email)]

Created attachment 18416 [details]
full patch, including test cases

This is a combined version of the previous 3 patches.

[Eric Seidel (no email)]

I didn't include a changelog in the patch, but here's the gist:

This patch fixes the interesting test cases in Acid3 test23.  This makes us agree with the W3C as well as Mozilla wrt throwing exceptions from create*NS.  This cannot be a compat issue with IE since IE does not support either of these methods.  Mozilla is also moving to match our/the spec's behavior.

Most interesting was ":div" which we didn't throw about due to not checking prefix.isEmpty(), there were also a few obscure cases from the spec, where DOM Core Level 2 requires that you disallow "xmlns" and "xml" as prefixes when not matched with their correct namespace.

[Darin Adler]

Comment on attachment 18416 [details]
full patch, including test cases

Looks like great work!

 3 function quoteString(s) {

Please format JavaScript code so that function braces are on a new line, as in our C code.

 36     shouldThrowWithCode("document.createAttributeNS(" + quoteString(namespace) + ", " + quoteString(name) + ")", code);

Can you use the standard shouldThrow instead of writing your own?

 39 // We use numbers instead of names, because it's slightly easier to read

I think the names are clearer. I found the numbers confusing when looking at the test output. If you find the long qualified names and full exception strings too wordy maybe you could still write your own harness, but make it so only the short part of the names show up: "NAMESPACE_ERR". That's definitely clearer than "5".

 49 shouldBeEqualToString("document.createElementNS(null, undefined).toString()", "[object Element]");

I normally just use shouldBe here and put two sets of quotes around the expected string. Is this style better?

 703     // FIXME: We define these more than once in WebCore, we could use an XMLNSNames.cpp someday

While this is true, I personally don't think it's helpful to have a FIXME for this.

Also, I don't think we want to add a file named XMLNSNames.cpp. I'd suggest we add these XML namespace constants to XMLNames.h.

 704     static const AtomicString xmlnsURI = "http://www.w3.org/2000/xmlns/";

I think it's confusing to have this named xmlnsURL and another one named xmlNamespaceURI meaning something different. The new one should probably be xmlnsNamespaceURI.

 701 static bool checkForNamespaceError(const QualifiedName& qName)

A function that returns a bool and is named "check" confused me. I couldn't figure out whether true meant "had an error" or "OK". It would be better to name this something like hasNamespaceError.

 710     if (qName.prefix() == emptyAtom) // createElementNS(null, ":div")
 714     if (!qName.prefix().isEmpty() && qName.namespaceURI().isNull()) // createElementNS(null, "html:div")

One of these checks uses isEmpty() and the other uses emptyAtom. Why the difference?

 744     // FIXME: the element factories should be fixed to not ignore qName.prefix()

This comment is not clear enough. The comment should state what the factories should do with the prefix rather than just saying "not ignore".

 746     // Then function can stop taking ExceptionCode& as well.

What function? You mean setPrefix? Please name the function and also say why.

 757 PassRefPtr<Element> Document::createElementNS(const String& _namespaceURI, const String& qualifiedName, ExceptionCode& ec)

Given the other changes you made, I'm surprised you didn't take the spurious underscore off of _namespaceURI here.

 761         // qualifiedName.isEmpty() also hits this path (not NAMESPACE_ERR), see:
 762         // dom/html/level1/core/hc_documentinvalidcharacterexceptioncreateattribute1.html
 763         // and http://www.w3.org/Bugs/Public/show_bug.cgi?id=525

I don't think this is a helpful comment. It's a great code change! But we already have a test case, and I don't really think we need a pointer to that bug. If we do want that bug URL somewhere, I suggest putting it in the test case rather than putting it in two places in the code.

 3447     // Spec: DOM Level 2 Core:  createAttributeNS(null, "xmlns") should throw
 3448     // http://www.w3.org/TR/DOM-Level-2-Core/core.html#ID-DocCrAttrN
 3449     if (qName.localName() == "xmlns" && qName.namespaceURI() != "http://www.w3.org/2000/xmlns/") {

You left an "S" off the URL here, so the URL didn't work. Please fix that.

I also don't think the example in the comment is clear enough since it's a specific case involving null and doesn't match the much broader test in the code, although the code does match the spec. It'd probably be better to just omit the example.

The code you added is only for xmlns, but the section you site in the DOM Level 2 document also mentions a couple other rules that don't seem to be implemented here. The one for xml and the one about a prefix without a namespace. The test cases seem to cover these, but I don't see why it works. Shouldn't you use the whole hasNamespaceError function here?

Even though everything looks good, there are enough comments here that I'll say review-.

[Eric Seidel (no email)]

The FireFox dudes have created another test case:
http://web.mit.edu/jwalden/www/ce.html

[Jeff Walden (remove +bwo to email)]

Created attachment 18427 [details]
Gecko tests, some with explanations

[Jeff Walden (remove +bwo to email)]

Created attachment 18428 [details]
Gecko tests, with bugs fixed

Sorry, I had a misunderstanding about colons in XML names -- this is better, especially since passing the previous would have meant failing acid3.  :-)

[Eric Seidel (no email)]

I still have this patch in my tree.  I'll see if I can get this polished off an submitted this week.

[Eric Seidel (no email)]

Created attachment 18866 [details]
Address Darin's comments and update tests to match mozilla

 .../createAttributeNS-namespace-err-expected.txt   |   70 +++++---
 .../createElementNS-namespace-err-expected.txt     |   68 +++++---
 .../resources/createAttributeNS-namespace-err.js   |  190 ++++++++++++--------
 .../resources/createElementNS-namespace-err.js     |  188 ++++++++++++--------
 WebCore/dom/Document.cpp                           |   32 ++--
 5 files changed, 331 insertions(+), 217 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 18866 [details]
Address Darin's comments and update tests to match mozilla

This includes darin's suggested fixes, as well as updated tests.  We don't pass all of the updated moz tests (and I haven't spent any serious effort to find out why).  I'm not sure passing the moz tests is worth much effort however, given how obscure a section of the spec this is.

[Eric Seidel (no email)]

"blocking" bug 16693, since some of the functions added by this patch should be reused there.  I'd like to land this (even though we "fail" some of moz's new tests -- we throw different exception numbers in a couple cases).  I can file a follow-up bug for those cases.  It would be hard to imagine any number of websites depending on such an obscure section of the DOM as "which exception is thrown for this particular flavor of invalid qualified name" -- hence my desire to get this landed and move on.

[Eric Seidel (no email)]

Created attachment 18882 [details]
Squashed patch for final review

 .../documentsetstricterrorchecking02-expected.txt  |    3 +-
 .../createAttributeNS-namespace-err-expected.txt   |   52 ++++++++
 .../Document/createAttributeNS-namespace-err.html  |   13 ++
 .../createElementNS-namespace-err-expected.txt     |   51 ++++++++
 .../Document/createElementNS-namespace-err.html    |   13 ++
 .../fast/dom/Document/resources/TEMPLATE.html      |   13 ++
 .../resources/createAttributeNS-namespace-err.js   |  135 ++++++++++++++++++++
 .../resources/createElementNS-namespace-err.js     |  134 +++++++++++++++++++
 WebCore/dom/Document.cpp                           |   77 ++++++++---
 WebCore/dom/Document.idl                           |    4 +-
 10 files changed, 470 insertions(+), 25 deletions(-)

[Darin Adler]

Comment on attachment 18882 [details]
Squashed patch for final review

+description("createAttirbuteNS tests adapted from createElementNS tests attached to webkit bug 16833");

Typo. Should be "createAttributeNS".

r=me

[Eric Seidel (no email)]

Fixed and landed in r29952.

[Kevin McCullough]

Unfortunately this fix caused a regression in Yahoo! mail.  Passing Acid 3 is important but we need to not break a real-world high-priority site, so we are going to roll this change out.

1. Login to the Yahoo! mail beta.
2. Click the New button on the top left to make a new message.
3. Fill in the To field and Subject field.
4. From the Subject field hit Tab.  In previous revisions the focus and cursor would go to the Body.
5. Fill in the body and hit the Send button.

It will take a long time to send and then fail saying "Oops! We are having a problem sending your message."

[Jeff Walden (remove +bwo to email)]

At a glance the problem is one of these three, assuming I didn't mess up line numbers or anything:

this.A.setAttribute("xmlns:xsi",yC.O+"-instance");
this.A.setAttribute("xmlns:xsd",yC.O);
this.A.setAttribute(yC.L+"encodingStyle",yC.q+"/encoding/");
this.n.createElement(yC.L+"Body");

yC looks a few lines up to be defined (with reformatting, natch) as:

  {
    q: "http://schemas.xmlsoap.org/soap",
    O: "http://www.w3.org/1999/XMLSchema",
    L: "SOAP-ENV:",
    B: "srcWindow",
    s: "urn:yahoo:ymws",
    n: function(){},
    u: function(){}
  }

So I'm guessing it's most likely the last one, although looking at the patch I don't quite know why.

[Eric Seidel (no email)]

OK, this is a real regression.

The problem is this check:

    if (!qName.prefix().isEmpty() && qName.namespaceURI().isNull()) // createElementNS(null, "html:div")
        return true;

fails for document.createElement("foo:bar"), because we send createElement down the createElementNS path (which is wrong).

So this patch is wrong (due to existing code being "wrong").

[Ian 'Hixie' Hickson]

Good lord what is Yahoo doing here. That's insane code.

[Darin Adler]

Comment on attachment 18882 [details]
Squashed patch for final review

Clearing review flag since this bug was reopened.

[Eric Seidel (no email)]

I do not wish to hold the mutex on this bug, I'm not working on it actively at the moment.

What remains to be done:
1.  create HTML and XHTML test cases for createElement (and test against other browsers to see what exceptions we should throw)
2.  make createElement stop calling createElementNS under the covers.

[Darin Adler]

Comment on attachment 18882 [details]
Squashed patch for final review

Marking this review- to make it clear that we need a new patch that doesn't break Yahoo. I didn't want to mark this obsolete since it's the latest and greatest we have so far.

[Jeff Walden (remove +bwo to email)]

I added a bunch of tests for createElement to complement the createElementNS ones:

http://mxr.mozilla.org/mozilla/source/dom/tests/mochitest/bugs/test_bug411103.html?raw=1

[Darin Adler]

I merged in Jeff's additional test, but since none of those tests fails with the patch applied, clearly none exactly covers the problem that affected Yahoo.

[Darin Adler]

Created attachment 19785 [details]
patch rebased with addition of more tests from Gecko tree

[Eric Seidel (no email)]

Created attachment 19938 [details]
Fix xml/xhtml handling of createElement and add tests

 .../documentsetstricterrorchecking02-expected.txt  |    3 +-
 .../createAttributeNS-namespace-err-expected.txt   |   52 ++++++
 .../Document/createAttributeNS-namespace-err.html  |   13 ++
 .../createElementNS-namespace-err-expected.txt     |  138 +++++++++++++++
 .../Document/createElementNS-namespace-err.html    |   13 ++
 .../fast/dom/Document/resources/TEMPLATE.html      |   13 ++
 .../resources/createAttributeNS-namespace-err.js   |  135 +++++++++++++++
 .../resources/createElementNS-namespace-err.js     |  180 ++++++++++++++++++++
 WebCore/dom/Document.cpp                           |   89 +++++++---
 WebCore/dom/Document.idl                           |    4 +-
 10 files changed, 609 insertions(+), 31 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 19938 [details]
Fix xml/xhtml handling of createElement and add tests

Oops, turns out there was an error in the test harness.

[Eric Seidel (no email)]

Created attachment 19940 [details]
additional patch needed to fix the test and code

[Eric Seidel (no email)]

Created attachment 19941 [details]
Fix xml/xhtml handling of createElement and add tests

xml/xhtml createElement fixed, now with test harness fixed too
---
 .../documentsetstricterrorchecking02-expected.txt  |    3 +-
 .../createAttributeNS-namespace-err-expected.txt   |   52 ++++++
 .../Document/createAttributeNS-namespace-err.html  |   13 ++
 .../createElementNS-namespace-err-expected.txt     |  138 +++++++++++++++
 .../Document/createElementNS-namespace-err.html    |   13 ++
 .../fast/dom/Document/resources/TEMPLATE.html      |   13 ++
 .../resources/createAttributeNS-namespace-err.js   |  135 +++++++++++++++
 .../resources/createElementNS-namespace-err.js     |  180 ++++++++++++++++++++
 WebCore/dom/Document.cpp                           |   89 +++++++---
 WebCore/dom/Document.idl                           |    6 +-
 10 files changed, 610 insertions(+), 32 deletions(-)

[Maciej Stachowiak]

Comment on attachment 19941 [details]
Fix xml/xhtml handling of createElement and add tests

r=me

[Eric Seidel (no email)]

r31230

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.