712831 - js::Shape::finalize can be called on an object with a deleted/finalized parent

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

As a spin-off from bug 711794 and bug 712448, I've been playing around
with the js::Shape::finalize method, looking for more fundamental
explanations as to why we've had crashes in this method (on all
platforms, in various quantities) since FF 5.0.

I've now written a debugging patch that shows this method can be
called more than once on the same js::Shape object.  This seems wrong,
and I wonder if	it could be at the root	of all these crashes.

In my next comment I'll attach my debugging patch.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Debugging patch (*not* a fix)

So far I've only tested on OS X, where it's quite easy to trigger the "js::Shape::finalize() called on finalized shape" error message.

But I've just started an all-platform tryserver build, whose results should be available tomorrow morning.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The Shape destructor is never getting called. That's why you're getting the messages.

If you have the constructor remove the shape from the finalizer set, then I think the checks you're doing would be correct and useful (although I'm a little skeptical that you'll find anything).

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 583690 [details] [diff] [review]
Debugging patch (*not* a fix)

Just to make sure people are aware, this patch is against current trunk code.  That's what I've been testing.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> The Shape destructor is never getting called.

I'm quite sure this isn't true.  But I'll double-check.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

My js::Shape:~Shape() destructor is most definitely getting called.  I confirmed this by adding a printf() statement to it.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Attachment: Debugging patch rev1 (also check js::Shape::parent, still *not* a fix)

I'm doing tryserver builds of this one, too.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Steven Michaud from comment #5)
> My js::Shape:~Shape() destructor is most definitely getting called.  I
> confirmed this by adding a printf() statement to it.

We allocate some shapes on the stack. I suspect that those are the destructor calls you're seeing. I'm quite sure that we never call the destructor for a GC-allocated Shape. We allocate GC-able shapes through js_NewGCShape. You should be able to check whether we call a destructor on a shape that's allocated through that function.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

> We allocate GC-able shapes through js_NewGCShape. You should be able
> to check whether we call a destructor on a shape that's allocated
> through that function.

OK, I'll check that tomorrow.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: Debugging patch rev2 (fix problems, still *not* a fix)

(Following up comment #8)

This was a lot harder than I realized.  But I think I've now managed
it in this latest revision of my patch.

It shows (I think) that js::Shape::finalize() is never called on a
non-GCed object, and js::Shape::~Shape() is never called on a GCed
object.  But it still shows problems:  Apparently there are many cases
of js::Shape::finalize() being called on a Shape who's parent has
already been finalized/deleted.

In writing this patch I assumed	that (as I was told) all GCed objects
(and only GCed objects) are allocated using js_NewGCShape(), and that
all Shape objects (GCed or non-GCed) are initialized using one of the
constructors in jsscopeinlines.h.

I don't *think* I've made any mistakes in this patch.  Please let me
know if I did.

For now I can't spend any more time on this -- I'm on vacation from
tomorrow!  I'll come back to this in January to see what I can make of
it.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Hi Steve,
It's not unexpected that a shape's parent will have already been finalized before the shape. The isMarked() call checks if the parent is still alive (i.e., not finalized). If the parent isn't live, we just don't ever touch it.

If you have any more questions about shapes, it's probably easiest to find me on IRC. I'm billm on the #jsapi channel.