Closed Bug 336921 Opened 18 years ago Closed 18 years ago

e4x: extra, undesired <br/> tags created


(Core :: JavaScript Engine, defect, P3)






(Reporter: jminta, Assigned: mrbkap)



(Keywords: verified1.8.0.7, verified1.8.1)


(2 files, 2 obsolete files)

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

1. navigate attachment "e4x_appendChild.html"
2. click Input Case "0" link
3. Click "test" button
4. repeat for links 1, 2, 3, 4 ....
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.
Attached patch patch v1 (obsolete) — Splinter Review
Does this break anything else?
Assignee: general → mrbkap
Attachment #230691 - Flags: review?(brendan)
Blocks: e4x
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-
Attached patch patch v2 (obsolete) — Splinter Review
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)
Attached patch patch v2.1Splinter Review
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.
Closed: 18 years ago
OS: Linux → All
Priority: -- → P3
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.8.1beta2
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.
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
verified fixed 1.8.1, 1.9 windows/mac(ppc|tel)/linux 20060728
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
verified fixed 20060818 windows/mac*/linux
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.