509929 - Reference cycle between nsProgressNotificationProxy and nsHttpChannel on channel redirect

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Bobby Holley (:bholley)]

Some slight modification (not sure which one) in the behavior of imgRequest that I implemented in bug 435296 uncovered a memory leak that can be reproduced reliably by running
http://mxr.mozilla.org/mozilla-central/source/content/events/test/test_bug336682_2.xul
with the current bug 435296 patch applied.

I banged my head against it for about a day and a half before convincing myself that I wasn't doing anything wrong imagelib side and that it was probably a necko bug. I then had the bright idea of asking biesi to look at it, and he diagnosed the problem in 15 minutes.

Basically, we need to set mCallbacks and mProgressSink to null here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#1281

nsProgressEventSink should also implement nsIChannelEventSink so that it can update mChannel when the channel changes.

Patch coming soon.

Comment 1

[Bobby Holley (:bholley)]

Attachment: patch v1

Added a patch. Flagging biesi for review.

Comment 2

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

Comment on attachment 394014 [details] [diff] [review]
patch v1

+    // ..and the old callbacks

maybe make that 3 dots for consistency?

Comment 3

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

fwiw, this is not an issue with redirects in general, just when the redirect is due to PAC. (and in that case, it could also cause memory leaks in other places)

Comment 4

[Bobby Holley (:bholley)]

Attachment: patch v2

fixed the .

Comment 5

[Bobby Holley (:bholley)]

pushed to mc in 7b05cc2cb9ee.

Resolving as fixed.