9.16 KB, patch
|Details | Diff | Splinter Review|
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.
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).
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.
> The Shape destructor is never getting called. I'm quite sure this isn't true. But I'll double-check.
My js::Shape:~Shape() destructor is most definitely getting called. I confirmed this by adding a printf() statement to it.
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.
(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.
> 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.
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.
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.