323454 - [FIX]Don't leak the channel and XMLHttpRequest object if AsyncOpen fails

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Martijn Wargers (dead)]

When visiting the url, Mozilla leaks memory:
Summary:
Leaked 2 out of 10 DOM Windows
Leaked 3 out of 36 documents
Leaked 0 out of 4 docshells

I'll attach a testcase from that site that leaks 2 DOM Windows in Mozilla.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Steve England [:stevee]]

Related to bug 206520? (XMLHttpRequest related)

Comment 3

[Martijn Wargers (dead)]

Maybe, except in this testcase, send is called and no event listener is set.

Comment 4

[Martijn Wargers (dead)]

Apparently, the testcase only leaks when testing locally.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So the problem here is that OpenContentStream() throws for a nonexistent file:// URI in nsBaseChannel::AsyncOpen.  And then we leak because the XMLHttpRequest has the channel in mChannel and the channel has a ref to XMLHttpRequest via the notification callbacks.

Should nsBaseChannel::AsyncOpen drop notification callbacks on any failure?  Or should XMLHttpRequest null out mChannel?  Or both, perhaps?

Note that this issue is specific to file:// URIs, so the MSN money thing is likely a different issue...

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

this would, obviously, be fixed if nonexistant file uris didn't throw in asyncopen, right?

I don't think AsyncOpen should drop notificationCallbacks in this case. maybe xmlhttprequest should set them to null explicitly.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> this would, obviously, be fixed if nonexistant file uris didn't throw in
> asyncopen, right?

True, but I was wondering about the general behavior (eg if allocating an nsInputStreamPump is OOM).

Is doing anything with a channel after AsyncOpen() fails supported?

Comment 8

[Darin Fisher]

Any caller of nsIChannel::AsyncOpen needs to be prepared for it to fail.  The particulars of why it fails here aren't really relevant.  XMLHttpRequest should ensure that if AsyncOpen fails that it breaks any reference cycles it may have created.  I vote for having XMLHttpRequest clear the channel's notification callbacks in this error case.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Actually, I was thinking it should just drop the mChannel ref if the AsyncOpen fails.

That said, we should probably check on other AsyncOpen callers and see whether they suffer from similar problems...

Comment 10

[Darin Fisher]

Agreed.  Docshell avoids this by hooking up a weakref to itself as the notification callbacks for channels it creates.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

I skimmed the other AsyncOpen callers and they look ok to me.

Comment 12

[Darin Fisher]

Comment on attachment 212190 [details] [diff] [review]
Proposed fix

ok, but chrome apps can access .channel.  will they be confused to see the channel go away?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Not sure...  But they shouldn't be accessing it anyway after a failed AsyncOpen, since it's in an indeterminate state.  Since they have no way to tell that, I think this is better than letting them mess with the channel at that point...  I would be happy to document that on the interface.

Comment 14

[Martijn Wargers (dead)]

Thanks for that fix, Boris.
As soon as that fix gets checked in, I'll try to get a minimised testcase again (it's currently confusing to get different memory leaks when you test it locally).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 212190 [details] [diff] [review]
Proposed fix

I think it's worth taking this on the branch.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.

Comment 18

[Martijn Wargers (dead)]

Verified fixed, using 2006-02-22 trunk build.
I still leak with the url as Boris already expected. I filed bug 328311 for it. (hoping I can make a minimal testcase of that).