352223 - <foo></ foo> accepted as valid XML (e4x/Regress/regress-352223.js)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

As pointed out by sayrer recently...

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Brendan Eich [:brendan]]

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

Was I thinking of SGML, or XML 1.0?  ECMA-357 referred to XML 1.0, not 1.1, FWIW.

/be

Comment 3

[Jeff Walden [:Waldo]]

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

In on trunk (and by the way, the no-space thing has been that way since at least XML 1.0 ed. 3).

This is quite possibly the smallest, most benign bugfix ever, so let's see if we can get it on branch.

Comment 4

[Robert Sayre]

Must be SGML (don't know). XML 1.0 has always had that rule, IIRC. XML 1.1 allows control characters in content and XML element names can be written in Mongolian on IBM mainframes. Expat doesn't do 1.1.

Comment 5

[Brendan Eich [:brendan]]

Not SGML either (right, mrbkap?).  Just a bug.

/be

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Reject "'<' S? QName " as well

I could have sworn that when reading this code I'd verified that < foo></foo> is marked as invalid, but I apparently didn't.  :-(

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 237850 [details] [diff] [review]
Reject "'<' S? QName " as well

Where did I get the idea that S? was produced by any grammar for XML?  I swear I saw it somewhere.

Is this it, or do you need to look for more js_MatchToken(cx, ts, TOK_XMLSPACE) calls?

/be

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> (From update of attachment 237850 [details] [diff] [review] [edit])
> Where did I get the idea that S? was produced by any grammar for XML?  I swear
> I saw it somewhere.

Well, '<' QName S? '>', '<' QName (AttrDef S)* S? '/>', '</' QName S? '>' at a minimum -- always after, never before.

> do you need to look for more js_MatchToken(cx, ts, TOK_XMLSPACE) calls?

Think that's it -- I did a search through jsparse.c and didn't see any other incorrect matchtok/xmlspaces.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Patch for both < foo></foo> and <foo></ foo>, for branch

Here's a branch wrapup of both patches for approval; same small-scope obvious fix properties as the previous patches.

Comment 10

[Brendan Eich [:brendan]]

Schrep raised a valid concern: if someone out there for some strange reason came to depend on this misfeature of our E4X implementation, 1.8.1 would not be kosher to break compatibility.  1.9 is fine for that.  So, why worry?  Do we really need this in Firefox 2, or is it more a "nice to have" that could (probably not, but we can make the risk exactly 0) break someone?

/be

Comment 11

[Robert Sayre]

(In reply to comment #10)
> Schrep raised a valid concern: if someone out there for some strange reason
> came to depend on this misfeature of our E4X implementation, 1.8.1 would not be
> kosher to break compatibility.

It's not that unlikely--think JS authored by calls to PHP echo() or other string concatenation methods.

> 1.9 is fine for that.  So, why worry?  Do we
> really need this in Firefox 2, or is it more a "nice to have" that could
> (probably not, but we can make the risk exactly 0) break someone?

Depends on which is more important to us: XML's error handling contract, or our 1.8.1 compat contract. I think 1.8.1 wins in this case.

Comment 12

[Mike Schroepfer]

Comment on attachment 237874 [details] [diff] [review]
Patch for both < foo></foo> and <foo></ foo>, for branch

Yep 1.8.1 compat wins here.  Love this for 1.9.

Comment 13

[Jeff Walden [:Waldo]]

(In reply to comment #12)
> Yep 1.8.1 compat wins here.  Love this for 1.9.

Fair enough.  :-)

Patch is in on trunk, so this is as fixed as it's going to be.

Comment 14

[Bob Clary [:bc] (inactive)]

Checking in regress-352223.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-352223.js,v  <--  regress-352223.js
initial revision: 1.1
done

Comment 15

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060914 windows/mac*/linux
bug _is_ evident on 1.8.