Shape is used after it is GCed

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
JavaScript Engine
P1
critical
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: brendan)

Tracking

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

Trunk
mozilla2.0b7
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical?] fixed-in-tracemonkey)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 474939 [details]
testcase

To reproduce:
  Run "./js t.js". (Don't paste the testcase into an interactive shell.)

Result:
  Assertion failure: thing, at js/src/jsgc.cpp:1895
or
  Assertion failure: !(addr & GC_CELL_MASK), at js/src/jsgc.cpp:425

The first bad revision is:
changeset:   52718:bb3730430112
user:        Steve Fink
date:        Wed Sep 01 14:09:54 2010 -0700
summary:     Bug 584175 - Unify various JS probes into a single set of static probe points. r=gal
(Reporter)

Comment 1

8 years ago
Created attachment 474941 [details]
valgrind's explanation

Updated

8 years ago
Assignee: general → sphink

Updated

8 years ago
Severity: critical → normal

Comment 2

8 years ago
Steve, let me know if you need help with this.
blocking2.0: --- → ?
(Reporter)

Comment 3

8 years ago
This *might* be the #14 topcrash on nightlies (see bug 595912).  Does that mean it should block beta 6?
Thanks Jesse for helping to reduce this. It was kind of a real pain to reduce.
var y = [];
Object.create(y);  // give y an emptyShape; new object is garbage
gc();  // array_trace does not mark y's emptyShape, so it is collected
y.t = 3;  // slowify y
gc();  // tries to trace y's emptyShape (now reachable from y)
Bonus points if you can add DEBUG-only code that makes this crash without involving `a` or having to loop 600 times. I feel sure it can be done.
(Assignee)

Comment 7

8 years ago
Created attachment 475129 [details] [diff] [review]
Fix layering

It's the little things...

/be
Assignee: sphink → brendan
Status: NEW → ASSIGNED
Attachment #475129 - Flags: review?(jorendorff)
(Assignee)

Comment 8

8 years ago
Thanks, J&J for the help. Sphink, sorry to darken your door with this bug.

This blocks b6.

/be
Severity: normal → critical
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → mozilla2.0b6
(Reporter)

Comment 9

8 years ago
Why does it have to loop ~300 times before hitting the assertion?
It doesn't, exactly. This'll crash too:

var y = [ [] for (i in function(n) { while(n) yield n--; }(100)) ];
for (i in y) Object.create(y[i]);
gc();
for (i in y) y[i].t = 42;
gc();

But it still needs a bunch of them piling up before it'll barf.
Created attachment 475179 [details] [diff] [review]
counterproposal - Same fix, extra fixin's
Attachment #475179 - Flags: review?(brendan)
(In reply to comment #9)
> Why does it have to loop ~300 times before hitting the assertion?

Because the GC'd Shape is only written over with 0xdadadada if the whole page of 
shapes is empty--basically only if that particular empty shape was the only thing on the page. The test loops, allocating one new Shape per iteration, until it triggers this case.

The patch adds a DEBUG-only memset call in JSShape::insertFree, so we poison every Shape with dadadada as soon as it's GC'd. With that, we crash the second time through the loop.
(Reporter)

Comment 13

8 years ago
Don't forget to hg add the testcase.
Created attachment 475193 [details] [diff] [review]
counterproposal, with test

Well, all right.
Attachment #475179 - Attachment is obsolete: true
Attachment #475193 - Flags: review?(brendan)
Attachment #475179 - Flags: review?(brendan)
(Assignee)

Comment 15

8 years ago
Created attachment 475203 [details] [diff] [review]
patch to commit

Not really a counterproposal, there -- I had almost the char-for-char identical test under construction (copied different boilerplate including a modeline), but I attached only the fix-patch in a big rush in case someone wanted to test it.

Thanks for the test and jsscope.h patch and review.

/be
Attachment #475129 - Attachment is obsolete: true
Attachment #475193 - Attachment is obsolete: true
Attachment #475203 - Flags: review+
Attachment #475193 - Flags: review?(brendan)
Attachment #475129 - Flags: review?(jorendorff)
(Assignee)

Comment 16

8 years ago
http://hg.mozilla.org/tracemonkey/rev/6e5f17315d3d

Thanks to all,

/be
Whiteboard: [sg:critical?] → [sg:critical?] fixed-in-tracemonkey

Comment 17

8 years ago
http://hg.mozilla.org/mozilla-central/rev/6e5f17315d3d
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Updated

8 years ago
Group: core-security
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected

Updated

7 years ago
blocking2.0: ? → betaN+
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-596103.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.