Closed Bug 657198 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox5 - wontfix
firefox6 - wontfix
firefox7 + fixed
blocking2.0 --- -
status2.0 --- wanted
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: gkw, Assigned: igor)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: [ccbr][sg:critical?] fixed-in-tracemonkey [qa-])

Attachments

(2 files, 1 obsolete file)

Attached file 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
Assuming [sg:critical?] unless otherwise known.
blocking2.0: --- → ?
Whiteboard: [ccbr][sg:critical?]
Assignee: general → igor
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.
blocking2.0: ? → -
status2.0: --- → wanted
I cannot reproduce this on TM tip with 64 bit Linux and GCC 4.4, apparently the changeset

changeset:   58554:52d20032116a
user:        Jason Orendorff <jorendorff@mozilla.com>
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.
Gary: on what platform do you see the crash with TM tip?
(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()
A 64-bit js shell off JM changeset 4c7ebbb52876 with -j on Mac also asserts identically.
(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).
I can reproduce on TM tip (rev 6c5ed6692f96, Mac OS X 10.6, 64-bit).
Igor, can you reproduce this with a 32-bit Linux build?
(In reply to comment #9)
> Igor, can you reproduce this with a 32-bit Linux build?

Yes.
What happens is that for some reason TraceMonitor::sweep() does not remove all the traces that contain a dead shape. Investigating.
Attached patch v1 (obsolete) — Splinter Review
When implementing HasUnreachableGCThings I have missed that we must search the whole tree for GC things, not just the tree root.
Attachment #535498 - Flags: review?(gal)
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.
Attachment #535498 - Attachment is obsolete: true
Attachment #535498 - Flags: review?(gal)
Igor, any updates on a new patch for this?
(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.
Attached patch v2Splinter Review
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.
Attachment #541428 - Flags: review?(gal)
Attachment #541428 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/dc137da5a3b4
Whiteboard: [ccbr][sg:critical?] → [ccbr][sg:critical?] fixed-in-tracemonkey
(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?
Flags: in-testsuite?
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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?
Can the fix for this land for 6? If so, please request approval for beta!
(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.
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.
qa-: fix verification not needed
Whiteboard: [ccbr][sg:critical?] fixed-in-tracemonkey → [ccbr][sg:critical?] fixed-in-tracemonkey [qa-]
Group: core-security
Did the testcase for this ever land?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Testcase needs tracer (-j) to trigger, tracer is long gone.

-> VERIFIED.
Status: RESOLVED → VERIFIED
Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.