Closed Bug 315951 Opened 19 years ago Closed 19 years ago

unknown content type dialog leaks domwindow

Categories

(Firefox :: General, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
Firefox 2

People

(Reporter: dbaron, Assigned: dbaron)

References

()

Details

(Keywords: fixed1.8.0.4, fixed1.8.1, memory-leak, Whiteboard: [patch])

Attachments

(1 file, 2 obsolete files)

The unknown content type dialog leaks a dom window.

Steps to reproduce:
 1. go to http://www.mozilla.org/projects/firefox/
 2. click "Download release candidate"
 3. click cancel in the dialog that opens

The source of the leak seems to be a wrapped JS object.
Status: NEW → ASSIGNED
Keywords: mlk
Priority: -- → P1
Target Milestone: --- → Firefox1.6-
Attached patch patch (obsolete) — Splinter Review
Attachment #202589 - Flags: review?(beng)
Severity: normal → major
Does this patch still do the right thing if you cancel out of the dialog before the timer fires?
Oh, probably not; I should probably put the first line of the notify method in a try-block so that the rest will still run if the document has been destroyed.
...or just moving my new line to the start of the function.
Attached patch patch (obsolete) — Splinter Review
Actually, I think try/catch is better since we don't want the exception to propagate.  (Although maybe there's some test better than try/catch.)
Attachment #202589 - Attachment is obsolete: true
Attachment #202726 - Flags: review?(mconnor)
Attachment #202589 - Flags: review?(beng)
(In reply to comment #0)
>  3. click cancel in the dialog that opens

Note that this presumes you haven't at some point in the past chosen to always save that type of file.  If you have, then you get the save dialog directly.
I'd like to test that these actually fix leaks too -- this is based only on a code audit of JS timer users.  The third file I'll need to test on mac; the fourth file I need to figure out how to test.
(In reply to comment #7)
> I'd like to test that these actually fix leaks too -- this is based only on a
> code audit of JS timer users.  The third file I'll need to test on mac; the
> fourth file I need to figure out how to test.

Actually, there's a pref for the Mac-style pref window animation.  (It's really slow on Linux!)  I tested that it leaks (a lot, since it looks like the leak entrains the document) without the patch and doesn't leak with the patch, and behaves pretty much the same either way.
Comment on attachment 202844 [details] [diff] [review]
patch fixing additional leaks (ignore last file in diff)

I couldn't figure out how to get the update wizard for extension update.  I'll skip that part of the patch unless I can test it somehow...
Attachment #202844 - Flags: review?(mconnor)
I think triggering the wizard requires an incompatible extension on startup; I should be able to test that using https://addons.mozilla.org/extensions/moreinfo.php?application=firefox&id=377&vid=1545
FWIW (I forgot to note it here for some reason), things still worked, but it seemed to increase leaks rather than decrease them, so I think I'll still skip the last part of the patch, at least for now.
Flags: blocking1.8.1?
Attachment #202844 - Flags: review?(mconnor) → review+
Checked in to trunk.

I filed bug 321667 on the 4th file in the patch, which I didn't check in.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
This wasn't checked in on the branch - David, is there any reason it shouldn't be?
Flags: blocking1.8.0.2?
Attachment #202844 - Attachment description: patch fixing additional leaks → patch fixing additional leaks (ignore last file in diff)
Attachment #202844 - Flags: branch-1.8.1?(mconnor)
Flags: blocking1.8.0.2? → blocking1.8.0.2-
Attachment #202844 - Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
Checked in to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment on attachment 202844 [details] [diff] [review]
patch fixing additional leaks (ignore last file in diff)

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #202844 - Flags: approval1.8.0.3? → approval1.8.0.3+
Fix checked in to MOZILLA_1_8_0_BRANCH.
Keywords: fixed1.8.0.3
You need to log in before you can comment on or make changes to this bug.