400091 - [patch] nsFaviconService leak with rapid page loads

Status: RESOLVED FIXED

(Core :: Networking, defect, P4)

Description

[Jesse Ruderman]

Attachment: testcase

Setup:
1. Save the testcase as "fsl.html".
2. export XPCOM_MEM_LEAK_LOG=2

Steps to reproduce:
3. Run a debug build of Firefox with "fsl.html" on the command line.
4. Wait a few seconds for it to finish.
5. Quit.

Result: trace-refcnt reports that nsFaviconService and a bunch of other things have leaked.

Comment 1

[Jesse Ruderman]

Attachment: what leaked

Comment 2

[Robert Sayre]

We fail to open the channel in DoSetAndLoadFaviconForPage

cvs diff isn't working for me right now, but this prevents the leak:

rv = channel->SetNotificationCallbacks(listenerRequestor);
NS_ENSURE_SUCCESS(rv, rv);
rv = channel->AsyncOpen(listener, nsnull);
if (NS_FAILED(rv)) {
  channel->SetNotificationCallbacks(nsnull);return rv;
}

Doesn't seem like it should be necessary, though.

Comment 3

[Robert Sayre]

We get a cycle here because nsBaseChannel doesn't null out mCallbacks if AsyncOpen fails. 

Comment 4

[Robert Sayre]

biesi, any reason we can't call SetNotificationCallbacks(nsnull) from nsBaseChannel::AsyncOpen if it takes the failure path?

Comment 5

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

I'm not sure that that's a good side-effect of asyncOpen, but I guess since you can't do much with a channel when asyncOpen failed that's probably ok.

Comment 6

[Robert Sayre]

Attachment: clear mCallbacks

Comment 7

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

I'm curious though, exactly what fails there and why?

Comment 8

[[e-mail]]

I don't suppose this could have caused the cb-sea-linux-tbox orange?