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)
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)
3.46 KB,
text/plain
|
Details | |
8.16 KB,
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•13 years ago
|
||
Assuming [sg:critical?] unless otherwise known.
blocking2.0: --- → ?
tracking-firefox5:
--- → ?
tracking-firefox6:
--- → ?
Whiteboard: [ccbr][sg:critical?]
Assignee | ||
Updated•13 years ago
|
Assignee: general → igor
Comment 2•13 years ago
|
||
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: ? → -
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
status-firefox5:
--- → affected
Assignee | ||
Comment 3•13 years ago
|
||
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.
Assignee | ||
Comment 4•13 years ago
|
||
Gary: on what platform do you see the crash with TM tip?
Reporter | ||
Comment 5•13 years ago
|
||
(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()
Reporter | ||
Comment 6•13 years ago
|
||
A 64-bit js shell off JM changeset 4c7ebbb52876 with -j on Mac also asserts identically.
Reporter | ||
Comment 7•13 years ago
|
||
(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•13 years ago
|
||
I can reproduce on TM tip (rev 6c5ed6692f96, Mac OS X 10.6, 64-bit).
Comment 9•13 years ago
|
||
Igor, can you reproduce this with a 32-bit Linux build?
Updated•13 years ago
|
Assignee | ||
Comment 10•13 years ago
|
||
(In reply to comment #9) > Igor, can you reproduce this with a 32-bit Linux build? Yes.
Assignee | ||
Comment 11•13 years ago
|
||
What happens is that for some reason TraceMonitor::sweep() does not remove all the traces that contain a dead shape. Investigating.
Assignee | ||
Comment 12•13 years ago
|
||
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)
Assignee | ||
Comment 13•13 years ago
|
||
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)
Comment 14•13 years ago
|
||
Igor, any updates on a new patch for this?
Assignee | ||
Comment 15•13 years ago
|
||
(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.
Assignee | ||
Comment 16•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #541428 -
Flags: review?(gal) → review+
Assignee | ||
Comment 17•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/dc137da5a3b4
Whiteboard: [ccbr][sg:critical?] → [ccbr][sg:critical?] fixed-in-tracemonkey
Assignee | ||
Comment 18•13 years ago
|
||
(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?
Reporter | ||
Updated•13 years ago
|
Flags: in-testsuite?
Comment 19•13 years ago
|
||
cdleary-bot mozilla-central merge info: http://hg.mozilla.org/mozilla-central/rev/dc137da5a3b4
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 20•13 years ago
|
||
(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•13 years ago
|
||
Can the fix for this land for 6? If so, please request approval for beta!
Assignee | ||
Comment 22•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
qa-: fix verification not needed
Whiteboard: [ccbr][sg:critical?] fixed-in-tracemonkey → [ccbr][sg:critical?] fixed-in-tracemonkey [qa-]
Updated•12 years ago
|
Group: core-security
Did the testcase for this ever land?
Comment 26•12 years ago
|
||
(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.
Reporter | ||
Comment 27•12 years ago
|
||
Testcase needs tracer (-j) to trigger, tracer is long gone. -> VERIFIED.
Status: RESOLVED → VERIFIED
Comment 28•11 years ago
|
||
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.
Description
•