Last Comment Bug 654106 - Setting innerHTML leaks an observer until the page is closed
: Setting innerHTML leaks an observer until the page is closed
Status: VERIFIED FIXED
: mlk, testcase
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: unspecified
: All All
: -- major (vote)
: mozilla6
Assigned To: Henri Sivonen (:hsivonen)
:
Mentors:
: 643940 (view as bug list)
Depends on:
Blocks: 653817 mlk-fx4
  Show dependency treegraph
 
Reported: 2011-05-02 07:39 PDT by Hugh Nougher [:Hughman]
Modified: 2011-07-29 02:41 PDT (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed


Attachments
Simplest testcase that still easily identifies the problem (227 bytes, text/html)
2011-05-02 07:41 PDT, Hugh Nougher [:Hughman]
no flags Details
Less minimal testcase to show its not just document.body or empty string (336 bytes, text/html)
2011-05-02 08:00 PDT, Hugh Nougher [:Hughman]
no flags Details
Massif output (95.12 KB, application/x-bzip)
2011-05-02 08:57 PDT, Mike Hommey [:glandium]
no flags Details
Hackeypatch to test that theory (6.25 KB, patch)
2011-05-02 11:06 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Move the fragment mode flag upwards, avoid running some nsContentSink initialization steps if it is set (6.29 KB, patch)
2011-05-03 05:15 PDT, Henri Sivonen (:hsivonen)
bzbarsky: review+
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Review

Description Hugh Nougher [:Hughman] 2011-05-02 07:39:06 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

This could be a cause for many other bugs in bug 640452.

In my findings, setting innerHTML on an element will extra memory to be used every time something is set to it. Even setting it to an empty string causes memory problems.

Using the almost minimal test case(s) that I will attach, I can get Firefox 4.0.1, Aurora 5.0a2 2011-05-01 and Nightly 6.0a1 2011-05-01 to use over 1GB of RAM in around 3 minutes. This memory will not be freed until the page is refreshed or tab closed. I can even navigate other pages in the same tab without a memory free occurring.

I have had javascript.options.mem.log enabled while testing and CC/GC have definitely been running without freeing the memory.

Reproducible: Always

Steps to Reproduce:
1. Start some version of Firefox in a new profile.
2. Run test case (note that the test case does not actually make anything appear to change).
3. Monitor the memory usage using whatever method you can think of (about:memory, task manager, etc).
4. Click on page when you wish for the memory usage to stop rising (this is an event listener I placed in there to see if the memory cleared when left alone, does not seem to affect results).



I am wondering if this is used in some addons and if so, is it causing the same leak? Since they likely would never close/refresh without the browser closing it could cause problems without even loading a web page.
Comment 1 Hugh Nougher [:Hughman] 2011-05-02 07:41:45 PDT
Created attachment 529472 [details]
Simplest testcase that still easily identifies the problem
Comment 2 Ed Morley [:emorley] 2011-05-02 07:53:22 PDT
[Changed platform to x86; since WOW64 in user agent indicates 32bit build of Firefox (platform refers to browser build, not OS)].
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-02 07:59:45 PDT
OK, I can _definitely_ reproduce on the attached testcase.

Henri, any idea what's going on here?

roc, do we have a good way of dumping out the heap to see where the memory is used?
Comment 4 Hugh Nougher [:Hughman] 2011-05-02 08:00:57 PDT
Created attachment 529477 [details]
Less minimal testcase to show its not just document.body or empty string

Ed: I will remember that for next time.
Comment 5 Hugh Nougher [:Hughman] 2011-05-02 08:02:41 PDT
Comment on attachment 529477 [details]
Less minimal testcase to show its not just document.body or empty string

That select box for text/html when adding an attachment keeps setting it as text/plain...
Comment 6 Henri Sivonen (:hsivonen) 2011-05-02 08:37:48 PDT
(In reply to comment #0)
> This memory will not be freed until the page is
> refreshed or tab closed. I can even navigate other pages in the same tab
> without a memory free occurring.

Sure sounds like the cached fragment parsing holding onto something that it shouldn't hold onto.

(In reply to comment #3)
> Henri, any idea what's going on here?

Not right away, no.

http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5Parser.cpp#553 is supposed to drop what there is to drop at the end of each fragment parse.
Comment 7 Mike Hommey [:glandium] 2011-05-02 08:57:01 PDT
Created attachment 529488 [details]
Massif output

This is a massif output I got from running iceweasel 4.0 (basically, the same as firefox 4.0) with a more or less fresh profile, starting directly on the less minimal testcase.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-02 09:19:23 PDT
OK, well, the first thing that shows is that nsHtml5Parser::ParseHtml5Fragment calls nsContentSink::Init which calls nsScriptLoader::AddObserver (passing a shim that makes sure that the content sink is not holding a strong ref to the sink).

There is no corresponding RemoveObserver call, as far as I can see.  So the script loader's observer array keeps growing at one word per innerHTML set (modulo whatever reallocation algorithm that code actually uses).

The page does 0xFFF sets per interval firing; that's every 10ms in Fx4 and every 4ms on trunk if the processor is keeping up.  So we should expect about 3.3MB/s growth on 64-bit in Fx4 and about 9MB on trunk.  That actually matches my numbers pretty closely.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-02 09:19:55 PDT
Henri, the fragment parser doesn't even need to be a script loader observer, right?
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-02 11:06:11 PDT
Created attachment 529520 [details] [diff] [review]
Hackeypatch to test that theory

With this change, memory usage on the testcase in this bug is stable for me.
Comment 11 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-02 15:46:04 PDT
/me is watching this bug with *intense* interest :)
Comment 12 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-02 15:49:12 PDT
Hughmann:  BTW, thanks for a *wonderful* bug report and test case.  We get so many vague leak reports -- "I browse for a while, close all my tabs, and Firefox still holds onto lots of memory" -- that specific reports like this are extremely valuable.
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-02 16:20:31 PDT
Yeah, I meant to say that, actually.  This is by far the best "memory leaks" report I've seen, complete with steps to reproduce and a characterization of how long the memory leaks for.  This made hunting this down much much easier!
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-05-02 16:35:37 PDT
FWIW this is good stuff but I don't think it explains bugs like 653817, where memory usage persists after closing all tabs that contained Web apps.
Comment 15 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-02 17:37:24 PDT
Indeed.  There's not a "the leak"...
Comment 16 Hugh Nougher [:Hughman] 2011-05-02 17:47:18 PDT
I was actually trying to make a memory rise/leak around bug 650350 comment 10
at the time without much success before finding that the line I was using for
clearing the page was eating memory... not helpful for that test.

I have just run the test in Firefox 3.6.14 where it does not cause any memory
rise. So its a regression of some sort for 4.0.
Comment 17 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-02 17:53:59 PDT
Yeah, this is specific to the HTML5 parser, which first shipped in 4.0.
Comment 18 Henri Sivonen (:hsivonen) 2011-05-03 05:15:32 PDT
Created attachment 529685 [details] [diff] [review]
Move the fragment mode flag upwards, avoid running some nsContentSink initialization steps if it is set

(In reply to comment #9)
> Henri, the fragment parser doesn't even need to be a script loader observer,
> right?

Right, AFAICT.

(In reply to comment #12)
> Hughmann:  BTW, thanks for a *wonderful* bug report and test case.

Indeed. Very useful. Thank you.
Comment 19 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-03 08:19:17 PDT
Comment on attachment 529685 [details] [diff] [review]
Move the fragment mode flag upwards, avoid running some nsContentSink initialization steps if it is set

>+  mCanInterruptParser = mFragmentMode ? PR_FALSE : sCanInterruptParser;

I'd prefer:

  mCanInterruptParser = !mFragmentMode && sCanInterruptParser;

r=me with that.
Comment 20 Johnathan Nightingale [:johnath] 2011-05-03 14:48:40 PDT
This isn't FF5-specific, so minusing for tracking-firefox5, but once this lands and is happy on central, would be nice to see an approval nom for aurora
Comment 21 Henri Sivonen (:hsivonen) 2011-05-03 23:39:12 PDT
(In reply to comment #19)
> I'd prefer:
> 
>   mCanInterruptParser = !mFragmentMode && sCanInterruptParser;
> 
> r=me with that.

Thanks. Landed with the change:
http://hg.mozilla.org/mozilla-central/rev/c3c4c902e9cd
Comment 22 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-03 23:47:34 PDT
Great work Hughmann, Henri and bz!
Comment 23 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-05-04 08:58:27 PDT
Comment on attachment 529685 [details] [diff] [review]
Move the fragment mode flag upwards, avoid running some nsContentSink initialization steps if it is set

I think we'll want this for 5.
Comment 24 Hugh Nougher [:Hughman] 2011-05-04 17:31:24 PDT
Should the mozilla-2.0 branch also be patched?
If there is going to be a security update for 4.0 before 5.0 then I think it would be a good idea.
Comment 25 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-05-04 17:48:43 PDT
Barring a zero-day there are no more security updates planned before 5.0.
Comment 26 Henri Sivonen (:hsivonen) 2011-05-09 04:07:28 PDT
Landed in Aurora:
http://hg.mozilla.org/releases/mozilla-aurora/rev/e56b37ce413b
Comment 27 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-26 16:45:18 PDT
*** Bug 643940 has been marked as a duplicate of this bug. ***
Comment 28 George Carstoiu 2011-07-28 08:09:20 PDT
Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

I've tried to verify whether this issue was fixed using the following steps:
 1. Opened the attached testcase: "Simplest testcase that still...."
 2. Opened a task manager and start observing memory and CPU consumption

What happened is that memory didn't increase. Is this the intended behavior for this patch?

Thanks!
Comment 29 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-07-28 16:19:39 PDT
George: yes.  If you try Firefox 4 you should see the memory usage increase.
Comment 30 George Carstoiu 2011-07-29 02:41:03 PDT
(In reply to comment #29)
> George: yes.  If you try Firefox 4 you should see the memory usage increase.

Great! Considering your comment and the fact that this is no longer reproducible on Firefox 6.0b3 I am setting the status to Verified Fixed.

Thanks!

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