177533 - The memory is increased every time XMLHttpRequest.send is fired

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[José Antonio Sánchez Sánchez]

Hello,
I am trying to get remote XML with XMLHttpRequest. my source code is:

function load() {
  var req = new XMLHttpRequest();
  req.open("GET","http://www.foo.com/anxml.xml",false);
  req.setRequestHeader('content-type','text/xml');
  req.onload = function() {
    alert("received");
  }
  req.send(null);

}

it works, but When I execute the function load more than once, the memory used
by mozilla grows exactly the size of the document I am requesting. For example:

first time: 22.4 mb;
second time: 23.4 mb;
third time: 24.4 mb;

when the file size is 1mb;

But, if I initialize the req object like a global variable, the memory is always
the same.

I don't understand why the memory is being increased. The object would be
destroyed every time the function load finishes. Or it would not?

Is it a bug? Is it correct?

Is there any way to access the garbage collector, and clean up the memory usage
of the var req?

I think the garbage collector must run every time a JS thread finish.

Thanks in advance.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The object would be destroyed when it's garbage collected.... I'm not sure what
you mean by "js thread" -- there are no js threads involved here.

Comment 2

[Phil Schwartau]

As a garbage-collected language, JS does not guarantee when
such collection takes place. And as far as I know, there is 
no way for the JS end-user to invoke JS garbage collection.

Native objects, like the XMLHttpRequest object in this example,
should expose a finalize() method that gets invoked when JS garbage
collection does occur on JS objects like |req| in the above example.
This method would be responsible for releasing all memory used by
the native object. 

On this basis, let me assign this to the XML component for further
analysis of this memory usage issue -

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I was almost certain this bug was a dupe of bug 69145, but my tests seem to
indicate otherwise. When using document.load() I can make the "leak" go away by
introducing enough dummy variables that are not referenced. With XMLHttpRequest
that does not seem to happen, so we might have a real leak in this case.

The URL is to a Netscape-internal testcase that I am working on, but it is just
variations of the sample from José and my samples in bug 69145).

Comment 4

[José Antonio Sánchez Sánchez]

Boris I meant the req object might be destroyed once the load function finished.

I agree with Phil. There are two test case at 
http://www.mans-hq.net/XUL/request.xul and http://www.mans-hq.net/XUL/request2.xul.

the requested file used in the two tests is http://www.mans-hq.net/XUL/test.rdf

with The first you can test my first code. 

The second is a way to get around the problem. In order to test it, you must set
the preference pref("signed.applets.codebase_principal_support", true); in
defaults/pref/all.js of your mozilla directory. 

Anyway if you reload the page the memory is not freed, in addition it only
allows synchronous request. 

Jose. 

Comment 5

[Jo Hermans]

*** Bug 183425 has been marked as a duplicate of this bug. ***

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: output from make-tree.pl refcount balancer

I was following the instructions at
http://www.mozilla.org/performance/leak-tutorial.html to try and find why we
leak. After setting the environment variables, launching, running my testcase
once, going to mozilla.org home, and quitting, I saw that one (the first one)
nsXMLHttpRequest object leaked, and there were three refs to it. I then run
find-leakers.pl and make-tree.pl, and this is the output from the latter.

Unfortunately this does not look at all like it should look, according to the
tutorial or manual, so I haven't gotten anywhere yet.

I did this on Windows, so maybe I need to try this on Linux for better results.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Linux version, with comptrs

Linux seems to work much better. 2 of the 3 leaking refs are from comptrs.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: comleak

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Partial fix, comptrs

The comleak file pointed me to this: SetNotificationCallbacks() will addref the
parameter, so we need to use the proxy for this as well. This got rid of 2
refs, 1 more to go (no idea where that is yet). This still has a hacky feeling
to it, which I would like to fix (the business with temp that gets passed in to
SetNotificationCallbacks()).

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: wip, crashes

Actually I don't think I tested the previous patch well enough, I suspect the
QI resulted in null so basically we were calling like this:
SetNotificationCallbacks(nsnull), which is the same as if I commented out the
line.

I think the AsyncOpen() is the another culprit where we need to use the proxy
or something.

This patch is how I'd like things to be, but unfortunately this does not work
(in fact it crashes). Need to get some sleep before I can figure this one out.

Comment 11

[Juan José Mata]

Discussed at EDT triage: plussing topembed nomination since the leak seems to be
directly poportional to the file size, therefore potentially rather large.

Comment 12

[Brad Clements]

I have encountered this memory bug. I have an embedded webserver running on
uClinux which controls a KVM switch in "real time" via XML-RPC commands from any
browser.

The web client continually updates it's status display, sending a short xml-rpc
request and receiving a short reply every 2 seconds. The memory use of Mozilla
1.3b grows without bounds. Within a few hours, it exceeds 82 megabytes. (Win2k)

We're only sending and receiving perhaps 100 bytes with each request.

We're using async http requests, with the vcXMLRPC javascript code.

If anyone wants to test against our webserver, I'll set you up. I'd like to see
this fixed. Thanks

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: one ref left

This patch clears all but one ref.

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 121965 [details] [diff] [review]
one ref left

Actully it seems like this fixes the mlk when there are no (load?) event
listeners.

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix most cases and other issues

This is the previous patch that fixes some other issues that I noticed, namely:

* minor perf/footprint optimizations
* addref some out parameters (how the heck had we missed those?)

This fixes all cases where there are no event listeners. Additionally, it fixes
most(?) cases with event listeners (I will file a new bug to fix this fully,
but it will not make milestone 1.4).

Below is a sample of how leaking JS code can be made not to leak:

// We can have the event listener function here which prevents ref. cycles
function foo() {
  // Do something
}

function asyncload() {
  var req = new XMLHttpRequest();
  req.open("GET", "myfile.xml", true);
  req.onload = foo;
//If we set onload here like this, we will have reference cycles and
//leak XMLHttpRequest objects
//req.onload = function() {
//  // Do something
//}
  req.send(null);
}

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 122343 [details] [diff] [review]
Fix most cases and other issues

darin, jst: the meat here is the SetNotificationcallbacks(), the rest are minor
tweaks/fixes. I want this in for 1.4, and I'll work on the full fix in some
later milestone.

Comment 17

[Darin Fisher]

Comment on attachment 122343 [details] [diff] [review]
Fix most cases and other issues

looks ok to me.  r=darin

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 122343 [details] [diff] [review]
Fix most cases and other issues

Very low risk of regressions, fixes memory leaks in most situations,
potentially fix crashers and other minor goodies.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 122343 [details] [diff] [review]
Fix most cases and other issues

sr=jst

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I filed bug 204545 for the remaining issues.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 122343 [details] [diff] [review]
Fix most cases and other issues

a=asa (on behalf of drivers) for checkin to 1.4b.

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in.