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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
P2
normal
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: mrbkap)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha1
x86
Windows XP
fixed1.8.0.4, fixed1.8.1, mlk, testcase
Points:
---
Bug Flags:
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(2 attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Comment 1

12 years ago
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('');
(Reporter)

Comment 2

12 years ago
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

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

Updated

12 years ago
Summary: Memory leak when visiting www.economist.com → Memory leak if a site sets location and then document.writes (e.g. when visiting www.economist.com)
(Assignee)

Comment 4

12 years ago
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?).
(Assignee)

Comment 5

12 years ago
Apparently, parser contexts keep strong refs to things that matter. Who knew?
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 6

12 years ago
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #210308 - Flags: superreview?(jst)
Attachment #210308 - Flags: review?(bugmail)
Comment on attachment 210308 [details] [diff] [review]
Fix

sr=jst
Attachment #210308 - Flags: superreview?(jst) → superreview+
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
Attachment #210308 - Flags: review?(bugmail) → review+
(Assignee)

Comment 9

12 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 10

12 years ago
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

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

Comment 12

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

Comment 13

12 years ago
I don't see a leak on www.nasa.gov using 2006-02-03 trunk.
(Reporter)

Comment 14

12 years ago
(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.
Status: RESOLVED → VERIFIED

Updated

12 years ago
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3? → blocking1.8.0.3+
(Assignee)

Comment 15

12 years ago
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.
Attachment #210308 - Flags: approval1.8.0.3?

Comment 16

12 years ago
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!
(Assignee)

Updated

12 years ago
Attachment #210308 - Flags: approval-branch-1.8.1?(jst)

Updated

12 years ago
Attachment #210308 - Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
(Assignee)

Comment 17

12 years ago
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1
Comment on attachment 210308 [details] [diff] [review]
Fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #210308 - Flags: approval1.8.0.3? → approval1.8.0.3+
(Assignee)

Comment 19

12 years ago
Fix checked into the 1.8.0 branch.
Keywords: fixed1.8.0.3
You need to log in before you can comment on or make changes to this bug.