83301 - provide better xml parser error reporting for TAG_MISMATCH

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, enhancement)

Description

[timeless]

mozilla [5/29-04] output:
XML Error in file 'chrome://messenger/content/timeless.xul', Line Number: 74, 
Col Number: 3, Description: mismatched tag
Source Line: </comandset>

tidy (vers 30th April 2000) output:
line 74 column 1 - Warning: unexpected </comandset> in <commandset>

Using tidy I was able to figure out that I made a typo.  Using mozilla I wasn't.

I know it isn't important to provide good error/warning messages, but it would 
be helpful if we could show what we expected (and ideally what line number made 
us expect it)

Comment 1

[harishd]

Not a high priority setting to FUTURE. CCing heikki.

Comment 2

[timeless]

Attachment: patch

Comment 3

[timeless]

Attachment: modified and commented

Comment 4

[Boris Zbarsky [:bzbarsky]]

Just talked to timeless about this one.  With a change from "foo" to 
"currentToken" in the code and a capitalization of "yes" in "yes we are setting 
and checking ..." in the comments, this has r=bzbarsky.

timeless says he'll attach another patch if anyone has any other concerns.

Over to timeless

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

1. I think you should remove the XXX switch... comment.
2. I don't understand the NOT WELL-FORMED comment.
3. The comment block should be indented as well. Also use the style as in the
   comment above the function. Do not use the weird indentaton in the comment.
   Use empty line to distinguish between paragraphs.
4. The for statement looks bizarre. Could you move the initial initializations
   outside the for, and then increment them more normally?
5. Use curly braces liberally, i.e. place them around the for even if the switch
   statement is the only statement in there.
6. Please update the comment above the function, about the content model.
   It is also missing the filename, not sure if we need the expected info there.
7. Are you absolutely certain that balance can be PRUint32? Is there any way
   it could go into negative value (in which case this code here would stink)?
   I think it would be safer to just use PRInt32 and make the check take that
   into account.

In short, I want the code to be more readable and in the same style with the
rest of the file.

Comment 6

[timeless]

1. sure.
2. in theory expat will give a not well formed error if it sees 
data:text/xml,</foo>.
If we did reach the for loop we would only exit because the deque would set 
expected to null, instead of the normal exit when balance reaches zero.
I can certainly remove the comment.
3. i'll try
4. i could move the initialization out, but moving the increment stuff around 
led me to a while loop that wasn't correct or one which wasn't as null proof.
5. ok.
6. Please update the comment above the function, about the content model.
huh?
   It is also missing the filename, not sure if we need the expected info 
there.
'filename' huh?
<q src="console"><[CDATA[
XML Error in file 'data:text/xml,<foo></bar>', Line Number: 1, Col Number: 8, 
Description: mismatched tag. Expected: </foo>
Source Line: <foo></bar>
]]></q>
<q src="browser"><[CDATA[
XML Parsing Error: mismatched tag. Expected: </foo>
Location: data:text/xml,<foo></bar>
Line Number 1, Column 8:
<foo></bar> 
-------^
]]></q>
actually, the debug output is almost more useful than the browser output, but 
there's not much i can do about that -- and it's certainly beyond the scope of 
this bug.
7. the reason it could be negative is if we have way too many close tags 
(>MAXINT), hence i double the number of close tags i tolerate. in order for 
balance to reach <0 the correct way, it first has to reach =0 which will 
terminate the for loop and is the correct way to do so.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

2. In theory? Could you test this and either remove the comment if it does not
make sense or modify it a little so it is more clear?

4. How about:

nsDequeIterator current = mState->tokenDeque->End();
CToken *expected = NS_STATIC_CAST(CToken*,--current);
PRUint32 balance = 1;

for (; balance && expected; expected=NS_STATIC_CAST(CToken*,--current) )

   Is --current always safe, or can it cause crash?

6. This is almost outside the scope of this bug, but I'd like you to fix the
   comment anyway. The comment has not evolved with the code so it would be
   nice to catch up.

   Is the filename sometimes not a filename? It should be a filename always.
   Cc: rbs who added that feature.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Or actually, the for loop is starting to look more like a while loop now... so
something like:

nsDequeIterator current = mState->tokenDeque->End();
CToken *expected = NS_STATIC_CAST(CToken*,--current);
PRUint32 balance = 1;

while ( balance && expected ) {
  switch (expected->GetTokenType()){
    case eToken_start:
      --balance;
      break;
    case eToken_end:
      ++balance;
      break;
    default:
      break; /*we don't care about newlines or other tokens*/
  }

  expected=NS_STATIC_CAST(CToken*,--current);
}

Comment 9

[rbs]

>  Is the filename sometimes not a filename? It should be a filename always.
Yep, it is always a filename. To be more specific: by construction, it is always 
the absolute URL of the XML file (or the DTD file) being parsed.

Comment 10

[timeless]

ok, it sounds like 6 is worth doing, but i'm open to suggestions.

heiki, the problem w/ the modified for/while loop is:

  expected=NS_STATIC_CAST(CToken*,--current);
happens _after_ balance is set. so we end up chomping the tag I actually care 
about.

notice the logic of my <evil>for loop</evil>

initialization:
nsDequeIterator foo=mState->tokenDeque->End();
condition:
balance
&& (expected=NS_STATIC_CAST(CToken*,--foo));
first run balance is 1 [true] and gets the first token [hopefully true]
other runs, stop if balance reaches 0. This means don't walk to the next token.
body:
with the expected token modify balance

<foo></bar>
w/ your code, we get foo, set balance to 0 and then march to the next token 
NULL. result: no errror reporting.

bz and I talked through this problem when I first attached the patch, and we 
concluded that there isn't a good way to write either a normal for loop or a 
good while loop.

rbs/heikki: you call a url a filename? wierd.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Yeah, maybe filename is a bit misleading name. URI/URL might be more appropriate.

Sorry about missing the obvious. Let me see if I am now thinking more clearly...
Also since I am being paranoid I am adding some more sanity checks... This code
has moved the if(expected) into the loop as well, and the balance check into the
loop.

nsDequeIterator current = mState->tokenDeque->End();
CToken *expected = NS_STATIC_CAST(CToken*,--current);
PRUint32 balance = 1;

while ( expected ) {
  tokenType = expected->GetTokenType();

  switch ( tokenType ) {
    case eToken_start:
     --balance;
     break;
    case eToken_end:
      ++balance;
      break;
    default:
      break; /*we don't care about newlines or other tokens*/
  }

  if (!balance) {
    NS_WARN_IF_FALSE(tokenType == eToken_start, "unexpected token type");
    if (tokenType == eToken_start) {
      CStartToken *startToken=NS_STATIC_CAST(CStartToken*,expected);
      error->description.Append(NS_LITERAL_STRING(". Expected: </"));
      error->description.Append(startToken->GetStringValue());
      error->description.Append(NS_LITERAL_STRING(">"));
    }
    break;
  }

  expected = NS_STATIC_CAST(CToken*,--current);
}

Comment 12

[rbs]

> rbs/heikki: you call a url a filename? wierd.
A legacy that nobody has care to Search'nReplace :-) It is called 'sourceURL' in
the error struct now.

Comment 13

[timeless]

it took me way to long to figure out what this meant, which means it's too late 
in the day for me to try to read the code...
    NS_WARN_IF_FALSE(tokenType == eToken_start, "unexpected token type");
imo the warning isn't even accurate.  The problem isn't that we got the wrong 
token type, it's that we somehow managed to reach balance==0 while parsing a 
close token (the code clearly only changes balance for open and close. An 
open's expected behavior is a possible =0, and it is very unexpected for a 
close to cause balance==0).

the loop is now relatively large instead of being tight, but if that's the 
price of clarity...  and it now uses various constructs including while { if 
{break}} which are thing i like to avoid, but if they're part of parser style 
that's that.

the code given shouldn't compile because tokenType isn't declared... we can 
make it local to the loop.

otherwise the code is fine w/ me and accomplishes the task for which this bug 
was filed.

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

You are right about the assertion/if check in the current code. It can be
removed in this case. You could add a comment so that if someone later changes
the loop they will remember to honor our assumption that !balance means it has
got to be a start tag. After this change there is no need for the tokenType
variable, obviously.

Tight code usually means code that is hard to read. Break inside
while/for/do-while loops is ok. Breaking inside of one is usually the best way
to exit "infinite" loop (like while(1){/*need break*/}). If you don't need
break, great, but do not try to run through hoops to avoid one. Make it obvious
what is happening.

Comment 15

[Chase Tingley]

Attachment: patch integrating comments from heikki, timeless

Comment 16

[Chase Tingley]

I think this version reflects all the comments... The for loop is now straight
out of heikki's comment, minus the warning and tokenType.

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

1. Typo here:

+       * want), so balance is initially sent to one.
                                           ^

2. Since tokenType is only needed once, this code:

+        tokenType = expected->GetTokenType();
+
+        switch ( tokenType ) {

   should be changed to:

+        switch ( expected->GetTokenType() ) {

Once those are fixed r=heikki.

Comment 18

[Chase Tingley]

Attachment: fix typo and remove tokenType

Comment 19

[Mike Shaver (:shaver emeritus)]

+      while ( expected ) {

  vs.

+        if (!balance) {

It looks like the rest of that file eschews spaces inside parens, so you should too.

Tidy that up, and sr=shaver.  Thanks for the fix: better error reporting helps
everyone.

Should this bug be closed now, or are there still more improvements needed?

Comment 20

[timeless]

thanks everyone, fix checked in. other issues can be filed in new bugs.

Comment 21

[Moied]

As per the comments from reporter(timeless) this bug is fixed. Marking verified.