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

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: smichaud, Assigned: smichaud)

Tracking

Trunk
Points:
---
Bug Flags:
wanted1.9.1 +
wanted1.9.0.x -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 321316 [details] [diff] [review]
Patch

This is a revision of a patch at bug 433644 (attachment 321101 [details] [diff] [review]).
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #321316 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

10 years ago
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.
Flags: wanted1.9.0.x?
(Assignee)

Comment 3

10 years ago
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).
Summary: Make sure Destroy() is called (at least once) before an nsIWidget is destroyed → Make sure Destroy() is called before an nsIWidget is destroyed
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.
Flags: wanted1.9.1+
(Assignee)

Comment 5

10 years ago
> 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).
Ok, let's just go with 433644 then.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
OS: Mac OS X → All
Hardware: PC → All
Comment on attachment 321316 [details] [diff] [review]
Patch

r=bzbarsky
Attachment #321316 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

10 years ago
Attachment #321316 - Flags: superreview?(vladimir)
Attachment #321316 - Flags: superreview?(vladimir) → superreview+
(Assignee)

Comment 8

10 years ago
Landed on mozilla-central.

(As per comment #4 and comment #5, this won't land on the 1.9.0 branch.)
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Depends on: 492123
(Assignee)

Updated

9 years ago
No longer depends on: 492123
You need to log in before you can comment on or make changes to this bug.