434089 - Make sure Destroy() is called before an nsIWidget is destroyed

Status: RESOLVED FIXED

(Core :: Widget, defect, P2)

Description

[Steven Michaud [:smichaud] (Retired)]

The nsView destructor always calls nsIWidget::Destroy() on the widget
with which it's associated before destroying it.  But there are some
other places in the tree where a widget is destroyed without calling
nsIWidget::Destroy().

Specific widgets' implementations of Destroy() give them a chance to
tear down their infrastructure before their destructors are called --
by which time it may no longer be safe to do so.  It also gives them a
chance to NULL important internal variables, and thereby (via NULL
checks) stop themselves from functioning before they're actually
destroyed -- which makes it less likely that they'll continue to be
used (by external code) after they've been destroyed.

So (if at all feasible) nsIWidget::Destroy() should be called on every
widget before that widget is destroyed.

This patch goes at least part of the way towards this goal.

If nsIWidget::Destroy() can be called from multiple locations in the
tree, it's at least conceivable that it may be called more than once
on the same widget.  If so, every kind of widgets' Destroy() methods
will need to be idempotent.  I know that Cocoa widgets' Destroy()
methods (for nsChildView and nsCocoaWindow widgets) are idempotent,
and I _think_ all the others are, too.  But this will need to be
tested.

The fact that this patch touches cross-platform code in several
modules also requires fairly widespread testing.

As far as I know there's no way to verify this patch with automated
testing.  So it should be landed well before the release that it
targets -- to make sure that any problems it might cause are shaken
out in good time.

I'll post a patch in my next comment.  This will be a revision of one
already posted at bug 433644 (attachment 321101 [details] [diff] [review]).  This bug has been
spun off from bug 433644.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch

This is a revision of a patch at bug 433644 (attachment 321101 [details] [diff] [review]).

Comment 2

[Steven Michaud [:smichaud] (Retired)]

As I say in comment #0, this patch needs some testing, and therefore
should be landed well before the release that it targets.  But I think
it's reasonable to try to get this into the first Firefox 3 point
release.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

I've looked more closely at the code that creates and destroys
widgets, and I can't find any places where it seems possible that
Destroy() could be called more than once on the same nsIWidget.

This patch still needs testing -- because it's a fairly large change.
But I think I was mistaken to worry about Destroy()'s idempotence (at
least as the patch now stands).

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Do we really need this in 1.9.0.x?  We should fix it for 1.9.1 for sure, but unless it's causing some badness right now, it seems risky to try for 1.9.0.x.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> Do we really need this in 1.9.0.x?

As long as bug 433644 is fixed in 1.9.0.x I'd say no.  In that case
waiting for 1.9.1 is fine (and I think is a good idea).

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Ok, let's just go with 433644 then.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 321316 [details] [diff] [review]
Patch

r=bzbarsky

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Landed on mozilla-central.

(As per comment #4 and comment #5, this won't land on the 1.9.0 branch.)