Shutdown() from nsAccessNode::~nsAccessNode() is useless because it will not invoke a Shutdown() method of a sub-class at that point. If there are code paths that don't make explicit Shutdown() calls and instead relies on the destructor to do it then there is a risk of crashing because the sub-class Shutdown() will not be invoked. If there are no such code paths then this call is redundant. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/base/nsRootAccessible.cpp&rev=1.199&root=/cvsroot&mark=870#868 http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/accessible/src/base/nsAccessNode.cpp&rev=1.44&root=/cvsroot&mark=116-118#111 I suggest that we remove it and instead add: NS_ASSERTION(!mWeakShell, "missing Shutdown() call");

Hookup Destroy() to Release() and make the Shutdown() call there instead of in the destructor. Add some assertions (which found the problem in nsCaretAccessible::Shutdown).

... and fix the bogus comment too.

Would a safer possibility be to add Shutdown() to every destructor in a class that inherits from nsAccessNode?

Comment on attachment 261776 [details] [diff] [review] Patch rev. 3 Need more work...

What if the destructor for just the classes that override Shutdown() would call Shutdown() directly. That would be these: src/atk/nsRootAccessibleWrap.h: NS_IMETHOD Shutdown(); src/base/nsAccessible.h: NS_IMETHOD Shutdown(); src/base/nsBaseWidgetAccessible.h: NS_IMETHOD Shutdown(); src/base/nsCaretAccessible.h: NS_IMETHOD Shutdown(); src/base/nsDocAccessible.h: NS_IMETHOD Shutdown(); src/base/nsRootAccessible.h: NS_IMETHOD Shutdown(); src/html/nsHTMLFormControlAccessible.h: NS_IMETHOD Shutdown(); src/html/nsHTMLTextAccessible.h: NS_IMETHOD Shutdown(); src/html/nsHTMLTextAccessible.h: NS_IMETHOD Shutdown(); src/msaa/nsDocAccessibleWrap.h: NS_IMETHOD Shutdown(); src/msaa/nsHTMLWin32ObjectAccessible.h: NS_IMETHOD Shutdown(); src/xforms/nsXFormsAccessible.h: NS_IMETHOD Shutdown(); src/xul/nsXULFormControlAccessible.h: NS_IMETHOD Shutdown(); src/xul/nsXULMenuAccessible.h: NS_IMETHOD Shutdown(); src/xul/nsXULTreeAccessible.h: NS_IMETHOD Shutdown(); src/xul/nsXULTreeAccessible.h: NS_IMETHOD Shutdown();

(In reply to comment #4) > Would a safer possibility be to add Shutdown() to every destructor in a > class that inherits from nsAccessNode? Doing this centrally seems easier to maintain in the long run. Also, I would advice against doing anything complex from a destructor *in general* since it can lead to surprises... That said, there is still something going on in bug 377767 that I don't understand yet...

> Doing this centrally seems easier to maintain in the long run. Yes, but the code was built around this incorrect assumption. I think it will be hard to catch everything, even with the assertion. But I hope not.

This patch makes the mechanism work as originally intended.

Comment on attachment 262112 [details] [diff] [review] Patch rev. 4 Wow, very cool -- I didn't know about NS_IMPL_RELEASE_WITH_DESTROY before. Will you get sr= just to make sure it's right? I don't know those mechanisms well enough to review properly, but in general it looks good.

Comment on attachment 262112 [details] [diff] [review] Patch rev. 4 Boris, can you review the XPCOM aspects of this patch please?

Comment on attachment 262112 [details] [diff] [review] Patch rev. 4 Looks fine.

Checked this in for Mats.