336921 - e4x: extra, undesired <br/> tags created

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Joey Minta]

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

[Biju]

Attachment: 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 ....

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

Does this break anything else?

Comment 4

[Brendan Eich [:brendan]]

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

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2.1

Sorry, the comment in this patch is slightly better.

Comment 7

[Brendan Eich [: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

Comment 8

[Brendan Eich [:brendan]]

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

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 10

[Joey Minta]

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

[Bob Clary [:bc] (inactive)]

Checking in regress-336921.js;
/cvsroot/mozilla/js/tests/e4x/XML/regress-336921.js,v  <--  regress-336921.js
initial revision: 1.1

Comment 12

[Mike Connor [:mconnor]]

not going to block on this, but will take the patch for 1.8.1

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.

Comment 15

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.1, 1.9 windows/mac(ppc|tel)/linux 20060728

Comment 16

[Daniel Veditz [:dveditz]]

ditto the 1.8.1 triage team (comment 12)

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 230742 [details] [diff] [review]
patch v2.1

approved for 1.8.0 branch, a=dveditz for drivers

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8.0 branch.

Comment 19

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.7 20060818 windows/mac*/linux

Comment 20

[Bob Clary [:bc] (inactive)]

note to self: e4x/XML/regress-336921.js, result: FAILED TIMED OUT, firefox_1.8.1b2_2006082806_dbg, windows/prune, browser