Closed Bug 525409 Opened 13 years ago Closed 13 years ago

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


(Core :: Widget: Gtk, defect)

Not set





(Reporter: karlt, Assigned: rich)


(Keywords: memory-leak)


(1 file, 2 obsolete files)

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.
Attached patch Patch attempt 1 (obsolete) — Splinter Review
Attachment #434454 - Flags: review?(karlt)
Assignee: nobody → rich
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.
Attachment #434454 - Flags: review?(karlt)
Attached patch Patch attempt 2 (obsolete) — Splinter Review
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?
Attachment #434454 - Attachment is obsolete: true
Attachment #434673 - Flags: review?(karlt)
Attached patch Patch attempt 3Splinter Review
- 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).
Attachment #434673 - Attachment is obsolete: true
Attachment #434728 - Flags: review?(karlt)
Attachment #434673 - Flags: review?(karlt)
Comment on attachment 434728 [details] [diff] [review]
Patch attempt 3

Excellent, thanks.
Attachment #434728 - Flags: review?(karlt) → review+
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
You need to log in before you can comment on or make changes to this bug.