350206 - Assertion failure: serial <= n in jsxml.c

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Lubos Ures]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322; .NET CLR 2.0.50727)
Build Identifier: JavaScript-C 1.7 pre-release 1 2006-04-04

Script (5 lines below) always ends with above assertion failure:

var pa = <state1 xmlns="http://aaa" id="D1"><tag1/></state1>
var ch = <state2 id="D2"><tag2/></state2>
pa.appendChild(ch);
pa.@msg = "Before assertion failure";
pa.toXMLString();

Without the xmlns attribute in <state1> script works fine. It doesn't matter if child gets appended before inserting attribute msg or vice versa. Before the failure the partially formatted output looks like <state1 xmls:aaa="http://aaa xmlns="http://aaa" id="D1" msg="Before assertion failure"> and <tag1 xmlns.


Reproducible: Always

Steps to Reproduce:
1. run the 5 source lines from detailed description in jsshell


Actual Results:  
Assertion failure: serial <= n, at ../../../SpiderMonkey/17.1/src/jsxml.c:2554

Expected Results:  
No output from script

Observation from debugger(devenv): before the failure the partially formatted output (structure sb in XMLToXMLString) looks like '<state1 xmls:aaa="http://aaa xmlns="http://aaa" id="D1" msg="Before assertion failure">' at higher recursion level and '<tag1 xmlns' at lower level.

Comment 1

[Brendan Eich [:brendan]]

E4X, sigh.

/be

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Fix GeneratePrefix bug

The string at cp is never modified, so the consequent will always be evaluated and we'll continue do-while looping forever without the assertion.  Fix this by comparing using a pointer initialized to cp and changed to bp if a collision happens.

Note that while this fixes GeneratePrefix, there's still an underlying bug in toXMLString (assuming xmlns:aaa-1 is superfluous; reading the spec makes me loath to find out for sure).  With this patch:

js> pa.toXMLString();
<state1 xmlns:aaa="http://aaa" xmlns="http://aaa" id="i1" msg="Before assertion failure">
  <tag1 xmlns:aaa-1="http://aaa"/>
  <state2 id="i2">
    <tag2/>
  </state2>
</state1>

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 235550 [details] [diff] [review]
Fix GeneratePrefix bug

Whoa, I can't believe how broken this was. Thanks for diagnosing.

How about eliminating tmp by setting bp = cp initially, then instead of testing if (!bp), test if (bp == cp) in order to decide to call JS_malloc.

I haven't the heart to read the awful spec yet, but I will on Monday.

/be

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Eliminate tmp

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Oops

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 235608 [details] [diff] [review]
Oops

Excellent -- you able to do the trunk checkin?  Thanks,

/be

Comment 7

[Jeff Walden [:Waldo]]

> you able to do the trunk checkin?

No; unlike the last time I recall this happening, despot's actually enforcing partition restrictions.

Comment 8

[Brendan Eich [:brendan]]

Just for patching this bug (not to mention lots of generator/iterator help), you are now a Member.  So check that patch in -- and thanks!

/be

Comment 9

[Jeff Walden [:Waldo]]

Patch checked in on trunk.

Comment 10

[Brendan Eich [:brendan]]

Save iloop fix, good for 1.8.1 thanks to Jeff.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: followup fix to relieve warning

This needs to follow the main patch into the 1.8 branch, if the main patch is approved.

/be

Comment 12

[Bob Clary [:bc] (inactive)]

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

Comment 13

[Mike Schroepfer]

Comment on attachment 235608 [details] [diff] [review]
Oops

a=schrep/beltnzer for drivers.

Comment 14

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 windows/mac*/linux

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Testcase

Since nobody's checked yet that our altered behavior on the original JS test code is correct, here's a testcase which is definitely correct and (obviously) must trigger prefix generation (since |n2| is created with an undefined prefix) and will cause a namespace collision:

  var t1 = <ns2:t1 xmlns:ns2="http://bar/ns2"/>;
  var n2 = new Namespace("http://ns2");
  t1.@n2::a1 = "a1 from ns2";
  t1.toXMLString();

I've tested this against pre-checkin code, and it does produce the assertion.

Comment 16

[Jeff Walden [:Waldo]]

Checked in on branch.

Comment 17

[Bob Clary [:bc] (inactive)]

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

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.8, 1.9 20060830 windows/mac*/linux