852551 - Unsafe use of reinterpret_cast in nsContentUtils::CheckQName

Status: RESOLVED INVALID

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

Description

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

This causes us to fail a test in <http://w3c-test.org/webapps/DOMCore/tests/submissions/Ms2ger/Document-createElementNS.html>.  Basically, it creates an element name that includes U+0300.  nsContentUtils::CheckQName casts a PRUnichar* to char* to parse the name, which makes this a null byte.  This test fails because the method throws the wrong exception type.

The relevant code was introduced by bug 729048 part 1.  Henri, can that part of the patch just be reverted, or should MOZ_XMLCheckQName be converted to use PRUnichar, or what?

Comment 1

[Henri Sivonen (:hsivonen)]

(In reply to :Aryeh Gregor from comment #0)
> The relevant code was introduced by bug 729048 part 1.  Henri, can that part
> of the patch just be reverted, or should MOZ_XMLCheckQName be converted to
> use PRUnichar, or what?

AFAICT, the patch should not be reverted. MOZ_XMLCheckQName indeed deals with UTF-16 pointed to by byte pointers instead. It's pretty weird indeed. There must be a reason why it's not using PRUnichar, likely related to the weird way how expat can be configured between UTF-8 and UTF-16 at compile time, but maybe it could be converted to use PRUnichar.

Comment 2

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

I looked more closely, and this isn't a valid bug at all.  The error being returned here doesn't match what DOM wants, but it doesn't have to do with incorrect casting.  The macros here look like they handle everything properly.  It's just that MOZ_XMLCheckQName is returning MOZ_EXPAT_MALFORMED (which translates to NamespaceError in the DOM) for some names that don't match the Name production, while the spec says to only throw this if they match Name but don't match QName.  I'm not sure why it's doing this.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So do we need to raise a spec issue, then?

Comment 4

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

The spec seems sane, and Chrome and Opera versions I tested in both pass the test in comment #0, so I'd say we're the ones who should change.  I don't understand what the code is doing or why well enough to say how to fix it, though.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Well, then we still need a bug on whatever the real issue is, right?

Comment 6

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

Yes, along with the many other tests we fail.