220027 - [FIX]Excessive assertions triggered by nsXMLElement::GetID()

Status: RESOLVED FIXED

(Core :: MathML, defect, P1)

Description

[rbs]

nsresult
nsGenericContainerElement::GetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
                                   nsIAtom** aPrefix,
                                   nsAString& aResult) const
{
  NS_ASSERTION(nsnull != aName, "must have attribute name");
  NS_ASSERTION(aNameSpaceID != kNameSpaceID_Unknown,
               "must have a real namespace ID!");
  [...]
}  


This later assertion fires a lot at:
http://www.mozilla.org/projects/mathml/demo/basics.xhtml

I basically have to abort the browser to get out.

Bonsai says that it was added by bz:
"3.272 <[e-mail]> 23 Jun 2003 23:45
Add asserts on people attempting to fetch attributes in the
kNamespaceID_Unknown namespace or call HasAttr on such. Bug 210314"

The document freely mixes MathML+XHTML with its default attributes in the usual
way. Is this now obsolete?

Comment 1

[Boris Zbarsky [:bzbarsky]]

This GetID code is just completely bogus.  It should be using kNameSpaceID_None
for the namespace it passes to GetAttr.  And the assert in GetAttr is not only
correct, it caught a bug in this case.  With the code as-is, the following MathML

<maction xlink:id="bar" id="baz">

would return "bar" from GetID, if I read the code right.

We should change nsXMLElement::GetID to do the right thing, and take out the
remnants of kNameSpace_Unknown stuff in nsGenericContainerElement::GetAttr.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Silly testcase to show correctness bug

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132027 [details] [diff] [review]
Patch

jst?  sicking?	This is pretty straightforward...

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 132027 [details] [diff] [review]
Patch

sr=jst

Comment 6

[Boris Zbarsky [:bzbarsky]]

Fixed.  I don't think we need this on the 1.5 branch, do we?