323441 - Memory leak if a site sets location and then document.writes (e.g. when visiting www.economist.com)

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

When visiting http://www.economist.com without cookies yet set or cookies blocked for that site, Mozilla leaks.
I get this as result:
Leaked inner window 1f80c18 (outer 1ef1b78) at address 1f80c18.
 ... with URI "http://www.economist.com/index.html".
Leaked inner window 25b6370 (outer 1ef1b78) at address 25b6370.
 ... with URI "http://www.economist.com/".
Leaked outer window 1ef1b78 at address 1ef1b78.
Leaked document at address 26cca48.
 ... with URI "jar:file:///C:/Documents%20and%20Settings/mw22/Bureaublad/moz/fir
efox20060114/chrome/toolkit.jar!/content/global/bindings/scrollbar.xml".
 ... with URI "chrome://global/content/bindings/scrollbar.xml".
Leaked document at address 1fa8ca0.
 ... with URI "http://www.economist.com/".
Leaked docshell at address 1ef58e0.
 ... which loaded URI "about:blank".
 ... which loaded URI "http://www.economist.com/".
 ... which loaded URI "http://www.economist.com/index.html".
Summary:
Leaked 3 out of 10 DOM Windows
Leaked 2 out of 35 documents
Leaked 1 out of 4 docshells

I've made a testcase that at least could explain for some of the leaks.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

The testcase redirects to google, the redirect is necessary to get the leak, basically the script that causes the leak consists of this:
document.location.href = 'http://google.com';
document.write('');

Comment 2

[Martijn Wargers (dead)]

With the leak-gauge script I get with the testcase:
Leaked document at address 1c70bd0.
 ... with URI "jar:file:///C:/Documents%20and%20Settings/mw22/Bureaublad/moz/fir
efox20060114/chrome/toolkit.jar!/content/global/bindings/scrollbar.xml".
 ... with URI "chrome://global/content/bindings/scrollbar.xml".
Leaked document at address 1f6b078.
 ... with URI "https://bugzilla.mozilla.org/attachment.cgi?id=208489".
Leaked docshell at address 1f6c0b8.
 ... which loaded URI "about:blank".
 ... which loaded URI "http://google.com/".
 ... which loaded URI "https://bugzilla.mozilla.org/attachment.cgi?id=208489".
 ... which loaded URI "https://bugzilla.mozilla.org/show_bug.cgi?id=323441".
 ... which loaded URI "http://www.google.nl/".
Summary:
Leaked 0 out of 13 DOM Windows
Leaked 2 out of 46 documents
Leaked 1 out of 4 docshells

Comment 3

[Bob Clary [:bc] (inactive)]

A similar leak occurs on http://www.nasa.gov where if you don't have flash 6 you will be redirected to http://www.nasa.gov/externalflash/screenreader/noflash.html. In this case a single document leaks, and no windows or docshells leak.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Without any debugging to backup my claim, it looks like we're hitting the parser-contentsink-document cycle (Where oh where is my document.write termination function? Where oh where could it be?).

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Apparently, parser contexts keep strong refs to things that matter. Who knew?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

This patch is defense-in-depth against bugs of this nature:
- The nsParser::Parse(const nsAString&, ...) change makes it so that the parser won't even bother creating new parser contexts after it's already called Terminate() (and thus, DidBuildModel).

- The nsParser::~nsParser() change makes it so that even if we do end up in this weird state at the end of the parser's lifecycle, we are sure to delete all parser contexts.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 210308 [details] [diff] [review]
Fix

sr=jst

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 210308 [details] [diff] [review]
Fix

>Index: parser/htmlparser/src/nsParser.cpp

>+#ifdef DEBUG
>+  if (mParserContext && mParserContext->mPrevContext) {
>+    NS_WARNING("Extra parser contexts still on the parser stack");
>+  }
> #endif

Use NS_WARN_IF

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 10

[Jesse Ruderman]

Verified fixed with a 2006-02-02 afternoon hourly build on Mac.  (I was able to reproduce with a 2006-01-30 build.)

I only tested with the testcase.  I didn't figure out how to reproduce on economist.com and I didn't try nasa.

Comment 11

[Jesse Ruderman]

Martijn, did this patch fix the original problem with economist.com?

Comment 12

[Jesse Ruderman]

*** Bug 324649 has been marked as a duplicate of this bug. ***

Comment 13

[Bob Clary [:bc] (inactive)]

I don't see a leak on www.nasa.gov using 2006-02-03 trunk.

Comment 14

[Martijn Wargers (dead)]

(In reply to comment #11)
> Martijn, did this patch fix the original problem with economist.com?
Yes, I did not see a memory leak at economist.com with the 2006-02-04 trunk build.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 210308 [details] [diff] [review]
Fix

This patch fixes a leak. There have been no known regressions in two months of bake time.

Comment 16

[Darin Fisher]

Please get this in on the MOZILLA_1_8_BRANCH (fixed1.8.1) soon to maximize bake time before we take this for MOZILLA_1_8_0_BRANCH.  Thanks!

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 210308 [details] [diff] [review]
Fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8.0 branch.