545302 - High memory usage and crashes caused by getElementsByClassName()

Status: NEW

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

Description

[Andreas Dopfer]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a2pre) Gecko/20100209 Minefield/3.7a2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a2pre) Gecko/20100209 Minefield/3.7a2pre

Repetitively creating large node lists with document.getElementsByClassName()
from an unchanging set of DOM elements results in high memory usage, 
though only if some of the nodelist's attributes or elements are being accessed in each iteration, e.g.:

function foo() {
	var nodeList = document.getElementsByClassName("someClass");
	 var a = nodeList.length;  
	 var b = nodeList[nodeList.length-1];
	 setTimeout("foo()", 1);
}

Reproducible: Always

Steps to Reproduce:
1. Open the attached test case in Firefox
2. press "Start" button 
3. watch memory usage
Actual Results:  
Strong increase of memory usage, unresponsive UI; may also abort (throwing std::bad_alloc)

Expected Results:  
No significant increase of memory usage

Also tested on:
* Kubuntu 9.04: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.7) Gecko/20100106 Ubuntu/9.04 (jaunty) Shiretoko/3.5.7 
* Kubuntu 9.04: Mozilla XULRunner 1.9.1.4 - 20091028075319 
* Windows XP SP2: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.14) Gecko/2009082707 Firefox/3.0.14 (.NET CLR 3.5.30729)

Comment 1

[Andreas Dopfer]

Attachment: Testcase for Bug 545302 (High memory usage and potential crash)

Comment 2

[Andreas Dopfer]

The problem seems to be fixed in http://hg.mozilla.org/mozilla-central/rev/bb7f5fa01db7. The last revision I tried before filing the bug was http://hg.mozilla.org/mozilla-central/log?rev=e6967149838a (which is a couple of hours older).

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks for the very well-written bug report; as it seems to be fixed by
bug 453929, I'll mark this one as a duplicate. If you disagree, please do tell me.

Comment 4

[Boris Zbarsky [:bzbarsky]]

No, this isn't a duplicate.  For example, just changing the testcase to use a different class name list (all including "testclass") every time would bring the problem back.

The real issue is that getting the length of the nodelist makes it hold pointers to all the divs in the list (and in this case take up at least 2000 words worth of space; that's 8KB on a 32-bit machine).  The testcase creates 100 nodelists per timeout, and runs the timeouts every 10ms.  So it's trying to allocate memory at about 80MB a second.

Now the testcase could just force us to allocate that much memory by holding refs to the lists, but it's not doing that.  So we should ideally be gcing the lists... but I bet JS doesn't realize how much memory they take up and so doesn't realize how much heap is _really_ in use.

So things that should probably happen:

1)  Memory pressure notifications should actually happen
2)  Memory pressure notifications should cause nsContentLists
    to go into the dirty state (dropping their allocated vector).
2)  Memory pressure notifications should trigger gc.

Anything else?  I don't see a sane way to treat the allocated vector sizes as part of the gc heap for "is it time to gc" purposes....

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and I would _love_ to know what code throws bad_alloc and why.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Reopening per all that.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Anecdotal and somewhat off topic, but I hit

http://crash-stats.mozilla.com/report/index/bp-fd7d3ffb-97ed-41e1-89e3-048612100208

the other day on winxp, while loading a full tinderbox log.  I didn't bother filing because it's covered by existing bugs (some on my TODO list).

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #4)
>  I don't see a sane way to treat the allocated vector sizes as
> part of the gc heap for "is it time to gc" purposes....
A not-so-sane way would be to call updateMallocCounter whenever adding new
elements to the vector.

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> (In reply to comment #4)
> >  I don't see a sane way to treat the allocated vector sizes as
> > part of the gc heap for "is it time to gc" purposes....
> A not-so-sane way would be to call updateMallocCounter whenever adding new
> elements to the vector.

This is quite sane an idea, AFAICT. Igor and Gregor should comment.

/be

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> > A not-so-sane way would be to call updateMallocCounter whenever adding new
> > elements to the vector.
> 
> This is quite sane an idea

I second that. We have already fixed similar bugs like bug 516458.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, one problem is that there's no obvious cx to use for the call...

Comment 12

[Andreas Dopfer]

(In reply to comment #5)
> Oh, and I would _love_ to know what code throws bad_alloc and why.

Here you go!

[Thread 0xad27bb90 (LWP 31452) exited]
terminate called after throwing an instance of 'std::bad_alloc'
  what(): std::bad_alloc

Program received signal SIGABRT, Aborted.
[Switching to Thread 0xb4dd5760 (LWP 31438)]
0xb7f9c430 in __kernel_vsyscall ()
(gdb) bt
#0  0xb7f9c430 in __kernel_vsyscall ()
#1  0xb545d6d0 in raise () from /lib/tls/i686/cmov/libc.so.6
#2  0xb545f098 in abort () from /lib/tls/i686/cmov/libc.so.6
#3  0xb56608f8 in __gnu_cxx::__verbose_terminate_handler () from /usr/lib/libstdc++.so.6
#4  0xb565e7d5 in ?? () from /usr/lib/libstdc++.so.6
#5  0xb565e812 in std::terminate () from /usr/lib/libstdc++.so.6
#6  0xb565e94a in __cxa_throw () from /usr/lib/libstdc++.so.6
#7  0xb565efa3 in operator new () from /usr/lib/libstdc++.so.6
#8  0xb788e05c in EdgePool::Builder::Add (this=0xbfdb35e0, aEdge=0x3e8a9274)
    at /home/andreas/Projects/MozillaTrunk/src_bug_rev/src/xpcom/base/nsCycleCollector.cpp:407
#9  0xb788c3d1 in GCGraphBuilder::NoteXPCOMChild (this=0xbfdb35cc, child=0xaa9fba30)
    at /home/andreas/Projects/MozillaTrunk/src_bug_rev/src/xpcom/base/nsCycleCollector.cpp:1560
#10 0xb6a9c887 in nsBaseContentList::cycleCollection::Traverse (this=0xb7f589a0, p=0xaab0f800, cb=@0xbfdb35cc)
    at /home/andreas/Projects/MozillaTrunk/src_bug_rev/src/content/base/src/nsContentList.cpp:82
#11 0xb788cc33 in GCGraphBuilder::Traverse (this=0xbfdb35cc, aPtrInfo=0x3e84b23c)
    at /home/andreas/Projects/MozillaTrunk/src_bug_rev/src/xpcom/base/nsCycleCollector.cpp:1475
#12 0xb788ccd0 in nsCycleCollector::MarkRoots (this=0xb4b32000, builder=@0xbfdb35cc)
    at /home/andreas/Projects/MozillaTrunk/src_bug_rev/src/xpcom/base/nsCycleCollector.cpp:1697
#13 0xb788d158 in nsCycleCollector::BeginCollection (this=0xb4b32000)
    at /home/andreas/Projects/MozillaTrunk/src_bug_rev/src/xpcom/base/nsCycleCollector.cpp:2641
[... see attachment]

Built from:
andreas@linux01:~/Projects/MozillaTrunk/src_bug_rev/src$ hg parent
changeset:   37995:e6967149838a
user:        Jeff Muizelaar <[e-mail]>
date:        Tue Feb 09 00:07:00 2010 -0500
summary:     Revert 3f91dcac71c4 for breaking 'make check'.

Built with:
andreas@linux01:~/Projects/MozillaTrunk/src_bug_rev/src$ cat .mozconfig
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@
mk_add_options MOZ_BUILD_PROJECTS="browser"
mk_add_options MOZ_CO_PROJECT="browser"
ac_add_app_options browser --enable-application=browser
ac_add_options --disable-javaxpcom
ac_add_options --disable-optimize
ac_add_options --enable-debug
ac_add_options --enable-tests

Comment 13

[Andreas Dopfer]

Attachment: Full stack trace for std::bad_alloc

Comment 14

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.