313799 - E4X: Assertion failure: !JSVAL_IS_PRIMITIVE(v), at jsxml.c:5558

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Aiko]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051025 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051025 Firefox/1.6a1

Calling a XMLList's child method causes this assertion:
  <><t/></>.child(0)

Reproducible: Always

Steps to Reproduce:




Just in case you want a talkback id: TB11087432Y

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Partial fix

This fixes the assertion botching and does something like the right thing, but by my reading of the spec. this is not compliant, since it seems that |<xml/>.child(0)| should return an empty XMLList, not void (or undefined) as we currently return.

I'm trying to figure out how to return a true empty XML list (which would fix the assertion, as well as the compliance issue).

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better partial patch

Shaver pointed out that using an if-continue was cleaner.

Comment 3

[Brendan Eich [:brendan]]

D'oh!  I must have been typing while asleep.  I'll try for the full fix.

/be

Comment 4

[Bob Clary [:bc] (inactive)]

Checking in regress-313799.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-313799.js,v  <--  regress-313799.js
initial revision: 1.1
done

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 200813 [details] [diff] [review]
Better partial patch

Note that this is the full fix.  There's nothing wrong with xml_child_helper, it is an inlined specialization of the spec (13.4.4.6 1(a-b)).

This is a nice safe crash fix, good for 1.8.

/be

Comment 6

[Brendan Eich [:brendan]]

Invalid (but known value, JSVAL_VOID) pointer defense, no risk.

/be

Comment 7

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 8

[Aiko]

(In reply to comment #4)
> /cvsroot/mozilla/js/tests/e4x/Regress/regress-313799.js,v  <-- 

48           var val = <><t/></>.child(0);
51           TEST(2, 'xml',  typeof x);

|typeof val| or |var x =| ;-)

Comment 9

[Bob Clary [:bc] (inactive)]

(In reply to comment #8)

doh! /me was testing in the shell using x but... Thanks Seno!

Checking in regress-313799.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-313799.js,v  <--  regress-313799.js
new revision: 1.2; previous revision: 1.1
done

Comment 10

[Asa Dotzler [:asa]]

not going to lose a day with this but could become a ride-along if we're forced to respin by some other bug.

Comment 11

[Brendan Eich [:brendan]]

See also bug 313949.

/be

Comment 12

[Brendan Eich [:brendan]]

Looking good on trunk, closing but tracking for rc2 ridealong.  Thanks,

/be

Comment 13

[Aiko]

Attachment: XMLList testcase

The assertion is certainly gone but the results of XMLList.prototype.child still look pretty broken to me.

Comment 14

[Brendan Eich [:brendan]]

Attachment: proposed fix

I'll let mrbkap mark r+ after he runs the testsuite ;-).

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 201037 [details] [diff] [review]
proposed fix

Fixed on trunk.  I'll attach a roll-up branch patch.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: rollup patch for 1.8 branch

This is the new "ride-along" for rc2.

/be

Comment 17

[Asa Dotzler [:asa]]

moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2.

Comment 18

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be