Closed
Bug 315951
Opened 19 years ago
Closed 19 years ago
unknown content type dialog leaks domwindow
Categories
(Firefox :: General, defect, P1)
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)
7.89 KB,
patch
|
mconnor
:
review+
mconnor
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•19 years ago
|
Assignee | ||
Comment 1•19 years ago
|
||
Attachment #202589 -
Flags: review?(beng)
Assignee | ||
Updated•19 years ago
|
Whiteboard: [patch]
Assignee | ||
Updated•19 years ago
|
Severity: normal → major
Comment 2•19 years ago
|
||
Does this patch still do the right thing if you cancel out of the dialog before the timer fires?
Assignee | ||
Comment 3•19 years ago
|
||
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.
Assignee | ||
Comment 4•19 years ago
|
||
...or just moving my new line to the start of the function.
Assignee | ||
Comment 5•19 years ago
|
||
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)
Assignee | ||
Comment 6•19 years ago
|
||
(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.
Assignee | ||
Comment 7•19 years ago
|
||
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.
Assignee | ||
Comment 8•19 years ago
|
||
(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.
Assignee | ||
Updated•19 years ago
|
Attachment #202726 -
Flags: review?(mconnor)
Assignee | ||
Comment 9•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Attachment #202726 -
Attachment is obsolete: true
Assignee | ||
Comment 10•19 years ago
|
||
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
Assignee | ||
Comment 11•19 years ago
|
||
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.
Assignee | ||
Updated•19 years ago
|
Flags: blocking1.8.1?
Updated•19 years ago
|
Attachment #202844 -
Flags: review?(mconnor) → review+
Assignee | ||
Comment 12•19 years ago
|
||
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
Comment 13•18 years ago
|
||
This wasn't checked in on the branch - David, is there any reason it shouldn't be?
Updated•18 years ago
|
Flags: blocking1.8.0.2?
Assignee | ||
Updated•18 years ago
|
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)
Updated•18 years ago
|
Flags: blocking1.8.0.2? → blocking1.8.0.2-
Updated•18 years ago
|
Attachment #202844 -
Flags: approval-branch-1.8.1?(mconnor) → approval-branch-1.8.1+
Updated•18 years ago
|
Flags: blocking1.8.0.3?
Updated•18 years ago
|
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Assignee | ||
Updated•18 years ago
|
Attachment #202844 -
Flags: approval1.8.0.3?
Comment 15•18 years ago
|
||
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+
You need to log in
before you can comment on or make changes to this bug.
Description
•