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.
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)
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.
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.
> 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-
Priority: -- → P2
Comment on attachment 321316 [details] [diff] [review] Patch r=bzbarsky
Attachment #321316 - Flags: review?(bzbarsky) → review+
Attachment #321316 - Flags: superreview?(vladimir)
Attachment #321316 - Flags: superreview?(vladimir) → superreview+
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
You need to log in before you can comment on or make changes to this bug.