643839 - Crash [@ js::gc::MarkChildren]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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] (NOT official MoCo now)]

Tested on 64-bit shell.

Comment 2

[Daniel Veditz [:dveditz]]

Assuming the worst given GC MarkChildren.

Comment 3

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

#26 topcrash on https://crash-stats.mozilla.com/topcrasher/byversion/Firefox/4.0

Comment 4

[Johnny Stenback (:jst)]

Gal, I hear this might be the cause for a top crash bug as well, can you investigate here?

Comment 5

[Daniel Veditz [:dveditz]]

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)]

gal?

Comment 7

[Andreas Gal :gal]

I wasn't able to reproduce this with TM tip.

Comment 8

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

Attachment: 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] (NOT official MoCo now)]

(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)]

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 [Laid off!]]

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 [Laid off!]]

Attachment: 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]]

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] (NOT official MoCo now)]

Based on inspection of line change, the regressing changeset is probably bug 596805 :

http://hg.mozilla.org/tracemonkey/rev/7ef107ab081e

Comment 15

[Brian Hackett [Laid off!]]

How, where and when should this get pushed?  (Also, not with a testcase, right?)

Comment 16

[Brendan Eich [:brendan]]

Dan, Christian: can you advise re: comment 15? Thanks.

/be

Comment 17

[Christian Holler (:decoder)]

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

[LegNeato]

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 19

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/mozilla-aurora/rev/bfdb6e623a36
http://hg.mozilla.org/mozilla-central/rev/d50f943c21e3

Comment 20

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

in-testsuite? can be turned to + after test is eventually checked in.

Comment 21

[Cameron Kaiser [:spectre]]

Per security group discussion, requesting landing on mozilla-2.0.

Comment 22

[Daniel Veditz [:dveditz]]

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]]

Comment on attachment 526454 [details] [diff] [review]
patch

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

Comment 24

[Christian Holler (:decoder)]

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.