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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

As pointed out by sayrer recently...
Created attachment 237813 [details] [diff] [review]
Patch
Attachment #237813 - Flags: review?(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
Attachment #237813 - Flags: review?(brendan) → review+
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.
Attachment #237813 - Flags: approval1.8.1?

Comment 4

11 years ago
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.
Not SGML either (right, mrbkap?).  Just a bug.

/be
Created attachment 237850 [details] [diff] [review]
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.  :-(
Attachment #237850 - Flags: review?(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
Attachment #237850 - Flags: review?(brendan) → review+
(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.
Status: NEW → ASSIGNED
Created attachment 237874 [details] [diff] [review]
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.
Attachment #237874 - Flags: review+
Attachment #237874 - Flags: approval1.8.1?
(Assignee)

Updated

11 years ago
Attachment #237813 - Flags: approval1.8.1?
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

11 years ago
(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

11 years ago
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.
Attachment #237874 - Flags: approval1.8.1? → approval1.8.1-
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 14

11 years ago
Checking in regress-352223.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-352223.js,v  <--  regress-352223.js
initial revision: 1.1
done
Flags: in-testsuite+

Comment 15

11 years ago
verified fixed 1.9 20060914 windows/mac*/linux
bug _is_ evident on 1.8.
Status: RESOLVED → VERIFIED

Updated

10 years ago
Summary: <foo></ foo> accepted as valid XML → <foo></ foo> accepted as valid XML (e4x/Regress/regress-352223.js)
You need to log in before you can comment on or make changes to this bug.