leak with contentsink-parser-dtd cycle

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
HTML: Parser
P2
normal
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: dbaron, Assigned: mrbkap)

Tracking

({memory-leak})

Trunk
mozilla1.9alpha1
x86
Linux
memory-leak
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 -
blocking1.8.0.4 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

The attached testcase causes a memory leak with the contentsink-parser-dtd cycle.  Testcase is reduced from a webpage.  The throbber also never stops spinning.

I wish we'd have fewer strong pointers so we wouldn't need to break cycles and these things wouldn't leak.
Created attachment 205018 [details]
script for testcase
Note that this is simplified from some ad-inside-iframe code, which could be rather common.
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 4

13 years ago
Created attachment 205089 [details] [diff] [review]
Fix the leak

This patch fixes the leak (and the infinite load bit), but it doesn't quite do the right thing for the case where the script tries to write out stuff before or after the script (IE seems to ignore the document.close call, or at least wait until after the entire document.write has happened until it does actually close the document). The answer to this might be to make mWriteLevel work correctly, but I'd rather tackle that in another bug.
Attachment #205089 - Flags: superreview?(jst)
Attachment #205089 - Flags: review?(jst)
Comment on attachment 205089 [details] [diff] [review]
Fix the leak

r+sr=jst
Attachment #205089 - Flags: superreview?(jst)
Attachment #205089 - Flags: superreview+
Attachment #205089 - Flags: review?(jst)
Attachment #205089 - Flags: review+
(Assignee)

Comment 6

13 years ago
Fix checked in. I filed bug 319713 on the problem with using Terminate().
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Blocks: 320915

Updated

13 years ago
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3? → blocking1.8.0.3+
leak fixes are nice and this is a very small patch to fix a very large leak (when triggered, which might be rare), but then it requires the rather large regression fix in bug 321781. We probably don't want it on the 1.8.0 branch that much: flipping back to "1.8.0.3-nominated" for another triage pass.

Would be nice in 1.8.1, but then we'd need a different patch for the regression since it changes the nsIHTMLDocument and nsIParser interfaces :-(
Flags: blocking1.8.1?
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3+

Comment 8

13 years ago
Given that bug 321781 requires modifications to nsIHTMLDocument, and this patch requires that patch, we cannot take this on the 1.8.0 branch.  Please re-nominate the bug if there is a solution that does not require API changes.
Flags: blocking1.8.0.3? → blocking1.8.0.3-

Comment 9

12 years ago
Would really like to get some patch that is suitable for the 1.8.1 release.
Flags: blocking1.8.1? → blocking1.8.1+

Comment 10

12 years ago
BKap - any chance you'll have time for a branch patch?
(Assignee)

Comment 11

12 years ago
(In reply to comment #10)
> BKap - any chance you'll have time for a branch patch?

So, to be clear, this patch caused a bunch of regressions and is generally not what we want to put on any branches. The patch over in bug 321781 contains the fix that we'd 'want' to take in favor of this patch, but I'm not sure if you want that on the branch or not.
Minusing for blocking1.8.1 because the fix seems too scary to take at this point, despite that we do want this.  Ah well.
Flags: blocking1.8.1+ → blocking1.8.1-
You need to log in before you can comment on or make changes to this bug.