319123 - leak with contentsink-parser-dtd cycle

Status: RESOLVED FIXED

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

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: script for testcase

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that this is simplified from some ad-inside-iframe code, which could be rather common.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 205089 [details] [diff] [review]
Fix the leak

r+sr=jst

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in. I filed bug 319713 on the problem with using Terminate().

Comment 7

[Daniel Veditz [:dveditz]]

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 :-(

Comment 8

[Darin Fisher]

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.

Comment 9

[Darin Fisher]

Would really like to get some patch that is suitable for the 1.8.1 release.

Comment 10

[Mike Schroepfer]

BKap - any chance you'll have time for a branch patch?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

(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.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Minusing for blocking1.8.1 because the fix seems too scary to take at this point, despite that we do want this.  Ah well.