Last Comment Bug 643839 - Crash [@ js::gc::MarkChildren]
: Crash [@ js::gc::MarkChildren]
Status: RESOLVED FIXED
[sg:critical?]
: crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
:
Mentors:
Depends on:
Blocks: jsfunfuzz 596805
  Show dependency treegraph
 
Reported: 2011-03-22 11:47 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-03-13 06:17 PDT (History)
10 users (show)
choller: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
wanted
unaffected
unaffected


Attachments
stack (21.60 KB, text/plain)
2011-04-15 08:30 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
patch (1.28 KB, patch)
2011-04-15 21:42 PDT, Brian Hackett (:bhackett)
brendan: review+
christian: approval‑mozilla‑aurora+
dveditz: approval2.0+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2011-03-22 11:47:45 PDT
try {
    x
} catch(r) {}
try {
    x = <x/>
    x.c = x;
    with(x) {
        x
    }
    x.__defineSetter__("x", function() {})
    gc()
    instanceof({
        a: x
    })
} catch(r) {}
gc()

crashes js opt shell on TM changeset c811be25eaad without -m nor -j at js::gc::MarkChildren but does not seem to crash debug shell.

Locking s-s just to be safe.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   63258:b16be37906fe
user:        Andreas Gal
date:        Wed Mar 09 00:53:56 2011 -0800
summary:     Don't shrink object slots during GC (bug 639727, r=bhackett).
Comment 1 Gary Kwong [:gkw] [:nth10sd] 2011-03-22 12:03:39 PDT
Tested on 64-bit shell.
Comment 2 Daniel Veditz [:dveditz] 2011-03-24 13:53:31 PDT
Assuming the worst given GC MarkChildren.
Comment 3 Gary Kwong [:gkw] [:nth10sd] 2011-03-30 09:42:47 PDT
#26 topcrash on https://crash-stats.mozilla.com/topcrasher/byversion/Firefox/4.0
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2011-03-31 14:01:40 PDT
Gal, I hear this might be the cause for a top crash bug as well, can you investigate here?
Comment 5 Daniel Veditz [:dveditz] 2011-04-08 10:54:43 PDT
Probably not going to be done in time for Macaw; we'll take it in a security update when it's ready.
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2011-04-14 13:54:26 PDT
gal?
Comment 7 Andreas Gal :gal 2011-04-14 14:28:33 PDT
I wasn't able to reproduce this with TM tip.
Comment 8 Gary Kwong [:gkw] [:nth10sd] 2011-04-15 08:30:27 PDT
Created attachment 526260 [details]
stack

I could still reproduce this on 32-bit Linux opt js shell on Ubuntu 10.10, TM changeset e06d53aec568.
Comment 9 Gary Kwong [:gkw] [:nth10sd] 2011-04-15 08:34:11 PDT
(In reply to comment #8)
> Created attachment 526260 [details]
> stack

Valgrind opt stack is also available in this attachment.

Setting tracking-firefox5/6 ? nomination flags to track this. Now #19 on topcrash list.
Comment 10 Christian Holler (:decoder) 2011-04-15 10:13:39 PDT
We have a very similar crash on the TI branch (see 649152) and the testcase there is also hard to reproduce (worked on Linux 64 bit but not on Mac OSX 64 bit). Maybe the underlying issue is the same, in that case I guess the additional information in that bug might be helpful.
Comment 11 Brian Hackett (:bhackett) 2011-04-15 11:03:14 PDT
I can also repro on OSX 32 bit opt, rev e06d53aec568 (tip).  I'll look at this tonight if no one gets to it by then.
Comment 12 Brian Hackett (:bhackett) 2011-04-15 21:42:17 PDT
Created attachment 526454 [details] [diff] [review]
patch

Patch.  The defineSetter overwrote a value property with a setter, but did not write undefined to the old slot.  The object in that old slot was collected, and then another property was added (from MarkSharpObjects of all places) which reused the slot and didn't write to it (addProperty* expect that slots above slotSpan are undefined), so it held a stale pointer to an already-collected object.

In debug mode we cleared the slot, but not in release mode (the patch just makes us always clear the slot; also, auto-bisect is a red herring, conservative GC etc).

Can we avoid this kind of code where debug mode affects the semantics of the program?  (Ran into similar issues in JM where we would invalidate entries only in debug mode).  Places where we write 0xdededede or similar in debug mode to trigger a crash on the next access are good, but I'm not sure what the intention here was.  This took an order of magnitude longer to understand than if it had showed up in a debug build.
Comment 13 Brendan Eich [:brendan] 2011-04-16 07:34:55 PDT
Comment on attachment 526454 [details] [diff] [review]
patch

Good catch.

For sure this should not have been DEBUG-only. That was a mistake that assumed the only observation of a slot at or above slotSpan would be from assertions, which are also DEBUG-only. A warning sign, in hindsight.

/be
Comment 14 Gary Kwong [:gkw] [:nth10sd] 2011-04-16 07:40:21 PDT
Based on inspection of line change, the regressing changeset is probably bug 596805 :

http://hg.mozilla.org/tracemonkey/rev/7ef107ab081e
Comment 15 Brian Hackett (:bhackett) 2011-04-16 08:02:12 PDT
How, where and when should this get pushed?  (Also, not with a testcase, right?)
Comment 16 Brendan Eich [:brendan] 2011-04-16 15:54:08 PDT
Dan, Christian: can you advise re: comment 15? Thanks.

/be
Comment 17 Christian Holler (:decoder) 2011-04-17 03:04:57 PDT
As this should affect FF4 stable as well as FF5 aurora build, I'd say push to TM trunk first (without test) and then it must be merged to FF4/FF5 branches (don't know exactly who does this right now).

Once a security update for FF4 was issued we can add the missing test to trunk.

Is that correct Daniel?
Comment 18 christian 2011-04-20 11:35:23 PDT
This already missed the boat for Macaw/4.0.1 and we aren't planning on doing a 4.0.2. That being said, we want this on mozilla-aurora so that it will end up in FF5 (and of course mozilla-central as well).

Please land this on mozilla-aurora default WITHOUT the test.

Thanks!
Comment 20 Gary Kwong [:gkw] [:nth10sd] 2011-04-22 21:19:41 PDT
in-testsuite? can be turned to + after test is eventually checked in.
Comment 21 Cameron Kaiser [:spectre] 2011-05-21 16:33:59 PDT
Per security group discussion, requesting landing on mozilla-2.0.
Comment 22 Daniel Veditz [:dveditz] 2011-06-03 16:35:34 PDT
Comment on attachment 526454 [details] [diff] [review]
patch

Approved for the mozilla2.0 repository, a=dveditz for release-drivers

When landed please add a link to the changeset and flip the status2.0 field to ".x-fixed"
Comment 23 Daniel Veditz [:dveditz] 2011-07-07 14:42:48 PDT
Comment on attachment 526454 [details] [diff] [review]
patch

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Comment 24 Christian Holler (:decoder) 2013-03-13 06:17:58 PDT
I wasn't able to rewrite the test in comment 0 to work without E4X. If this is possible and you want to add a test, feel free to do so and adjust the in-testsuite flag.

Note You need to log in before you can comment on or make changes to this bug.