Reference cycle between nsProgressNotificationProxy and nsHttpChannel on channel redirect

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

({memory-leak})

Trunk
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Created attachment 394014 [details] [diff] [review]
patch v1

Added a patch. Flagging biesi for review.
Attachment #394014 - Flags: review?(cbiesinger)
Comment on attachment 394014 [details] [diff] [review]
patch v1

+    // ..and the old callbacks

maybe make that 3 dots for consistency?
Attachment #394014 - Flags: review?(cbiesinger) → review+
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)
Keywords: mlk
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Updated

9 years ago
Blocks: 435296
(Assignee)

Comment 4

9 years ago
Created attachment 394023 [details] [diff] [review]
patch v2

fixed the .
Attachment #394014 - Attachment is obsolete: true
(Assignee)

Comment 5

9 years ago
pushed to mc in 7b05cc2cb9ee.

Resolving as fixed.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.