205550 - Mozilla WSDL leaks memory

Status: RESOLVED FIXED

(Core Graveyard :: Web Services, defect)

Description

[harishd]

With XPCOM_MEM_LEAK_LOG set I noticed that our WSDL ( and hence SOAP )
implementation leaks memory.

Steps to reproduce:
1) set XPCOM_MEM_LEAK_LOG ( works only in debug mode ).
2) open up the above URL in mozilla ( 05/10/03 or later builds ).
3) enter some text in the text filed and click translate. 
4) quit the browser after you see the translated message.

Result: Leaks are reported to the standard output.

Comment 1

[harishd]

Attachment: Memory leak report.

Comment 2

[harishd]

This could be due to bug 204545 but not totally sure.

Comment 3

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

Please try with the patch in bug 204545, does that make the leak go away?

Comment 4

[John Gaunt (redfive)]

tested with the patch from bug 204545 and it still leaks like a sieve

Comment 5

[harishd]

It looks like there is a circular reference between
nsHTTPSOAPTransportCompletion and WSPCallContext. Releasing
nsHTTPSOAPTransportCompletion object after the call completion seems to fix the
leak. 

Comment 6

[Harshal Pradhan]

Taking. I have a fix for the problem mentioned in comment 5.

Comment 7

[Harshal Pradhan]

Attachment: Break the cycle by using a weak ref

This makes the SOAP completion object not have a direct ref to WSPCallContext.
I am still seeing some leaks, but this makes things remarkably better than they
are right now. All the JSRoot leakage at least is gone.

Comment 8

[Harshal Pradhan]

This has been in my tree for a while and I have been playing with WSDL stuff. I
have not seen anything broken by this.

I am not sure who does webservices reviews these days. John, Heikki, jst would
any either of you be willing to r the patch above?

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 142905 [details] [diff] [review]
Break the cycle by using a weak ref

This worries me slightly, who now holds on to the WSPCallContext while we're
waiting for the call to complete? Any references other than the caller? What if
the caller is from C++, and it releases the call context before the call comes
through, seems like that should be ok (though maybe odd, in some ways). Can't
we make sure the transport layer releases the listener when it's done with it
in stead (i.e. in  nsHTTPSOAPTransportCompletion::HandleEvent() and 
nsHTTPSOAPTransportCompletion::Abort())?

Comment 10

[Darin Fisher]

upping the severity of this one.  i see huge leaks as a result of this.

Comment 11

[Harshal Pradhan]

Attachment: Possible fix

This patch is what I have right now. Its basically making the wsp code drop the
ref to the completion object and holding on to the nsISOAPResponse object
instead. This should hopefully fix the leaks. (I havent actually confirmed
that.)

Re comment 9
> who now holds on to the WSPCallContext while we're
> waiting for the call to complete? 

Thats a fair point. Personally, I still think it is reasonable to expect the
caller to hold on to the callcontext in this case. There are unlikely to be C++
callers anyway. And if there are, we are likely to know about them.

Making the SOAP code drop the ref to the listner will make the 
|nsISOAPResponseListener listener| attribute of the completion object not
"work" after the call ends. I don't know enough to judge if that is a safe
change to make, or if it would break some scripts in the wild that happen to
use the low-level SOAP API's. Hence, the above approach of making the wsp code
drop its ref instead. 

I have not had a chance to test at all since I through a combination of lack of
time and bug 237845. And it is possible that I may not have access to the
machine with my mozilla tree for the next couple of weeks. Feel free to take
this up if you need it fixed before that.

Comment 12

[Harshal Pradhan]

Comment on attachment 144248 [details] [diff] [review]
Possible fix

I finally got around to giving this a spin. It seems to work. jst, what do you
think?

Comment 13

[Johnny Stenback (:jst)]

(In reply to comment #11)
> Re comment 9
> > who now holds on to the WSPCallContext while we're
> > waiting for the call to complete? 
> 
> Thats a fair point. Personally, I still think it is reasonable to expect the
> caller to hold on to the callcontext in this case. There are unlikely to be C++
> callers anyway. And if there are, we are likely to know about them.

True, but even from JS one could write code that wouldn't work if noone holds a
reference to the callcontext from JS (i.e. if the variable goes out of scope,
and GC runs before the call completes), so someone needs to ensure that it stays
alive.

> Making the SOAP code drop the ref to the listner will make the 
> |nsISOAPResponseListener listener| attribute of the completion object not
> "work" after the call ends. I don't know enough to judge if that is a safe
> change to make, or if it would break some scripts in the wild that happen to
> use the low-level SOAP API's. Hence, the above approach of making the wsp code
> drop its ref instead. 

Sounds reasonable.

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 144248 [details] [diff] [review]
Possible fix

- In WSPCallContext::Abort():

+    mCompletion = nsnull;

You should add a comment where you do this to explain why it's needed so that
we know to do this in other places too in the future, if this code ends up
changing a bunch.

- In WSPCallContext::CallSync():

+  nsCOMPtr<nsISOAPResponse> mResponse;
+  return mCall->Invoke(getter_AddRefs(mResponse));

You didn't mean to declare mResponse (which is the new member you added) here
did you? :-)

r=jst, darin, could you sr?

Comment 15

[Harshal Pradhan]

Attachment: Address jst's issues

Added comments and fixed the stupid mResponse thing in the sync call (Duh!)

Comment 16

[Mike Kaply [:mkaply]]

Is this "the big web services leak"?

Comment 17

[Harshal Pradhan]

mkaply, 

Yes, at least as far as I know, this was the single worst leak in webservices code.

Patch checked in on trunk.

Comment 18

[Mike Kaply [:mkaply]]

Does anyone think this is safe for 1.7?

My understanding is that this is one of the things that made our web services
unusable

Comment 19

[Harshal Pradhan]

> Does anyone think this is safe for 1.7?

I think it is fairly safe. It should certainly make things no worse than they
have been.

Comment 20

[Asa Dotzler [:asa]]

moving the request from blocking? to approval? where I think it was intended

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 146829 [details] [diff] [review]
Address jst's issues

a=asa (on behalf of drivers) for checkin to 1.7

Comment 22

[Harshal Pradhan]

Checked in on the 1.7 branch.