e4x: extra, undesired <br/> tags created

VERIFIED FIXED in mozilla1.8.1beta2



13 years ago
12 years ago


(Reporter: jminta, Assigned: mrbkap)


({verified1.8.0.7, verified1.8.1})

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

Firefox Tracking Flags

(Not tracked)



(2 attachments, 2 obsolete attachments)



13 years ago
I'm trying to create a structure like:

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/>); }

Comment 1

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


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


13 years ago
Blocks: 335159
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.
Created attachment 230691 [details] [diff] [review]
patch v1

Does this break anything else?
Assignee: general → mrbkap
Attachment #230691 - Flags: review?(brendan)
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 #.

Attachment #230691 - Flags: review?(brendan) → review-
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)
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.

Attachment #230742 - Flags: review?(brendan) → review+
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?
Comment on attachment 230742 [details] [diff] [review]
patch v2.1

I'm trying for 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.

Attachment #230742 - Flags: approval1.8.1?
Attachment #230742 - Flags: approval1.8.0.6?
Fix checked into trunk.
Last Resolved: 13 years ago
OS: Linux → All
Priority: -- → P3
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8.1beta2

Comment 10

13 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

13 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+
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1

Comment 15

13 years ago
verified fixed 1.8.1, 1.9 windows/mac(ppc|tel)/linux 20060728
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+
Fixed on the 1.8.0 branch.
Keywords: fixed1.8.0.7

Comment 19

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

Comment 20

13 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.