Closed Bug 571159 Opened 11 years ago Closed 11 years ago

Leak nsGlobalWindow with unknown-content-type dialog


(Toolkit :: Downloads API, defect)

Not set



Tracking Status
blocking2.0 --- final+


(Reporter: jruderman, Assigned: peterv)


(Blocks 1 open bug)


(Keywords: memory-leak, testcase)


(2 files, 1 obsolete file)

Attached file 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.
It doesn't matter whether I click save, click cancel, or ignore the diaog and quit with it still up.  It leaks every way.
blocking2.0: ? → final+
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.
Any idea if this is Mac-only?
Happens on Linux64 too.
Attached patch v1 (obsolete) — Splinter Review
This seems to fix it for me.
Comment on attachment 453850 [details] [diff] [review]

so we are completely unhooking the cycle, right?

Attachment #453850 - Flags: review+
Shouldn't this cycle be detected by the cycle collector, without needing to break it manually in js?
(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.
Attached patch v2Splinter Review
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.
Assignee: nobody → peterv
Attachment #453850 - Attachment is obsolete: true
Attachment #455265 - Flags: review?(sdwilsh)
Comment on attachment 455265 [details] [diff] [review]

You just moved code so I'm not going to nit pick style.  r=sdwilsh
Attachment #455265 - Flags: review?(sdwilsh) → review+
Keywords: checkin-needed
I landed this for you:
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b3
You need to log in before you can comment on or make changes to this bug.