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

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Ed Lee :Mardak]

Attachment: v1

(Add|Retry)Download make a cycle with mCancelable when starting downloads, but the cycle is *only* broken when the download successfully finishes.

Comment 1

[Shawn Wilsher :sdwilsh]

>+  // 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.

Comment 2

[Shawn Wilsher :sdwilsh]

This leak exists on branch as well.  We may want to fix it there as well...

Comment 3

[Ed Lee :Mardak]

Attachment: v2 (trunk only)

Consolidated cycle breaking code to SetState as well as refactored the "done" states from (GetCan)?CleanUp.

Comment 4

[Ed Lee :Mardak]

Attachment: v3

break the cycle from RemoveDownloadFromCurrent

Comment 5

[Shawn Wilsher :sdwilsh]

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.

Comment 6

[Ed Lee :Mardak]

Attachment: v4

add comments

Comment 8

[Ed Lee :Mardak]

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.

Comment 9

[Ed Lee :Mardak]

Attachment: v5

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

Comment 10

[Shawn Wilsher :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.

Comment 11

[Ed Lee :Mardak]

(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 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 268620 [details] [diff] [review]
v5

er, yes.  My bad :)

Comment 13

[Shawn Wilsher :sdwilsh]

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

Comment 14

[Daniel Veditz [:dveditz]]

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.