377737 - Remove Shutdown() from nsAccessNode::~nsAccessNode()

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Mats Palmgren (inactive)]

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");

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 2

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

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

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

... and fix the bogus comment too.

Comment 4

[Aaron Leventhal]

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

Comment 5

[Mats Palmgren (inactive)]

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

Need more work...

Comment 6

[Aaron Leventhal]

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();

Comment 7

[Mats Palmgren (inactive)]

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

Comment 8

[Aaron Leventhal]

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

Comment 9

[Mats Palmgren (inactive)]

Attachment: Patch rev. 4

This patch makes the mechanism work as originally intended.

Comment 10

[Aaron Leventhal]

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 11

[Mats Palmgren (inactive)]

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

Boris, can you review the XPCOM aspects of this patch please?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Looks fine.

Comment 13

[Aaron Leventhal]

Checked this in for Mats.