525409 - leaking nsWindows when constructed with an nsIWidget parent until the parent is destroyed

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Karl Tomlinson (:karlt)]

STR:
1) Open a browser window.
2) Open several tabs.
3) Close all tabs except one.

Expected results:
Parent nsWindow for the tab root nsWindows has one child.

Actual results:
Parent nsWindow still has a reference to all the destroyed tab root nsWindows.

nsWindow::Create calls (nsBaseWidget::)BaseCreate, which adds each tab's root nsWindow to the parent's child list.

However nsWindow::Destroy does not call nsBaseWidget::Destroy(), which would remove itself from the parent's child list.

Comment 1

[Rich Dougherty]

Attachment: Patch attempt 1

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 434454 [details] [diff] [review]
Patch attempt 1

When nsBaseWidget::Destroy() unlinks this window from its parent, the parent (or a sibling) removes its reference.  If that is the last reference it will cause |this| to be deleted.  Methods on |this| must not touch member variables after |this| is deleted.

Was there a reason for moving the release of the parent to before OnDestroy()?
Releasing the parent may destroy it.  Ideally we'd complete as much of the child destruction as possible before the parent gets destroyed.

Comment 3

[Rich Dougherty]

Attachment: Patch attempt 2

Thanks Karl. Now we wait until the end of nsWindow::Destroy() before removing the window from its parent and siblings. Not sure if the comment needs to mention the potential deletion; perhaps that point would be obvious to an experienced developer?

Comment 4

[Rich Dougherty]

Attachment: Patch attempt 3

- Make OnDestroy() the last thing called by Destroy() and added comment explaining why.
- Moved parent/child cleanup code into OnDestroy(), where it used to be.
- Move grip creation to happen earlier in OnDestroy(), before we start doing any cleanup at all (just in case).

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 434728 [details] [diff] [review]
Patch attempt 3

Excellent, thanks.

Comment 6

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/3c56d96a656d