Last Comment Bug 323441 - Memory leak if a site sets location and then document.writes (e.g. when visiting www.economist.com)
: Memory leak if a site sets location and then document.writes (e.g. when visit...
Status: VERIFIED FIXED
[patch]
: fixed1.8.0.4, fixed1.8.1, mlk, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Windows XP
: P2 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
: Hixie (not reading bugmail)
Mentors:
http://www.economist.com
: 324649 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-14 11:29 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2006-04-21 13:57 PDT (History)
9 users (show)
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (240 bytes, text/html)
2006-01-14 11:31 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Fix (2.20 KB, patch)
2006-01-31 17:20 PST, Blake Kaplan (:mrbkap)
jonas: review+
jst: superreview+
jst: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2006-01-14 11:29:19 PST
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 [:mwargers] (not working for Mozilla) 2006-01-14 11:31:52 PST
Created attachment 208489 [details]
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 [:mwargers] (not working for Mozilla) 2006-01-14 11:36:12 PST
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:] 2006-01-29 20:57:43 PST
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) 2006-01-30 18:19:34 PST
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) 2006-01-31 17:17:40 PST
Apparently, parser contexts keep strong refs to things that matter. Who knew?
Comment 6 Blake Kaplan (:mrbkap) 2006-01-31 17:20:33 PST
Created attachment 210308 [details] [diff] [review]
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, jst@mozilla.com) 2006-01-31 17:27:57 PST
Comment on attachment 210308 [details] [diff] [review]
Fix

sr=jst
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-01-31 18:02:05 PST
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) 2006-02-01 10:59:40 PST
Fix checked into trunk.
Comment 10 Jesse Ruderman 2006-02-02 22:03:07 PST
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 2006-02-03 18:22:23 PST
Martijn, did this patch fix the original problem with economist.com?
Comment 12 Jesse Ruderman 2006-02-03 21:08:56 PST
*** Bug 324649 has been marked as a duplicate of this bug. ***
Comment 13 Bob Clary [:bc:] 2006-02-03 22:34:59 PST
I don't see a leak on www.nasa.gov using 2006-02-03 trunk.
Comment 14 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-02-05 13:23:55 PST
(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) 2006-04-05 13:26:04 PDT
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 2006-04-10 12:22:19 PDT
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) 2006-04-11 13:23:26 PDT
Fix checked into the 1.8 branch.
Comment 18 Daniel Veditz [:dveditz] 2006-04-19 12:08:13 PDT
Comment on attachment 210308 [details] [diff] [review]
Fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 19 Blake Kaplan (:mrbkap) 2006-04-21 13:57:33 PDT
Fix checked into the 1.8.0 branch.

Note You need to log in before you can comment on or make changes to this bug.