Closed Bug 340024 Opened 16 years ago Closed 16 years ago

E4X regression: <tag {expression}="constant" attr2="constant"/> now raises error

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: jhs, Assigned: mrbkap)

References

()

Details

(Keywords: regression, verified1.8.0.5, verified1.8.1)

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3

In Firefox 1.5(.0.2 and lower, IIRC), generating E4X XML like <tag {expression}="constant" attr2="constant"/> would allow a dynamically named attribute to be set to some constant value. In Firefox 1.5.0.3, this raises an "invalid XML tag syntax" error. Reordering attr2 to be listed prior to the dynamic expression does not trigger the bug.

For the curious, this bug was encountered in a real-world environment, where the code in question looked like this:

      <input type="checkbox" id={'o'+i} {(m.o?'':'un')+'checked'}="yes"
	     title="Override the original favicon with this icon."
	     style="position:absolute; margin:5px 4px; padding:0;"/>

Reproducible: Always

Steps to Reproduce:
javascript:try{alert( <tag {0?"a":"b"}="c" d="e"/>.toXMLString() )}catch(E){ alert(E) }
Actual Results:  
SyntaxError: invalid XML tag syntax

Expected Results:  
<tag b="c" d="e"/>
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Priority: -- → P3
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Attached patch FixSplinter Review
If we fail to fold the attribute name, we currently aren't able to properly fold the attribute value. Fixing this is not trivial (it'll require making the code generator understand what's going on) and is generally more work than is worth it.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #225638 - Flags: review?(brendan)
Comment on attachment 225638 [details] [diff] [review]
Fix

Add the traditional /* FALL THROUGH */ comment and r=me.  Thanks for fixing this,

/be
Attachment #225638 - Flags: review?(brendan) → review+
Attachment #225638 - Flags: approval-branch-1.8.1?(brendan)
Comment on attachment 225638 [details] [diff] [review]
Fix

Actually, this is a regression from an earlier 1.8.0.x release, do we want this patch (or the patch with the added comment) on that branch?
Attachment #225638 - Flags: approval1.8.0.6?
Attachment #225638 - Flags: approval1.8.0.5?
Comment on attachment 225638 [details] [diff] [review]
Fix

Dan, I'd like to urge you to take this regression fix for 1.8.0.5.  It's safe, and given that, we should try to upgrade people away from the regression ASAP.

/be
Attachment #225638 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
(and the 1.8 branch)
Comment on attachment 225638 [details] [diff] [review]
Fix

Approved for 1.8.0 checkin, a=jay for drivers
Attachment #225638 - Flags: approval1.8.0.6?
Attachment #225638 - Flags: approval1.8.0.5?
Attachment #225638 - Flags: approval1.8.0.5+
Keywords: regression
Fix checked into the 1.8.0 branch.
Keywords: fixed1.8.0.5
Blocks: 334310
Checking in regress-340024.js;
/cvsroot/mozilla/js/tests/e4x/Expressions/regress-340024.js,v  <--  regress-340024.js
initial revision: 1.1
Flags: in-testsuite+
I can verify that this has been fixed in 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.5) Gecko/20060620 Firefox/1.5.0.5
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.