Closed Bug 384180 Opened 15 years ago Closed 15 years ago

[leak] nsDownload doesn't break mCancelable cycle on fail or cancel

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Keywords: memory-leak)

Attachments

(1 file, 4 obsolete files)

Attached patch v1 (obsolete) — Splinter Review
(Add|Retry)Download make a cycle with mCancelable when starting downloads, but the cycle is *only* broken when the download successfully finishes.
>+  // Creates a cycle that will be broken when the download can't continue (done/fail/cancel)
How about this instead:
Creates a cycle that will be broken when the download stops

Also, what about using nsDownload::SetState to see if we are in a state where we should remove the cycle and do so there (so that it is always correct as opposed to having to keep adding it in in various places in the code).

Another thing to consider - do we want nsDownload to participate in cycle collection?  After talking to dbaron, in order to do this we'd have to make nsWebBrowser and nsWebBrowserPersist participate as well.  Both are not threadsafe, so I think we can do that.
This leak exists on branch as well.  We may want to fix it there as well...
Flags: blocking1.8.1.6?
Flags: blocking-firefox3?
Keywords: mlk
Attached patch v2 (trunk only) (obsolete) — Splinter Review
Consolidated cycle breaking code to SetState as well as refactored the "done" states from (GetCan)?CleanUp.
Attachment #268112 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
break the cycle from RemoveDownloadFromCurrent
Attachment #268141 - Attachment is obsolete: true
Comment on attachment 268147 [details] [diff] [review]
v3

Now this I like :)

>+  PRBool RemoveDownloadFromCurrent(nsDownload *aDownload);
Can you add a comment here indicating that it removes the cycle and that you should grip it with some kungFuDeathGrip if you need access to member variables still please?


You also didn't update the comments in AddDownload and RetryDownload like you had done in previous patches.
Attached patch v4 (obsolete) — Splinter Review
add comments
Attachment #268147 - Attachment is obsolete: true
Attachment #268166 - Flags: review?
Duplicate of this bug: 326938
Attachment #268166 - Flags: review? → review?(gavin.sharp)
Comment on attachment 268166 [details] [diff] [review]
v4

CancelDownload didn't kungFu, and right after calling RemoveDownloadFromCurrent, it does..

nsresult rv = dl->SetState(nsIDownloadManager::DOWNLOAD_CANCELED);

Waiting on Bug 384219 which will consolidate functionality into FinishDownload, and that function will have its own local kungFu.
Attachment #268166 - Flags: review?(gavin.sharp) → review-
Depends on: 384219
Attached patch v5Splinter Review
diff'd with the changes from bug 384219.

Added a kungFu within FinishDownload and broke the cycle early in the method because putting it later might not have it break the cycle if returning early.

CancelDownload doesn't actually need a kungFu - it already has a RefPtr |dl|.

<mconnor>	he [sdwilsh] should be {a suitable reviewer for download manager}, IMO
Attachment #268166 - Attachment is obsolete: true
Attachment #268620 - Flags: review?(sdwilsh)
Comment on attachment 268620 [details] [diff] [review]
v5

Almost :)

>+  // We don't want to lose access to the download's member variables
>+  nsRefPtr<nsDownload> kungFuDeathGrip = aDownload;
It sucks that we have to do this in here :(
No way around it though.


> NS_IMETHODIMP
> nsDownload::OnStatusChange(nsIWebProgress *aWebProgress,
>                            nsIRequest *aRequest, nsresult aStatus,
>                            const PRUnichar *aMessage)
> {   
>   if (NS_FAILED(aStatus)) {
>+    // We don't want to lose access to our member variables
>+    nsRefPtr<nsDownload> kungFuDeathGrip = this;
We don't actually use an member variables after this, so we don't need this.
Attachment #268620 - Flags: review?(sdwilsh) → review-
(In reply to comment #10)
> > nsDownload::OnStatusChange(nsIWebProgress *aWebProgress,
> >+    nsRefPtr<nsDownload> kungFuDeathGrip = this;
> We don't actually use an member variables after this, so we don't need this.

Does the mDownloadManager not count - a few lines after FinishDownload?

nsCOMPtr<nsIStringBundle> bundle = mDownloadManager->mBundle;
Comment on attachment 268620 [details] [diff] [review]
v5

er, yes.  My bad :)
Attachment #268620 - Flags: review- → review+
Summary: nsDownload doesn't break mCancelable cycle on fail or cancel → [leak] nsDownload doesn't break mCancelable cycle on fail or cancel
This isn't testable as far as I know.

Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.28; previous revision: 1.27
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.87; previous revision: 1.86
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Flags: blocking-firefox3? → blocking-firefox3+
Is this really a huge problem? Trying to limit churn on the security branch to stuff we really have to have (top crashes, security fixes, regression from previous security fixes). Renominate if we're not appreciating the scale of the problem.
Flags: blocking1.8.1.7? → blocking1.8.1.7-
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.