e4x: extra, undesired <br/> tags created

VERIFIED FIXED in mozilla1.8.1beta2

Status

()

Core
JavaScript Engine
P3
normal
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: Joey Minta, Assigned: mrbkap)

Tracking

({verified1.8.0.7, verified1.8.1})

Trunk
mozilla1.8.1beta2
verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 -
blocking1.8.0.7 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
I'm trying to create a structure like:
<div>
 a<br/>
 b<br/>
 c<br/>
</div>

However, if I do...

js> var array = ["a","b","c"];
js> var myDiv = <div/>;
js> for each (a in array) { myDiv.appendChild(a); myDiv.appendChild(<br/>); }
<div>
  a
  <br/>
  <br>b</br>
  <br/>
  <br>c</br>
  <br/>
</div>

Comment 1

12 years ago
Created attachment 221200 [details]
testcase e4x_appendChild.html

step:-

1. navigate attachment "e4x_appendChild.html"
2. click Input Case "0" link
3. Click "test" button
4. repeat for links 1, 2, 3, 4 ....
(Reporter)

Updated

12 years ago
Blocks: 335159
(Assignee)

Comment 2

12 years ago
jminta convinced me that calendar printing couldn't possibly work without this bug being fixed, so I looked into it. It appears to be an E4X spec bug that brendan translated into SpiderMonkey code.

The root of the problem is XML.prototype.appendChild calls [[Get]](x, "*") to get the list of children. [[Get]] calls [[Append]] on the resulting XMLList for each child in the element, which sets the list's [[TargetProperty]] to the current kid's name, which, at the end of the function, is <br>. Then, we call into [[Put]], which creates a kid with the same name of targetProp, and appends to that.

It seems to me that [[Get]] needs to ensure that when it returns, the returned list has a [[TargetProperty]] of whatever Get was asked for.
(Assignee)

Comment 3

12 years ago
Created attachment 230691 [details] [diff] [review]
patch v1

Does this break anything else?
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #230691 - Flags: review?(brendan)

Updated

12 years ago
Blocks: 246441
Comment on attachment 230691 [details] [diff] [review]
patch v1

Wow, this is a big bad erratum in the ECMA-357 spec (not that there haven't been others such before it!). Thanks for finding it!

>+++ jsxml.c	26 Jul 2006 03:59:37 -0000
>@@ -4092,16 +4092,18 @@ retry:
>         }
>     }
> 
>     /* Common tail code for list and non-list cases. */
>     js_LeaveLocalRootScopeWithResult(cx, (jsval) listobj);
>     if (!ok)
>         return JS_FALSE;
> 
>+    JS_ASSERT(list);

You don't need this, and it does no good since list is initialized non-null in all paths.

>+    list->xml_targetprop = nameqn;

This is the fix, but why didn't you remove the two redundant assignments identical this one that happen before the Append loops?  Comment this erratum too, citing the bug #.

/be
Attachment #230691 - Flags: review?(brendan) → review-
(Assignee)

Comment 5

12 years ago
Created attachment 230741 [details] [diff] [review]
patch v2

I was attempting to protect against uses of the list's targetProp before it was set early, but code inspection shows that I was being overly cautious.
Attachment #230691 - Attachment is obsolete: true
Attachment #230741 - Flags: review?(brendan)
(Assignee)

Comment 6

12 years ago
Created attachment 230742 [details] [diff] [review]
patch v2.1

Sorry, the comment in this patch is slightly better.
Attachment #230741 - Attachment is obsolete: true
Attachment #230742 - Flags: review?(brendan)
Attachment #230741 - Flags: review?(brendan)
Comment on attachment 230742 [details] [diff] [review]
patch v2.1

Yeah, the problem is that the spec sets [[TargetProperty]] too early too (in the ToXMLList call from [[Get]]).  Kill that with a set just before returning and the bug is fixed, in spec and in code.  So this looks great, nice comment.

/be
Attachment #230742 - Flags: review?(brendan) → review+

Updated

12 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Comment on attachment 230742 [details] [diff] [review]
patch v2.1

I'm trying for 1.8.0.6 even though this is not a stability or security fix. Tell me if that's wrong and I won't do it again.

This should be fixed in 1.8.1/Firefox 2.

/be
Attachment #230742 - Flags: approval1.8.1?
Attachment #230742 - Flags: approval1.8.0.6?
(Assignee)

Comment 9

12 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
OS: Linux → All
Priority: -- → P3
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8.1beta2
(Reporter)

Comment 10

12 years ago
Note that having this in 1.8.0.x would be very nice for Lightning, as this bug currently bites its printing.  Since Lightning needs to work in TB1.5, simply landing the fix in 1.8.1 isn't sufficient for that case.

Comment 11

12 years ago
Checking in regress-336921.js;
/cvsroot/mozilla/js/tests/e4x/XML/regress-336921.js,v  <--  regress-336921.js
initial revision: 1.1
Flags: in-testsuite+
not going to block on this, but will take the patch for 1.8.1
Flags: blocking1.8.1? → blocking1.8.1-
Comment on attachment 230742 [details] [diff] [review]
patch v2.1

a=dbaron on behalf of drivers for 1.8.1.  Please land on MOZILLA_1_8_BRANCH and mark fixed1.8.1 once you have done so.
Attachment #230742 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 14

12 years ago
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1

Comment 15

12 years ago
verified fixed 1.8.1, 1.9 windows/mac(ppc|tel)/linux 20060728
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
ditto the 1.8.1 triage team (comment 12)
Flags: blocking1.8.0.7? → blocking1.8.0.7-
Comment on attachment 230742 [details] [diff] [review]
patch v2.1

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #230742 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 18

12 years ago
Fixed on the 1.8.0 branch.
Keywords: fixed1.8.0.7

Comment 19

12 years ago
verified fixed 1.8.0.7 20060818 windows/mac*/linux
Keywords: fixed1.8.0.7 → verified1.8.0.7

Comment 20

12 years ago
note to self: e4x/XML/regress-336921.js, result: FAILED TIMED OUT, firefox_1.8.1b2_2006082806_dbg, windows/prune, browser
You need to log in before you can comment on or make changes to this bug.