Closed Bug 643839 Opened 13 years ago Closed 13 years ago

Crash [@ js::gc::MarkChildren]


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox5 + fixed
status2.0 --- wanted
status1.9.2 --- unaffected
status1.9.1 --- unaffected


(Reporter: gkw, Assigned: bhackett1024)



(Keywords: crash, regression, testcase, Whiteboard: [sg:critical?])

Crash Data


(2 files)

try {
} catch(r) {}
try {
    x = <x/>
    x.c = x;
    with(x) {
    x.__defineSetter__("x", function() {})
        a: x
} catch(r) {}

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).
Tested on 64-bit shell.
Assuming the worst given GC MarkChildren.
Assignee: general → gal
Whiteboard: [sg:critical?]
blocking2.0: --- → ?
blocking-fx: --- → ?
Gal, I hear this might be the cause for a top crash bug as well, can you investigate here?
Probably not going to be done in time for Macaw; we'll take it in a security update when it's ready.
blocking2.0: ? → ---
status2.0: --- → wanted
I wasn't able to reproduce this with TM tip.
Attached file stack
I could still reproduce this on 32-bit Linux opt js shell on Ubuntu 10.10, TM changeset e06d53aec568.
(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.
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.
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.
blocking-fx: ? → ---
Attached patch patchSplinter Review
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.
Attachment #526454 - Flags: review?(brendan)
Comment on attachment 526454 [details] [diff] [review]

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.

Attachment #526454 - Flags: review?(brendan) → review+
Based on inspection of line change, the regressing changeset is probably bug 596805 :
Assignee: gal → bhackett1024
Blocks: 596805
No longer blocks: 639727
How, where and when should this get pushed?  (Also, not with a testcase, right?)
Dan, Christian: can you advise re: comment 15? Thanks.

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?
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.

Attachment #526454 - Flags: approval-mozilla-aurora+
Closed: 13 years ago
Resolution: --- → FIXED
in-testsuite? can be turned to + after test is eventually checked in.
Flags: in-testsuite?
Per security group discussion, requesting landing on mozilla-2.0.
Attachment #526454 - Flags: approval2.0?
Comment on attachment 526454 [details] [diff] [review]

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"
Crash Signature: [@ js::gc::MarkChildren]
Comment on attachment 526454 [details] [diff] [review]

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Attachment #526454 - Flags: approval2.0? → approval2.0+
Group: core-security
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.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.