657198 - TM: "Assertion failure: shape->previous() == obj->lastProperty()," with gc

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

eval("")
o15 = {}
function f11(o) {
    props = Object.getOwnPropertyNames(o)
    prop = props.length ? prop[prop.e] + "" : ""
    o[prop] = 3
}
function f12(o) {
    _someglobal_ = o;
    for (j = 0; j < 5; j++) {
        for (x in {
            x: {
                x: function() {
                    return _someglobal_
                }
            }.x()
        }.x) {
            ({
                x: {
                    x: function() {}
                }.x()
            }[x])
        }
        gc()
    }
} {
    for (i = 0; i < 100; i++) {
        f12(o15)
        f11(o15)
    }
}

asserts js debug shell on TM changeset 599d1c6cba63 with -j at Assertion failure: shape->previous() == obj->lastProperty(), setting s-s just-in-case because this involves gc.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   54718:b079aae53212
user:        Igor Bukanov
date:        Tue Sep 21 14:58:19 2010 +0200
summary:     bug 597736 - fixing TreeFragment leak. r=gal

Comment 1

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

Assuming [sg:critical?] unless otherwise known.

Comment 2

[Daniel Veditz [:dveditz]]

We might take this in Fx5 if we get a safe patch, but likely the next release since we're already in the beta period.

Comment 3

[Igor Bukanov]

I cannot reproduce this on TM tip with 64 bit Linux and GCC 4.4, apparently the changeset

changeset:   58554:52d20032116a
user:        Jason Orendorff <[e-mail]>
date:        Thu Dec 09 12:04:35 2010 -0600
summary:     Bug 614051 - TM: wrong behavior setting existing properties to joined function object values again. r=brendan.


fixes the bug.

Comment 4

[Igor Bukanov]

Gary: on what platform do you see the crash with TM tip?

Comment 5

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

(In reply to comment #4)
> Gary: on what platform do you see the crash with TM tip?

Still asserts for me on TM changeset dba38247d691 with -j on a 32-bit js debug shell on Mac OS X 10.6 at Assertion failure: shape->previous() == obj->lastProperty()

Comment 6

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

A 64-bit js shell off JM changeset 4c7ebbb52876 with -j on Mac also asserts identically.

Comment 7

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

(In reply to comment #6)
> A 64-bit js shell off JM changeset 4c7ebbb52876 with -j on Mac also asserts
> identically.

So does a 32-bit js shell off the same changeset with -j on Linux using gcc 4.5.2 (Ubuntu Linux 11.04).

Comment 8

[Jesse Ruderman]

I can reproduce on TM tip (rev 6c5ed6692f96, Mac OS X 10.6, 64-bit).

Comment 9

[David Mandelin [:dmandelin]]

Igor, can you reproduce this with a 32-bit Linux build?

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Igor, can you reproduce this with a 32-bit Linux build?

Yes.

Comment 11

[Igor Bukanov]

What happens is that for some reason TraceMonitor::sweep() does not remove all the traces that contain a dead shape. Investigating.

Comment 12

[Igor Bukanov]

Attachment: v1

When implementing HasUnreachableGCThings I have missed that we must search the whole tree for GC things, not just the tree root.

Comment 13

[Igor Bukanov]

Comment on attachment 535498 [details] [diff] [review]
v1

The patch is bogus and lead to infinite recursion in HasUnreachableGCThings as dependent/linked tree may point back to the original tree fragment.

Comment 14

[Johnny Stenback (:jst)]

Igor, any updates on a new patch for this?

Comment 15

[Igor Bukanov]

(In reply to comment #14)
> Igor, any updates on a new patch for this?

Sorry, but I could not make it before 2011-06-15 as I need to travel for personal reasons and will be mostly offline until 2011-06-14.

Comment 16

[Igor Bukanov]

Attachment: v2

The new version navigates the whole trace tree searching for dead GC things. To detect cycles in the linked traces the patch adds a flag per fragment that is cleared after the trace is visited.

It is possible to minimize the number if times a trace fragment is visited, but that complicates the code and I opted for simplicity here.

Comment 17

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/dc137da5a3b4

Comment 18

[Igor Bukanov]

(In reply to comment #17)
> http://hg.mozilla.org/tracemonkey/rev/dc137da5a3b4

This is landed without the test case. Do we have a way to flag the bug to add the test case when FF with the fix is released?

Comment 19

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/dc137da5a3b4

Comment 20

[David Mandelin [:dmandelin]]

(In reply to comment #18)
> (In reply to comment #17)
> > http://hg.mozilla.org/tracemonkey/rev/dc137da5a3b4
> 
> This is landed without the test case. Do we have a way to flag the bug to
> add the test case when FF with the fix is released?

That's a good question. I don't think we have a convenient way to track things like that. Do we?

Comment 21

[Johnny Stenback (:jst)]

Can the fix for this land for 6? If so, please request approval for beta!

Comment 22

[Igor Bukanov]

(In reply to comment #21)
> Can the fix for this land for 6? If so, please request approval for beta!

I need to land the bug 667507 first as the patch here introduced a regression.

Comment 23

[Daniel Veditz [:dveditz]]

Seems unlikely we'd get bug 667507 through the process in time to make Firefox 6 safely. Messing with this area of the code has already caused that one regression, safer to just leave this for Firefox 7.

Comment 24

[u279076]

qa-: fix verification not needed

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Did the testcase for this ever land?

Comment 26

[David Mandelin [:dmandelin]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #25)
> Did the testcase for this ever land?

This was a tracer bug so I don't think we need the test case.

Comment 27

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

Testcase needs tracer (-j) to trigger, tracer is long gone.

-> VERIFIED.

Comment 28

[Christian Holler (:decoder)]

Bug in removed tracer code, setting in-testsuite- flag.