411103 - document.createElement(bad) (and the NS version) throwing wrong exception

Status: RESOLVED FIXED

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

Description

[Jeff Walden [:Waldo]]

document.createElement throws a NAMESPACE_ERR when it should throw an INVALID_CHARACTER_ERR.  We should also check out createElementNS at the same time to see if it has any similar problems, depending on exactly what the specified behavior is.

Relevant code is at the following locations:

http://mxr.mozilla.org/mozilla/source/parser/htmlparser/src/nsParserService.cpp#199
http://mxr.mozilla.org/mozilla/source/parser/expat/lib/moz_extensions.c#61

Trivial bugfix for someone willing to spend the time on it (just requires careful reasoning to make sure the fix is correct).

Comment 1

[Jeff Walden [:Waldo]]

There are also problems with createElementNS along the same lines, related to when the namespace error is thrown vs. when the invalid-character error is thrown.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Potential patch

This simplifies the name-checking code a bit, regresses none of the dom-level#-core Mochitests (rest haven't been run but probably don't exercise these edge cases), and makes us pass test 22 of acid3.  (Test 23 is currently buggy, but if you change the assertEquals to compare against |code| and ignore the last document.createElementNS with incorrectly-reversed arguments, we would pass it too with this patch.)

I'll wait to make the next move here until test 23 in acid3 is fixed.  DOM Core wording for exactly when INVALID_CHARACTER_ERR and NAMESPACE_ERR are raised is murky at best, and I want to be sure we're doing what we're supposed to do for all of these first.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: WebKit's tests and how current trunk does on them

This is how we currently do on WebKit's tests for these things; ideally we want to have the same results as they do on all these things (although I doubt it'll happen for, say, the argument-count one, for example).  I'll add whichever of these tests aren't in acid3 but which we want to pass as Mochitests in the final patch.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Final patch

Okay, this is good to go.  Hixie fixed acid3, and I added a bunch of tests for various interesting situations, and I think I now have a test for every branch in MOZ_XMLCheckQName.  We pass both acid3's two tests and all the ones in this patch with the fix.

This is in some ways a scary bit of code to edit because it's probably been un, but I hope the quantity and variety of tests will make it easier to review.  If you see a test I haven't hit, feel free to tell me and I can add it to the patch.

Comment 5

[Jeff Walden [:Waldo]]

> because it's probably been un

...touched for awhile.  Also, it passes the DOM Mochitests, too, since I didn't say so.

Comment 6

[Jeff Walden [:Waldo]]

I wouldn't bother requesting blocking on this usually, but seeing as we're going to be living with a broken acid3 in Firefox 3 for a fair while (unless a fair amount of tricky CSS and dynamic-style change hacking happens in contradiction of current trunk feature-landing policy, not to mention everything else in it), I think it only makes sense to make the easy changes that improve our score as much as we can in the time we have.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 296885 [details] [diff] [review]
Final patch

>Index: content/base/src/nsDocument.cpp

>   if (aPrefix) {
>-    aPrefix->ToString(qName);
>-    qName.Append(':');
>+    nsAutoString prefix;
>+    aPrefix->ToString(prefix);
>+    NS_ASSERTION(NS_SUCCEEDED(nsContentUtils::CheckQName(prefix, PR_TRUE)),
>+                 "Don't pass invalid prefixes to nsDocument::CreateElem, "
>+                 "check caller.");
>   }

Pretend the second argument to CheckQName is PR_FALSE, actually -- typo on my part.

Comment 8

[Mike Schroepfer]

We won't block but will take reviewed patch. 

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 296885 [details] [diff] [review]
Final patch

Here's the comments I already had:

>Index: content/base/src/nsDocument.cpp
>===================================================================

>+    NS_ASSERTION(NS_SUCCEEDED(nsContentUtils::CheckQName(prefix, PR_TRUE)),
>+                 "Don't pass invalid prefixes to nsDocument::CreateElem, "
>+                 "check caller.");

You're checking the prefix with a function that checks for a QName, that seems wrong.

>+  // We cannot check localName here with a PR_TRUE second argument because one
>+  // of our callers, nsDocument::CreateElement, may be called with an argument
>+  // "a:b:c", in which case aName isn't a valid XML QName.

You'll miss out on errors for those callers that do need PR_TRUE here.

>Index: parser/expat/lib/moz_extensions.c
>===================================================================

>-      if (ns_aware) {
>-        if (*colon != 0 || nmstrt || ptr + 2 == end) {
>-          /* We already encountered a colon or this is the first or the last
>-             character so the QName is malformed. */
>-          result |= MOZ_EXPAT_MALFORMED;
>-        }
>-        *colon = ptr;
>-        nmstrt = 1;
>-      }
>-      else if (nmstrt) {
>-        /* This is the first character so the QName is malformed. */
>-        result |= MOZ_EXPAT_MALFORMED;
>-        nmstrt = 0;
>+      if (nmstrt || ptr + 2 == end || (*colon && ns_aware)) {
>+        /* This is the first character, the last character, or a second colon
>+           when we're namespace-aware, so the QName is malformed. */

Why return malformed for a colon as the last character in non-namespace-aware mode? I think returning it for a colon as the first character is already up for debate, we should eithernot return it at all or return it for any colon (which we probably don't do because it could break some sites).

>+        result = MOZ_EXPAT_MALFORMED;
>+        goto end;

Don't use goto in this file.

>       }
>+      *colon = ptr;
>+      nmstrt = 1;

Why start checking for name start characters in non-namespace-aware mode? I think we shouldn't return a colon position in non-namespace-aware mode.

>       break;
>+

Gratuitous whitespace change (here and elsewhere).

>     case BT_NONASCII:
>-      if (nmstrt) {
>-        if (!IS_NMSTRT_CHAR_MINBPC(ptr)) {
>-          /* If this is a valid name character the QName is malformed, 
>-             otherwise it contains an invalid character. */
>-          result |= IS_NAME_CHAR_MINBPC(ptr) ? MOZ_EXPAT_MALFORMED :
>-                                               MOZ_EXPAT_INVALID_CHARACTER;
>-        }
>+      if (nmstrt && !IS_NMSTRT_CHAR_MINBPC(ptr)) {
>+        /* If this is a valid name character the QName is malformed if we've
>+           seen a colon (in which case it's a valid XML name, but not a valid
>+           QName), otherwise it contains an invalid character. */
>+        result = IS_NAME_CHAR_MINBPC(ptr)
>+               ? MOZ_EXPAT_MALFORMED
>+               : MOZ_EXPAT_INVALID_CHARACTER;

You changed the comment, I don't see why since the code didn't change.

Since you changed how this returns errors you should change nsParserService::CheckQName so it checks for errors with == instead of with &.

I have to say that it makes me nervous to make changes like this at this point in the development cycle. I don't see any tests that show how this affects createElement, that has the most undefined behaviour and could affect sites more than createElementNS.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Updated to comment, with createElement tests

(In reply to comment #9)
> You're checking the prefix with a function that checks for a QName, that seems
> wrong.

> You'll miss out on errors for those callers that do need PR_TRUE here.

Hopefully this modification will be better; it's more targeted, and it addresses the current assertion we fire when you execute |document.createElement("a:b:c")|.


> >Index: parser/expat/lib/moz_extensions.c

> Why return malformed for a colon as the last character in
> non-namespace-aware mode? I think returning it for a colon as the first
> character is already up for debate, we should eithernot return it at all
> or return it for any colon (which we probably don't do because it could
> break some sites).

DOM 3 Core specs say createElement accepts any valid XML name, and a:b:c is a valid XML name (but not QName, so createElementNS would throw), so we shouldn't throw in that case for that reason.  The end-in-colon thing is a mini-bug, fixed in the patch.


> >       }
> >+      *colon = ptr;
> >+      nmstrt = 1;
> 
> Why start checking for name start characters in non-namespace-aware mode? I
> think we shouldn't return a colon position in non-namespace-aware mode.

Colon position's meaningless if !ns_aware, but it's simpler code to assign to it.  As for nmstrt, I changed this to |nmstrt = ns_aware|, because otherwise createElement("a::b") would throw contrary to the DOM spec.


> You changed the comment, I don't see why since the code didn't change.

I found the comment hard to understand, so I rewrote it a little.  Given that the code actually changed in this patch, the (different) comment change now seems reasonable.


> I have to say that it makes me nervous to make changes like this at this point
> in the development cycle. I don't see any tests that show how this affects
> createElement, that has the most undefined behaviour and could affect sites
> more than createElementNS.

Yeah, I should have had tests for createElement in the original patch -- thanks for calling me on that.  I have to say, tho, I can't imagine any site ever relying on the behavior of createElement when given the junk that's being thrown against it in this patch, especially since browsers differ considerably on this.  (For the record, this patch will bring us closer in line with the behavior of the latest webkit builds, as a bonus.)

Comment 11

[Jeff Walden [:Waldo]]

For what it's worth, <http://bugs.webkit.org/show_bug.cgi?id=16833> has Yahoo Mail bustage from their changes to this behavior.  Looking at their patch and the suspected line that failed, I think their problem is too much codepath-sharing between the NS and non-NS cases, so I don't think we'd be hit by it, but I can't be certain.  (I should provide them the non-NS testcases I should have had in the first patch sometime, since I'm pretty sure they'd fail some of them.)

I tried running us against it with the patch to see what happens, but we're still croaking on the wrong-document error, and I haven't yet had the time to temporarily hack that out to check this patch against Yahoo's mess of code.  That of course would be a prerequisite to landing this, if only to see what happens to report the bug to Yahoo.  I'm sure it would help webkit if we both could tell Yahoo they need to fix it, for what it's worth.

Comment 12

[Jeff Walden [:Waldo]]

The Yahoo regression webkit had is irrelevant because it concerns a case they don't handle but which we plainly handle, as demonstrated by the tests in the patch here:

http://bugs.webkit.org/show_bug.cgi?id=16833#c21

Specifically, they didn't handle createElement("a:b") right because they unify the code path for createElement(NS)? too early, as I suspected in the previous comment.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 301327 [details] [diff] [review]
Updated to comment, with createElement tests

>Index: content/base/src/nsDocument.cpp
>===================================================================

>+  PRBool nsAware = aPrefix != 0 || aNamespaceID != GetDefaultNamespaceID();

s/0/nsnull/

Maybe we should drop the assertion instead, with your change it'll let through invalid local names for the default namespaces.

>Index: parser/expat/lib/moz_extensions.c
>===================================================================

>+        /* If we're at the first character and it's a colon, it's a malformed
>+           QName if we're namespace-aware and an invalid character otherwise. */

I'd drop the first 'If'.

>+        /* If this is a valid name character and we're namespace-aware, the
>+           QName is malformed.  Otherwise, this character's invalid at the
>+           start of a name (or, if we're namespace-aware, in the case of a
>+           localpart). */

s/in the case of a/at the start of a/

>+        return (IS_NAME_CHAR_MINBPC(ptr) && ns_aware)
>+               ? MOZ_EXPAT_MALFORMED
>+               : MOZ_EXPAT_INVALID_CHARACTER;

Move the operators to the end of the previous line.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: With comments addressed

The assertion's only a sanity-check; if it doesn't catch every case that's not a problem as long as it's not firing on valid inputs, I think.

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 302642 [details] [diff] [review]
With comments addressed

Two easy points on acid3, improve some error handling to not expose namespaces to non-namespace-aware code, add lots of tests to guarantee correct behavior now and going forward...

Comment 16

[Mike Schroepfer]

Comment on attachment 302642 [details] [diff] [review]
With comments addressed

Sweet.  Tests make approvals easy.

Comment 17

[Jeff Walden [:Waldo]]

Yay tests; turns out our dependency tracking story for moz_extensions.c was broken on Windows (not sure why not everywhere, to be honest; maybe GCC's better), so I had to add an explicit dependency to make it cause a recompilation of that code.  That change passed the Windows box and one other, so I'm going to call this good.

61/100!