Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#10690 closed defect (wontfix)

BorderContainer: Memory Leak in Firefox 3.0

Reported by: Tom Elliott Owned by:
Priority: high Milestone: tbd
Component: Dijit Version: 1.4.0
Keywords: Firefox Memory Leak Cc:
Blocked By: Blocking:

Description

Hey all.

I've spotted a memory leak in Firefox. I can reproduce this on 3.0.17 Ubuntu build (with all manner of addons) and a totally vanilla 3.0.17 on a totally vanilla Windows XP.

I cannot reproduce this on Chrome (latest Ubuntu) or any of Safari 4, Chrome 4, IE6, IE7 or Opera 10 on Windows, using the latest of all of those except IE. I've not tried IE8.

A page with the leak is available here: http://telliott.net/dojoExamples/dojo-ffMemoryLeakExample.html.

I'll attach the source here, along with a screen shot from the XP machine.

Please let me know if this is the fault of the code rather than a problem with dojo/dijit. Also please let me know if you need any further details.

Oh, and I've tested with dojo 1.4 and 1.3.2, both from the google CDN.

Regards,

mrtom

Attachments (9)

dojo-ffMemoryLeakExample.html (2.5 KB) - added by Tom Elliott 9 years ago.
memoryLeak_small.png (90.6 KB) - added by Tom Elliott 9 years ago.
dojo-ffMemoryLeakExample2.html (2.3 KB) - added by Tom Elliott 9 years ago.
dojo-ffMemoryLeakExample2-2.html (2.3 KB) - added by Tom Elliott 9 years ago.
Correct version of 2
dojo-ffMemoryLeakExample3.html (2.8 KB) - added by Tom Elliott 9 years ago.
dojo-ffMemoryLeakExample4.html (2.8 KB) - added by Tom Elliott 9 years ago.
ffLeakNoBCs.tar.gz (118.5 KB) - added by Tom Elliott 9 years ago.
Various HTML and PNG files used for my testing. No border containers
ffLeakWithBCs.tar.gz (194.6 KB) - added by Tom Elliott 9 years ago.
Various HTML and PNG files used for my testing. With border containers
dijits.zip (4.5 KB) - added by Tom Elliott 9 years ago.
Same test with a bunch of different dijits

Download all attachments as: .zip

Change History (25)

Changed 9 years ago by Tom Elliott

Changed 9 years ago by Tom Elliott

Attachment: memoryLeak_small.png added

comment:1 Changed 9 years ago by Tom Elliott

OH, and if you open this in FF it'll eat 200MB every 2s as it stands, so please be careful!

Tom

comment:2 Changed 9 years ago by Tom Elliott

Interestingly I even see a leak, albeit much smaller (because I'm not adding the whopping big object to the button) with this code, which is pretty much an exact copy from dojocampus (see dojo-ffMemoryLeakExample2.html, attached)

Changed 9 years ago by Tom Elliott

Changed 9 years ago by Tom Elliott

Correct version of 2

Changed 9 years ago by Tom Elliott

comment:3 Changed 9 years ago by Tom Elliott

Meh, it seems that I had a bug in dojo-ffMemoryLeakExample2.html. Please ignore and see dojo-ffMemoryLeakExample2_2.html instead. Sorry.

Also, it looks like the leak is substantially reduced - but not entirely removed - if you manually destroy the button via addOnUnload().

My understanding was that the border container should handle that for me? See dojo-ffMemoryLeakExample3.html for an example of this.

Perhaps this is a problem with the destroy code for buttons? I'll try other form widgets and see if I can reproduce there.

Tom

comment:4 Changed 9 years ago by Tom Elliott

Right, final word from me for tonight - it's home time in the UK! dojo-ffMemoryLeakExample4.html. Using a filtering select I get the same issue. So it looks like this isn't related to button nodes per se.

I'll try diving into the code tomorrow, but I'm a bit out of my depth. If anyone has any ideas please shout.

Regards,

Tom

Changed 9 years ago by Tom Elliott

comment:5 Changed 9 years ago by bill

Hmm, from the looks of your testcase it seems like this would reproduce without dojo at all. You are basically creating a giant string and then refreshing the page (and repeat).

It might be related to firebug too.

Can you try:

  • disabling firebug
  • reducing down the testcase even more

For example, if you just create the giant string and store it as a global variable (giantString = ... or window.giantString=...)m, does it still leak? Or also, dijit.giantString=... or dojo.giantString=....?

You are saying that the memory usage keeps going up every time the page reloads, right?

comment:6 Changed 9 years ago by Tom Elliott

Hi Bill.

Thanks for the quick response. To answer your questions:

Yup, basically creating a giant string and adding it to a JS object, then refreshing the page.

Firebug is not an issue. Have recreated this on a vanilla install of Firefox and XP. No plugins at all, no other applications or tabs open. I can see the memory leaking by looking at the memory graphs in the performance tab in the taskbar. Similarly have recreated in ubuntu.

Memory leak keeps going up every time the page loads, yes. See the png I attached.

As per my comment 2, I can see the leak even without the big string (I'll double check that now). The big string just makes the leak much worse, and hence easier to spot.

Either way, my understanding is that the browser will destroy any JS and DOM elements on page unload, unless there is a cyclic dependency between them (as per http://www.ibm.com/developerworks/web/library/wa-memleak/index.html - listing 5). In this case to my knowledge we are using dojo/dijit via the correct APIs and in the correct manner, and I would expect any such circular references to be tidied up by dijit internally. If this was the case you'd expect your memory graph to seesaw - up as the large string is created and down as it is garbage collected on page unload. But we're seeing a steady rise.

It's also worth noting that if I open up a second (empty tab) and let the first (leaky) tab run for a bit, then close the first tab down, the memory consumed by the browser does not fall. i.e. the memory has leaked out of the page/tab completely and can only be reclaimed by a restart of Firefox.

Just to double check. I assume you are *not* expected to manually destroy every widget on page unload??

Just for some further information. Our app has a fairly hefty dojo/dijit based GUI that is not single page, and under typical usage users will refresh or move away from the page once a minute or so over a 6 hour shift. We're seeing massive memory leak issues in IE and FF and are on a bit of a witch hunt to get rid of them all. I would imagine that most modern applications will have a much slower full page reload rate and thus this would be less of an issue, potentially why it hasn't been spotted before?

I'll keep looking into this today and post any further information I find here. I hang out in #dojo with the nick postoffice and am in front of my computer until 19:00ish GMT most work days if you want to discuss this in real time.

Thanks again.

Tom

comment:7 Changed 9 years ago by bill

You are right, the browser is supposed to destroy all JS objects on page unload, without you having to do anything. As you mentioned, usually the problem is on IE6/7 with cyclic dependencies, but that's not an issue on FF, so I'm not sure what's going on.

Dijit has code to automatically destroy widgets on page unload but it only runs on IE. You could try it on FF to see if it helps you. It's called dijit._destroyAll(), in manager.js (that's also where it sets up an addOnUnload() call for IE). Like I said, in theory it's not needed.

We fixed a bunch of leaks in 1.4 using http://thread.gmane.org/gmane.comp.web.dojo.devel/10251/focus=11832, maybe you should run that too and see if anything is reported... although it's for IE, not FF.

What I wanted you to check though was whether it still leaks when dojo/dijit isn't involved, just by creating that giant string and storing it in a global variable (see my comment above).

comment:8 Changed 9 years ago by Tom Elliott

Hi Bill, I think I'm getting to the bottom of this.

Firstly, istr reading somewhere that those cyclic leaks were possible with Firefox as well as IE, which I now suspect is incorrect and was leading me down a blind alley.

I've discovered the following:

Plain DOM: No leak, nice see-saw pattern Dojo, single button in the page: No leak, nice see-saw pattern Dojo, button within a BorderContainer?: Leak! This happens even when there is no onClick/onclick defined on the button in any way. Considering my realisation that the circular dependency issue shouldn't even be possible in FF I've started to think this is a problem with the BC not destroying it's children properly when it's being destroyed.

I patched _base/manager.js to call _destroyAll() on page unload by adding a simple "
isFF" to the IE specific code and that's cleared things up. So for this particular test case I could just patch dojo but I'd rather get to the bottom of the issue. As I said above my current best guess is some sort of clean up bug in the Border Container.

Any thoughts on the hypothesis or where I could start digging? I believe that the recursive destroying of widgets held within layout widgets is handled by _Widget or _LayoutWidget rather than the BorderContainer? itself, is that correct?

Thanks,

Tom

comment:9 Changed 9 years ago by Tom Elliott

Oh, and I've got a whole bunch of tests and screen shots I could attach, but the attachments are getting a bit full of cruft. Let me know if you'd like to review anything and I'll add what you think is important. --Tom

comment:10 Changed 9 years ago by bill

Very interesting. Sure, I'd like to see the Button example rather than the FilteringSelect example (I guess I could modify your example myself though).

I don't think it's a problem with BorderContainer not destroying it's children since you said that _destroyAll() fixes the problem. All _destroyAll() does (for your testcase) is to call BorderContainer.destroyRecursive().

The mystery is why the BorderContainer.destroyRecursive() call is needed on FF. Does a plain Button.destroyRecursive() also fix the leak?

comment:11 Changed 9 years ago by Tom Elliott

Hi Bill.

Zip file attached. Hopefully the names of the files makes sense, and you can link the html files with the pngs based on the names.

Some more results:

  • Adding destroyRecursive() to the button seemed to reduce the memory leak completely but on closer inspection there was a small leak - it just wasn't big enough to show up on the task manager graph properly. Because of this I went back and remeasured a bunch of the other tests to make sure I'd not missed anything.
  • Pure DOM, button only: No leak
  • Single dijit button, no border container: No leak.
  • Border container, no button (big string attached to the BC): LEAK! And a big enough one to show on the graph - see picture.

So this seems to indicate to me that the border container is not being properly destroyed on page unload, and that any dijits within it are also failing to be destroyed. Do you agree?

The 'tests' have not been massively scientific. I've been running a clean, vanilla FF, restarting the browser (but not the computer) after each test and waiting for the memory graph to settle, indicating the browser process has been properly killed. Each test refreshes every two seconds for two minutes. I leave it running for ~15s before starting the stopwatch to allow the browser to settle. I've not been explicitly testing that the memory remains high after the tab is closed (with the browser still open), which upon reflection would have been a good idea. Also, because the leak object is so high the leak during the test with button.destroyRecursive() was hard to gauge as the memory was jumping about all over the place as the button was being destroyed and the underlying leak which I believe is caused by the border container was somewhat lost in the noise. However I've measured it 3 times and got 0.8MB/refresh, 0.25MB/refresh and 0.5MB/refresh. I'd not put too much weight on those figures tho!

All the best.

Tom

Changed 9 years ago by Tom Elliott

Attachment: ffLeakNoBCs.tar.gz added

Various HTML and PNG files used for my testing. No border containers

Changed 9 years ago by Tom Elliott

Attachment: ffLeakWithBCs.tar.gz added

Various HTML and PNG files used for my testing. With border containers

comment:12 Changed 9 years ago by Tom Elliott

Paraphrased conversation with wild_bill on IRC:

Testing the BC only with Dojo 1.3 from the AOL CDN leaks. Testing by using Dojo 1.4 and adding the bigstring directly to Dijit doesn't leak. I'm going to try and find out which dijits seem to make the leak.

comment:13 Changed 9 years ago by Tom Elliott

Right, new tests attached.

I tried dijit.form.Form and dijit.layout.StackContainer?,TabContainer?,ContentPane?,BorderContainer? and only the border container leaks.

I'm going to dig around in the code and see if I can figure out what's going on here.

Changed 9 years ago by Tom Elliott

Attachment: dijits.zip added

Same test with a bunch of different dijits

comment:14 Changed 9 years ago by Tom Elliott

Oh, and I've confirmed that this exists in 1.3 as well as 1.4

comment:15 Changed 9 years ago by bill

Resolution: wontfix
Status: newclosed
Summary: Memory Leak in FirefoxBorderContainer: Memory Leak in Firefox 3.0

Hi mrtom, thanks for all the test cases.

I was able to reproduce this on FF3.0/WinXP. I saw the "VM Size" in task manager go up to 1G eventually.

I tried the BorderContainer test on FF3.5 and FF3.6, both mac and windows, with no leak shown. It oscillates a lot, up to 200M, then down less than 100M, but not a constant increase.

So there is something weird with BorderContainer, triggering a bug in FF3.0, but I'm not sure what. Given that it's fixed in FF3.5 and that FF3.0 is on the way out (not sure when it will be officially EOL, I thought it was end of 2009), I don't think it's worth trying to fix.

As we said earlier the dijit._destroyAll() call fixes the problem, so I think that's a good workaround if this is a big issue for you.

comment:16 Changed 9 years ago by Tom Elliott

Thanks for looking Bill.

I'm afraid I haven't had the time to look into this in the last week or so either, but I agree that if it's fixed in FF3.5 then there's not much point taking this any further.

Tom

Note: See TracTickets for help on using tickets.