We will still leak in this very unlikely situation where XMLHttpRequest is not used for anything... This sample from jst: function foo() { var r = new XMLHttpRequest(); r.onload = function(e) { ... }; }

This bug causes six DOMWindows to leak while loading Gmail (bug 321282), so it's not as obscure as it was when it was filed.

David Baron has a work-in-progress patch on bug 321054.

This fixes the 3 nsXmlHttpRequest objects leaked when loading gmail, but it doesn't fix any significant portion of the rest of what leaks when loading gmail, so it seems that either (1) not much was entrained by that leak (2) what was entrained by that is the same as what's entrained by other still-extant leaks. The only other things that went away with the leak, for the gmail case, were some XPConnect wrappers.

Then again, the big leak I was seeing on gmail was bug 335785, which was introduced only 2 days ago. I remember seeing bigger leaks in the past; that probably required doing more than just loading it.

Actually, this problem probably just entrained almost the same set of objects.

I haven't tested this much; however, I've done a good bit of testing of the nsDOMClassInfo changes as part of my work on bug 321054, and gmail still works, and doesn't leak. If anybody has an environment set up for testing XMLHttpRequest on documents that are served with significant delay, I could write some testcases, but I don't think it's critical.

> If anybody has an environment set up for testing XMLHttpRequest on documents > that are served with significant delay, I could write some testcases, but I > don't think it's critical. UniversalBrowserRead + big files on ftp.mozilla.org ;-)

<?php sleep(20); ?> DATA GOES HERE (But dbaron said that this patch doesn't *actually* need testing, when I offered that solution to him. :) )

Or rather that doing detailed testing of it isn't a priority for me, mainly since I just don't have time.

http://software.hixie.ch/utilities/cgi/test-tools/delayed-file is great for testcases.

I'm actually going to be attaching another patch here shortly to do make the GC marking a good bit more exact to satisfy bug 321054 comment 35.

This makes the marking a bit more exact -- it only marks externally referenced things *if* they have an event handler, and it does that by maintaining another table. I need to do this so that I don't add a leak regression (relative to pre- bug 241518) for multipart/x-mixed-replace cases other than the one described in bug 321054 comment 35 (which already leaked). It's also a good thing otherwise. And it also leaves room for making the test even stricter -- we could check for only event handlers that depend on network activity, although currently it's done for all of them (plus the treewalker stuff too). I should file a followup bug on making that stricter.

Oops, missed a file in the diff (nsJSUtils.cpp).

So, when reviewing this patch, it's probably worth starting by reading nsDOMClassInfo.h: the changes there are to support the concept of an objects being part of GC preservation because they're reachable by something that's not part of the object graph that nsIDOMGCParticipant exposes. The basic point of nsIDOMGCParticipant is to expose the reachability in the underlying C++ data structures so it can become part of the GC, but that's insufficient when something is reachable from the network code because it's loading. Rather than make the entire world implement nsIDOMGCParticipant so that we can learn that things are reachable, the basic idea here is to essentially root things **to the reachability code (which may then in turn mark them as rooted in the JS GC) if network activity could potentially cause a JS-implemented event handler to fire. Other than that, this is a pretty straightforward use of the code added in bug 241518, like bug 323534.

Comment on attachment 221016 [details] [diff] [review] patch >Index: dom/src/base/nsDOMClassInfo.cpp >+ if (!sRootWhenExternallyReferencedTable.ops && >+ !PL_DHashTableInit(&sRootWhenExternallyReferencedTable, >+ PL_DHashGetStubOps(), nsnull, >+ sizeof(RootWhenExternallyReferencedEntry), 16)) >+ return NS_ERROR_OUT_OF_MEMORY; Nit: Please overbrace the then clause of this if statement. >+ if (sExternallyReferencedTable.ops) >+ PL_DHashTableEnumerate(&sExternallyReferencedTable, >+ MarkExternallyReferenced, cx); Same nit here. >+ * The table of externally participants is a list of participants that The table of externally *referenced* participants... ? >+ nsTArray<listener_holder*> *array; > if (type.Equals(LOADSTR)) { ... >+ array = &mLoadEventListeners; > } > else if (type.Equals(ERRORSTR)) { ... >+ array = &mLoadEventListeners; > } Here and below, you use a pointer to an array that is only ever set to one thing. Did you mean mErrorEventListeners in the second case? Other than that, this looks pretty cool, r=mrbkap

Comment on attachment 221016 [details] [diff] [review] patch >+nsDOMClassInfo::UnsetExternallyReferenced(nsIDOMGCParticipant *aParticipant) ... >+ // XXX This churn is bad! This table is destroyed often. >+ if (sExternallyReferencedTable.entryCount == 0) { >+ PL_DHashTableFinish(&sExternallyReferencedTable); >+ sExternallyReferencedTable.ops = nsnull; >+ } Then why destroy the table? Maybe you could stash a pointer to the ops in another static variable, and then restore that when you would other re-init the table. Then at shutdown time, finish the table for real. >Index: content/base/src/nsXMLHttpRequest.cpp >+ nsTArray<listener_holder*> *array; s/listener_holder/ListenerHolder/ ? seems like a deviation from convention to name types with "lower_case" style in this file, but maybe this it the convention elsewhere for typedefs in content/? >+ // Allow a caller to remove O(N^2) behavior by removing end-to-start. >+ for (PRUint32 i = array->Length() - 1; i != PRUint32(-1); --i) { Instead of PRUint32(-1), use |array->NoIndex| >+ nsCOMPtr<nsIOnReadyStateChangeHandler> listener = mOnReadystatechangeListener.Get(); >+ *aOnreadystatechange = nsnull; >+ listener.swap(*aOnreadystatechange); It might be nice to have an overloaded version of .Get() that takes a T** out param. That way you can avoid writing out this .swap code here and elsewhere. > nsXMLHttpRequest::NotifyEventListeners(nsIDOMEventListener* aHandler, >+ nsCOMArray<nsIDOMEventListener>* aListeners, const nsCOMArray? >+ if (nsCOMPtr<nsIDOMEventListener>(mOnProgressListener.Get())) { Is there a way for nsMarkedJSFunctionHolder<T> to provide a .Has() method or something like that that could be used to avoid the AddRef/ Release pair here and elsewhere? > // Grab hold of the event listener lists we will need >+ nsCOMPtr<nsIDOMEventListener> onLoadListener = mOnLoadListener.Get(); >+ PRUint32 count = mLoadEventListeners.Length(); >+ nsCOMArray<nsIDOMEventListener> listeners_copy(count); >+ for (PRUint32 i = 0; i < count; ++i) >+ listeners_copy.ReplaceObjectAt(nsCOMPtr<nsIDOMEventListener>(mLoadEventListeners[i]->Get()), i); Likewise, is there no way to get a non-owning pointer to the object held by the nsMarkedJSFunctionHolder<T>? If we could get that, then we could avoid this AddRef/Release pair. After reading nsMarkedJSFunctionHolder_base::Get, I see that there is no way to return the object without the owning ref. But, it does seem like you could implement the Has() function. At least in the non-weak case, you could optimize that function. Maybe it's not worth the code? >+ NotifyEventListeners(onLoadListener, &listeners_copy, domevent); s/listeners_copy/listenersCopy/ for code style consistency. >+ nsCOMPtr<nsIDOMEventListener> onErrorListener = mOnErrorListener.Get(); >+ PRUint32 count = mErrorEventListeners.Length(); >+ nsCOMArray<nsIDOMEventListener> listeners_copy(count); >+ for (PRUint32 i = 0; i < count; ++i) >+ listeners_copy.ReplaceObjectAt(nsCOMPtr<nsIDOMEventListener>(mErrorEventListeners[i]->Get()), i); Move to subroutine and share with "load" listeners? >+/* virtual */ void >+nsXMLHttpRequest::AppendReachableList(nsCOMArray<nsIDOMGCParticipant>& aArray) >+{ >+ nsCOMPtr<nsIDOMGCParticipant> gcp = do_QueryInterface(mDocument); >+ if (gcp) >+ aArray.AppendObject(gcp); >+} The documentation for AppendReachableList says that null pointers can be appended and that they will be ignored by the caller. Is it worth it to null check here? If there is a reason to avoid appending nulls (to minimize memory use perhaps?) then maybe that should be mentioned in the nsIDOMGCParticipant header docs? >Index: content/base/src/nsXMLHttpRequest.h >+ nsMarkedJSFunctionHolder<nsIDOMEventListener> mOnLoadListener; >+ nsMarkedJSFunctionHolder<nsIDOMEventListener> mOnErrorListener; >+ nsMarkedJSFunctionHolder<nsIDOMEventListener> mOnProgressListener; listener_holder mOnLoadListener; ... Or, better yet: ListenerHolder as I mentioned above ;-) r=darin

Comment on attachment 221016 [details] [diff] [review] patch sr=jst with the mrbkap's and darin's comments addressed.

(In reply to comment #16) > >+ // Allow a caller to remove O(N^2) behavior by removing end-to-start. > >+ for (PRUint32 i = array->Length() - 1; i != PRUint32(-1); --i) { > > Instead of PRUint32(-1), use |array->NoIndex| I'd prefer to leave it; NoIndex need not be -1, but this must be -1.

> >+ if (nsCOMPtr<nsIDOMEventListener>(mOnProgressListener.Get())) { > > Is there a way for nsMarkedJSFunctionHolder<T> to provide a .Has() > method or something like that that could be used to avoid the AddRef/ > Release pair here and elsewhere? Given that the weak reference work is the normal case, it wouldn't help much.

> I'd prefer to leave it; NoIndex need not be -1, but this must be -1. Oh, that's fine. I actually don't think my advice was good. Perhaps PR_UINT32_MAX?

(In reply to comment #16) > Move to subroutine and share with "load" listeners? It can't be moved into NotifyEventListeners because of the clearing listeners that happens in the middle, and it doesn't seem like enough code to make a second function worthwhile. > The documentation for AppendReachableList says that null pointers can > be appended and that they will be ignored by the caller. Is it worth > it to null check here? If there is a reason to avoid appending nulls > (to minimize memory use perhaps?) then maybe that should be mentioned > in the nsIDOMGCParticipant header docs? I'd prefer not to grow arrays when unnecessary. Originally it wasn't null-safe, and then I changed it (I've forgotten why), and I've kept most of the callers that way. > Or, better yet: ListenerHolder as I mentioned above ;-) I'd rather use the typedef as little as possible, since it's just an extra level of indirection. (In reply to comment #20) > Oh, that's fine. I actually don't think my advice was good. Perhaps > PR_UINT32_MAX? What's wrong with PRUint32(-1) ?

Boris also had some review comments on *this* patch in bug 321054 comment 42 (and see the following comments for some replies).

> What's wrong with PRUint32(-1) ? Nothing, actually. Somehow each time I looked at that code, I didn't see that you wanted to break when the unsigned integer underflowed. PRUint32(-1) is the most clear in that case. Sorry for the spam.

(In reply to comment #16) > > // Grab hold of the event listener lists we will need > >+ nsCOMPtr<nsIDOMEventListener> onLoadListener = mOnLoadListener.Get(); > >+ PRUint32 count = mLoadEventListeners.Length(); > >+ nsCOMArray<nsIDOMEventListener> listeners_copy(count); > >+ for (PRUint32 i = 0; i < count; ++i) > >+ listeners_copy.ReplaceObjectAt(nsCOMPtr<nsIDOMEventListener>(mLoadEventListeners[i]->Get()), i); > > Likewise, is there no way to get a non-owning pointer to the object held > by the nsMarkedJSFunctionHolder<T>? If we could get that, then we could > avoid this AddRef/Release pair. The real solution here is for nsCOMArray<T> to have overloaded methods taking already_AddRefed<T>. I filed bug 339277 suggesting this.

In the process of merging I took an additional review comment that I commented above wasn't worthwhile -- I created CopyEventListeners, since it's now used 4 times instead of 2. If one of you wants to do a quick re-review of this, it might be worthwhile; there was a lot of merging.

With a few more tweaks.

Checked in to trunk.

will this be checked into the 1.8.1 branch once the branch re-opens?

Hopefully, but it requires a bunch of other patches as well. See bug 336791.

Well, users have been constantly complaining about memory leaks, whether actually leaks or not, so including these for Firefox 2.0 would definitely please many nagging users, especially Gmail users. (This patch fixes "six DOMWindows leaking while loading Gmail" - bug 321282.)

I think Gmail changed to stop triggering this bug (see bug 321282 comment 6). Gmail wasn't the only site that triggered this bug, of course.

Fixed on MOZILLA_1_8_BRANCH by checkin of bug 336791.