571159 - Leak nsGlobalWindow with unknown-content-type dialog

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100607 Minefield/3.7a5pre

When I quit Firefox after loading the testcase, it leaks 5x nsDocument and 2x nsGlobalWindow.  This happens consistently.

This might explain one of the most frequent oranges, bug 538462.

Comment 1

[Jesse Ruderman]

It doesn't matter whether I click save, click cancel, or ignore the diaog and quit with it still up.  It leaks every way.

Comment 2

[Jesse Ruderman]

Bug 538462 is not only one of the most frequent oranges, but also a difficult one to star, because you have to correlate the DOMWindow from the leak log with the ++DOMWindow from the mochitest log.

Comment 3

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

Any idea if this is Mac-only?

Comment 4

[Jesse Ruderman]

Happens on Linux64 too.

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v1

This seems to fix it for me.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 453850 [details] [diff] [review]
v1

so we are completely unhooking the cycle, right?

r=sdwilsh

Comment 7

[(no longer active)]

Shouldn't this cycle be detected by the cycle collector, without needing to break it manually in js?

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to comment #7)
> Shouldn't this cycle be detected by the cycle collector, without needing to
> break it manually in js?

No, it can't. nsUnknownContentTypeDialog.prototype has a property progressListener, which is an object that stores a pointer to the nsUnknownContentTypeDialog in a property helperAppDlg. The prototype will stay alive until we unload the module, which happens after the last cycle collection, as far as the cycle collector is concerned all these objects are alive until shutdown. IIRC we can't really run cycle collection after unloading modules, but even if we could it seems like a bad idea to keep this dialog alive after it has been closed, it's just unnecessary bloat. Storing objects with cycles on prototypes of components is a bad idea in general.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v2

I tried checking in attachment 453850 [details] [diff] [review], but it caused test failures. nsUnkownContentTypeDialog uses a timer that uses a check for mDialog being null to decide whether we still need to show the dialog. So nulling out mDialog will cause it to be reshown when the timer fires next. This patch also fixes the leak and passes on try server.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 455265 [details] [diff] [review]
v2

You just moved code so I'm not going to nit pick style.  r=sdwilsh

Comment 11

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

I landed this for you:
http://hg.mozilla.org/mozilla-central/rev/a8dd758e11f1