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

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
6 years ago
7 months ago

People

(Reporter: smichaud, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

Comment 1

6 years ago
Created attachment 583690 [details] [diff] [review]
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.
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).
(Reporter)

Comment 3

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

Comment 4

6 years ago
> The Shape destructor is never getting called.

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

Comment 5

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

Comment 6

6 years ago
Created attachment 583697 [details] [diff] [review]
Debugging patch rev1 (also check js::Shape::parent, still *not* a fix)

I'm doing tryserver builds of this one, too.
Attachment #583690 - Attachment is obsolete: true
(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.
(Reporter)

Comment 8

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

Comment 9

6 years ago
Created attachment 583980 [details] [diff] [review]
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.
Attachment #583697 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Summary: js::Shape::finalize can be called more than once on the same object → js::Shape::finalize can be called on an object with a deleted/finalized parent
(Reporter)

Updated

6 years ago
Attachment #583980 - Attachment description: Debugging patch rev1 (fix problems, still *not* a fix) → Debugging patch rev2 (fix problems, still *not* a fix)
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.
(Assignee)

Updated

3 years ago
Assignee: general → nobody

Updated

7 months ago
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.