56703 - leak global object, etc., in main mail window

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm filing this as a toolkit bug rather than a mail bug because I strongly
suspect it's a toolkit problem.

Opening the main mail window (run "mozilla -mail", and exit) leaks a
GlobalWindowImpl that is not garbage collected.  There is a single leaked JS
root: an nsXPCWrappedJS (that is an nsIController).  It is leaked from an
nsXULControllers object (it is inserted into that objects supports array and
never removed).  The leaked nsXULControllers object is leaked from a
GlobalWindowImpl.  This seems like a simple circular reference problem that
should be fixed by adding |mControllers = nsnull; // Forces release| to
|GlobalWindowImpl::CleanUp|.  However, that doesn't fix it.  I have no idea why?
Where did I go wrong?

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: leaked objects

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A few more thoughts.  I previously thought that CleanUp was called because the
window's script object was not rooted.  However, it could be that the window's
script object was never requested.  (How much could such a window be used? 
Maybe it was some sort of prototype?)

The mControllers=nsnull might be a good idea anyway, whether it's the problem
here or not.  I think once in the past I saw it reduce the fallout from another
leak.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The changes for bug 55426 may make this harder to detect.

Comment 4

[Peter Trudelle]

->danm for investigation. Please nominate for rtm or future as appropriate.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm seeing a very similar leak in the message compose window.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm, nsHTMLInputElement and nsHTMLTextAreaElement also have an
nsXULControllers.  I bet that's the problem.  If so, is releasing it on
SetDocument(null) the right solution?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, it didn't work.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It didn't work because nsGfxTextControlFrame2::Destroy calls GetControllers.  At
least, that's why fixing the nsXULControllers leaked from the input element
didn't work.  I still don't know if that's the cause of the bigger leak.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, I fixed the leak of the nsXULControllers from the input element (by adding
a SetControllers method and calling it in nsGfxTextControlFrame2::Destroy), and
the rest of the leak didn't go away, so the cycle is through the
nsXULControllers object that is the mControllers of the GlobalWindowImpl.  So
what's wrong here?  Is GlobalWindowImpl::Close() not being called?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that fixes the message-compose leak, but not the main mail window leak

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch that may help main mail window leak, since it makes the crash in bug 65798 reliable

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking this bug since I've been working on it.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: proposed patch

Comment 14

[Chris Waterson]

Nice. r=waterson

Comment 15

[Chris Waterson]

I should add, I'm assuming you only want r= on the last patch?

Comment 16

[Brendan Eich [:brendan]]

sr=[e-mail].  Only thought is to subroutine so that CleanUp and
SetDocShell both call a BreakCycles method (or a better-named one).  Your call.

/be

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix for leak in main mail window

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmm.  Perhaps a more general fix for this type of leak would be to set
mDocument to null in GlobalWindowImpl::SetDocShell.  This might break
a lot of potential cycles, but I'm not sure whether it would break all
the ones we need to break...

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: same patch with a comment explaining why

Comment 20

[Chris Waterson]

r=waterson

Comment 21

[David Hyatt]

dbaron, you need to patch text area and input element as well, since they have
controllers also.

sr=hyatt on the XULElement, but please make the same patch to the other elts
before checking in.

Comment 22

[David Hyatt]

Never mind.  Odds are slim it would become a problem.  Fire away.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix for nsXULElement checked in 2001-03-09 19:18 PST.  I need to investigate
whether we need a:
 * Input/TextArea fix
 * general fix in GlobalWindowImpl
so leaving the bug open for now.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Moving to 0.9.1 for dealing with the remaining issues.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: old patch that I removed from my tree that doesn't help anything (?)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

dbaron, is this still reproducible?

Comment 27

[Andrew McCreight [:mccr8]]

This is ancient and probably not relevant any more.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

based on comment 23, fixed