Crash [@ js::gc::MarkChildren]

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: gkw, Assigned: bhackett)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
x86
Mac OS X
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox5+ fixed, status2.0 wanted, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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).
(Reporter)

Comment 1

6 years ago
Tested on 64-bit shell.
Assuming the worst given GC MarkChildren.
Assignee: general → gal
Whiteboard: [sg:critical?]
(Reporter)

Updated

6 years ago
blocking2.0: --- → ?
blocking-fx: --- → ?
(Reporter)

Comment 3

6 years ago
#26 topcrash on https://crash-stats.mozilla.com/topcrasher/byversion/Firefox/4.0
(Reporter)

Updated

6 years ago
See Also: → bug 602803
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
gal?

Comment 7

6 years ago
I wasn't able to reproduce this with TM tip.
(Reporter)

Comment 8

6 years ago
Created attachment 526260 [details]
stack

I could still reproduce this on 32-bit Linux opt js shell on Ubuntu 10.10, TM changeset e06d53aec568.
(Reporter)

Comment 9

6 years ago
(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.
tracking-firefox5: --- → ?
tracking-firefox6: --- → ?
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.
(Assignee)

Comment 11

6 years ago
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.

Updated

6 years ago
blocking-fx: ? → ---

Updated

6 years ago
tracking-firefox5: ? → +
tracking-firefox6: ? → ---
(Assignee)

Comment 12

6 years ago
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.
Attachment #526454 - Flags: review?(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
Attachment #526454 - Flags: review?(brendan) → review+
(Reporter)

Comment 14

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

http://hg.mozilla.org/tracemonkey/rev/7ef107ab081e
Assignee: gal → bhackett1024
Blocks: 596805
No longer blocks: 639727
Status: NEW → ASSIGNED
(Assignee)

Comment 15

6 years ago
How, where and when should this get pushed?  (Also, not with a testcase, right?)
Status: ASSIGNED → NEW
Dan, Christian: can you advise re: comment 15? Thanks.

/be
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

6 years ago
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!

Updated

6 years ago
Attachment #526454 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 19

6 years ago
http://hg.mozilla.org/mozilla-aurora/rev/bfdb6e623a36
http://hg.mozilla.org/mozilla-central/rev/d50f943c21e3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Comment 20

6 years ago
in-testsuite? can be turned to + after test is eventually checked in.
Flags: in-testsuite?
(Assignee)

Updated

6 years ago
status-firefox5: --- → fixed
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Per security group discussion, requesting landing on mozilla-2.0.
Attachment #526454 - Flags: approval2.0?
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"
Crash Signature: [@ js::gc::MarkChildren]
Comment on attachment 526454 [details] [diff] [review]
patch

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.