283564 - possible memory leak with innerHTML

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Radek 'sysKin' Czyz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050224 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050224 Firefox/1.0+

Whenever JS resets the contents of innerHTML assinging an empty string to it,
and then assigning some new string, memory usage grows.

Reproducible: Always

Steps to Reproduce:
1. Open task manager to see memory usage
2. Open the attached testcase
3. Wait a bit for the script to loop, ignore possible warning about the script
looping forever. It's a small testcase so I made it run 50,000 times for
400..500mb usage.

Actual Results:  
Memory meter draws a nice linear climb in mem usage.

Expected Results:  
No extra memory used when the loop loops.

I found it on a real webpage, where innerhtml was used to build a menu system.
The innerHTML part was much larger there (several <img> tags on each call) so it
is easy to use 200mb memory just by using the menu for a couple of minutes.

Please note that if I didn't set innerHTML=''; on the first line, memory would
not grow.

Memory does not go down immidielty after the window/tab closes. It might go down
if you minimize the window, but even then, even invoking context menu makes
memory grow to 100mb again. In other words, firefox will never be the same until
proper restart.

Comment 1

[Radek 'sysKin' Czyz]

Attachment: test case

loops 50k times and resets and adds innerHTML each time. memory grows.

Comment 2

[Tuukka Tolvanen (sp3000)]

linux: confirmed firefox trunk 2005-02-24-18Z, memory usage grew to >400M on
testcase. seamonkey 1.8a6 doesn't have the problem.

Comment 3

[Tuukka Tolvanen (sp3000)]

not seeing the issue with a 2005-02-18 trunk seamonkey either.

Comment 4

[Martijn Wargers (dead)]

I'm not seeing the memory leak with 2005-02-18 trunk Firefox build.
But I'm seeing it with 2005-02-19 trunk Firefox build.
So I guess this could be a regression from bug 263053.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Does this memory leak also occur when using createContextualFragment in XML
(i.e., using innerHTML in an XHTML document)?

Comment 6

[Martijn Wargers (dead)]

Attachment: Same testcase but now in xml mode

No, in that case I don't think I see a memory leak.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

I should read more carefully: the |.innerHTML = ''| was precisely the culprit.
When nsParser::ParseFragment() called nsParser::Parse(), the incoming string
was empty, and the aLastCall parameter was |PR_TRUE|, resulting in an immediate
return from the function. This meant that the fragment sink's DidBuildModel()
function was not called, and the parser-sink combo leaked. The XML side of this
does not have this problem, as its last call is always guarenteed to have at
least the document element's end tag in it, so DidBuildModel() will always be
called.

Now, I release the parser when we stop using it (after DidBuildContent), so the
parser no longer leaks.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175601 [details] [diff] [review]
patch v1

r+sr=bzbarsky, but document on the fragment sink api that once you've parsed
one fragment the sink will not work unless you reset it as the sink on the
parser (which is pretty backwards, but there we are).

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 10

[Radek 'sysKin' Czyz]

Confirmed fixed with the testcase - and now guess what, the menu system I was
talking about is leaking just like before ;) Expect a new bug when I create
another testcase. Heh ;-)