Closed Bug 319123 Opened 15 years ago Closed 15 years ago

leak with contentsink-parser-dtd cycle


(Core :: DOM: HTML Parser, defect, P2)






(Reporter: dbaron, Assigned: mrbkap)



(Keywords: memory-leak)


(3 files)

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.
Note that this is simplified from some ad-inside-iframe code, which could be rather common.
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Attached patch Fix the leakSplinter Review
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

Attachment #205089 - Flags: superreview?(jst)
Attachment #205089 - Flags: superreview+
Attachment #205089 - Flags: review?(jst)
Attachment #205089 - Flags: review+
Fix checked in. I filed bug 319713 on the problem with using Terminate().
Closed: 15 years ago
Resolution: --- → FIXED
Blocks: mlk1.8
Depends on: 321781
Depends on: 321782
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 "" 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+
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-
Would really like to get some patch that is suitable for the 1.8.1 release.
Flags: blocking1.8.1? → blocking1.8.1+
BKap - any chance you'll have time for a branch patch?
(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-
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.