596103 - Shape is used after it is GCed

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

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

Comment 1

[Jesse Ruderman]

Attachment: valgrind's explanation

Comment 2

[Andreas Gal :gal]

Steve, let me know if you need help with this.

Comment 3

[Jesse Ruderman]

This *might* be the #14 topcrash on nightlies (see bug 595912).  Does that mean it should block beta 6?

Comment 4

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

Thanks Jesse for helping to reduce this. It was kind of a real pain to reduce.

Comment 5

[Jason Orendorff [:jorendorff]]

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)

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Brendan Eich [:brendan]]

Attachment: Fix layering

It's the little things...

/be

Comment 8

[Brendan Eich [:brendan]]

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

This blocks b6.

/be

Comment 9

[Jesse Ruderman]

Why does it have to loop ~300 times before hitting the assertion?

Comment 10

[Steve Fink [:sfink] [:s:]]

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.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: counterproposal - Same fix, extra fixin's

Comment 12

[Jason Orendorff [:jorendorff]]

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

Comment 13

[Jesse Ruderman]

Don't forget to hg add the testcase.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: counterproposal, with test

Well, all right.

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/6e5f17315d3d

Thanks to all,

/be

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/6e5f17315d3d

Comment 18

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-596103.js.