Heikki Toivonen (remove -bugzilla when emailing directly) Reporter
	Description • 22 years ago

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) { ... };
}

	Jesse Ruderman
	Comment 1 • 19 years ago

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.

	Jesse Ruderman
	Comment 2 • 19 years ago

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

	David Baron :dbaron: Assignee
	Comment 3 • 19 years ago

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.

	David Baron :dbaron: Assignee
	Comment 4 • 19 years ago

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.

	David Baron :dbaron: Assignee
	Comment 5 • 19 years ago

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

	David Baron :dbaron: Assignee
	Comment 6 • 19 years ago

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.

	Darin Fisher
	Comment 7 • 19 years ago

> 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 ;-)

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 8 • 19 years ago

<?php sleep(20); ?>
DATA GOES HERE





(But dbaron said that this patch doesn't *actually* need testing, when I offered that solution to him. :) )

	David Baron :dbaron: Assignee
	Comment 9 • 19 years ago

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

	Jesse Ruderman
	Comment 10 • 19 years ago

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

	David Baron :dbaron: Assignee
	Comment 11 • 19 years ago

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.

	David Baron :dbaron: Assignee
	Comment 12 • 19 years ago

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.

	David Baron :dbaron: Assignee
	Comment 13 • 19 years ago

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

	David Baron :dbaron: Assignee
	Comment 14 • 19 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 15 • 19 years ago

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

	Darin Fisher
	Comment 16 • 19 years ago

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

	Johnny Stenback (:jst)
	Comment 17 • 19 years ago

Comment on attachment 221016 [details] [diff] [review]
patch

sr=jst with the mrbkap's and darin's comments addressed.

	David Baron :dbaron: Assignee
	Comment 18 • 19 years ago

(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.

	David Baron :dbaron: Assignee
	Comment 19 • 19 years ago

> >+  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.

	Darin Fisher
	Comment 20 • 19 years ago

> 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?

	David Baron :dbaron: Assignee
	Comment 21 • 19 years ago

(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) ?

	David Baron :dbaron: Assignee
	Comment 22 • 19 years ago

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

	Darin Fisher
	Comment 23 • 19 years ago

> 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.

	David Baron :dbaron: Assignee
	Comment 24 • 19 years ago

(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.

	David Baron :dbaron: Assignee
	Comment 26 • 19 years ago

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.

	David Baron :dbaron: Assignee
	Comment 27 • 19 years ago

With a few more tweaks.

	David Baron :dbaron: Assignee
	Comment 28 • 19 years ago

Checked in to trunk.

	Frank Yan (:fryn)
	Comment 29 • 19 years ago

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

	David Baron :dbaron: Assignee
	Comment 30 • 19 years ago

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

	Frank Yan (:fryn)
	Comment 31 • 19 years ago

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.)

	Jesse Ruderman
	Comment 32 • 19 years ago

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.

	David Baron :dbaron: Assignee
	Comment 33 • 19 years ago

Fixed on MOZILLA_1_8_BRANCH by checkin of bug 336791.