561539 - "Assertion failure: isQName(), at ../jsobjinlines.h" or "Assertion failure: obj->getClass() == &js_NamespaceClass.base || IsQNameClass(obj->getClass()), at ../jsxml.cpp" with e4x

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

gczeal(1)
x = <x>></x>
print(x..x)

asserts js debug shell on TM tip without -j at Assertion failure: isQName(), at ../jsobjinlines.h:333

Tested on 64-bit Mac 10.6.3. Setting s-s because this involves gczeal.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This also asserts in 32-bit shells.

Another assert message can be Assertion failure: obj->getClass() == &js_NamespaceClass.base || IsQNameClass(obj->getClass()), at ../jsxml.cpp:161

autoBisect shows this is probably related to bug 560471:

The first bad revision is:
changeset:   41143:2d3e3a6cfba3
user:        Igor Bukanov
date:        Sat Apr 24 00:15:42 2010 +0200
summary:     bug 560471 - remove GC_LAST_DITCH and GC_KEEP_ATOMS. r=jorendorff

Comment 2

[Igor Bukanov]

This bug is sort-of regression from the bug 366975. That bug introduced a protection against wiping out weak roots in the last ditch GC when the END_GC callback allocates new GC things. But that was incomplete fix as in the patch I forgot to restore the weak roots back to the saved values. This was harmless as the browser callback does not allocate XML things.

The bug 560471 has started to use that functionality (now expressed in the form of AutoSaveWeakRoots class) on any path involving the last ditch GC. As such the last ditch GC would wipe out all weak roots after marking a copy. Thus a sequence that was safe previously like allocating XML thing and a string would be no longer a safe one.

Comment 3

[Igor Bukanov]

I have landed the fix for the bug as a followup for the bug 560471. But the rooting problem theoretically could exists on branches even if that is very unlikely. So we should fix this there using a trivial one-line of changes patch.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to comment #3)
> I have landed the fix for the bug as a followup for the bug 560471. But the
> rooting problem theoretically could exists on branches even if that is very
> unlikely. So we should fix this there using a trivial one-line of changes
> patch.

Landed on TM by Igor.

http://hg.mozilla.org/tracemonkey/rev/8e1084fb0cff

Setting branch flags for nomination...

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Can we get a roll-up patch either here on on bug 560471 and both bugs nominated if we think we need them on the branches? Thanks!

Comment 6

[Brendan Eich [:brendan]]

Bug 560358 uses "Preserve" as the single verb for what this bug's patch calls "SaveRestore". rs=me on renaming s/SaveRestore/Preserve/g.

/be

Comment 7

[Brendan Eich [:brendan]]

Sayrer, this is Igor's bug 560471, or a dup of it.

/be

Comment 8

[Igor Bukanov]

I have not made this bug a dup of the bug 560471 since branches would need different one-liner patch.

Comment 9

[Igor Bukanov]

Attachment: fix for 192

Comment 10

[Igor Bukanov]

Attachment: fix for 192 for real

The previous patch includes unrelated changes

Comment 11

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/22928ec2f2d9 - renaming AutoSaveRestoreWealRoots into AutoPreserveWeakRoots

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/22928ec2f2d9

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 442063 [details] [diff] [review]
fix for 192 for real

Approved for 1.9.2.5 and 1.9.1.11, a=dveditz for release-drivers

Comment 14

[Igor Bukanov]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/90658753b7da

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f4310a06ef2c

Comment 15

[Carsten Book [:Tomcat]]

Gary: can i use this testcase somehow in a Debug Build to verify this fix in 1.9.2/1.9.1 ?

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 442063 [details] [diff] [review]
fix for 192 for real

Approved for 1.9.0.20, a=dveditz