leak with contentsink-parser-dtd cycle

RESOLVED FIXED in mozilla1.9alpha1



14 years ago
13 years ago


(Reporter: dbaron, Assigned: mrbkap)



Dependency tree / graph
Bug Flags:
blocking1.8.1 -
blocking1.8.0.4 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)



(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.
Note that this is simplified from some ad-inside-iframe code, which could be rather common.
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Posted patch Fix the leak — — Splinter 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().
Last Resolved: 14 years ago
Resolution: --- → FIXED
Blocks: mlk1.8


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 "" 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

13 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

13 years ago
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.