384219 - Download manager doesn't fail downloads properly

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Shawn Wilsher :sdwilsh]

In nsDownload::OnStatusChange, we have the steps to failing a download in the wrong order.

It needs to be in the same/similar order as here:
http://mxr.mozilla.org/seamonkey/source/toolkit/components/downloads/src/nsDownloadManager.cpp#667

Comment 1

[Ed Lee :Mardak]

Attachment: v1

Refactor the download removal and notification process to keep the correct removal process for dl-(done|cancel|failed)

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 268542 [details] [diff] [review]
v1

>? toolkit/components/downloads/src/.nsDownloadManager.cpp.swp
>? toolkit/components/downloads/src/.nsDownloadManager.h.swp
Hope you saved first ;)

>+inline void
>+nsDownloadManager::RemoveDownloadAndNotify(nsDownload* const &aDownload,
>+    const DownloadState &aState, char* const &aTopic) {
eww... how about this:
inline nsresult
nsDownloadManager::RemoveAndNotify(nsDownload *aDownload, DownloadState aState, const char *aTopic) {

>+  // This has to be done in this exact order to not mess up our invariants
>+  // 1) when the state changed listener is dispatched, it must no longer be
>+  //    an active download.
>+  // 2) when the observer is dispatched, the same conditions for 1 must be
>+  //    true as well as the state being up to date.
>+  mCurrentDownloads.RemoveObject(aDownload);
>+  aDownload->SetState(aState);
>+  mObserverService->NotifyObservers(aDownload, aTopic, nsnull);
>+}
(void)mCurrentDownloads.RemoveObject(aDownload);
nsresult rv = aDownload->SetState(aState);
if (NS_FAILED(rv)) return rv;
(void)mObserverService->NotifyObservers(aDownload, aTopic, nsnull);
return NS_OK;

> NS_IMETHODIMP
> nsDownload::OnStatusChange(nsIWebProgress *aWebProgress,
>                            nsIRequest *aRequest, nsresult aStatus,
>                            const PRUnichar *aMessage)
> {   
>   if (NS_FAILED(aStatus)) {
>-    SetState(nsIDownloadManager::DOWNLOAD_FAILED);
>-    mDownloadManager->mObserverService->NotifyObservers(this, "dl-failed", nsnull);
>-
>-    mDownloadManager->RemoveDownloadFromCurrent(this);
>+    mDownloadManager->RemoveDownloadAndNotify(this, nsIDownloadManager::DOWNLOAD_FAILED, "dl-failed");
cast to void please when disregarding the return variable
(void)mDownloadManager->...

>-      // We can safely remove it from the current downloads
>-      mDownloadManager->RemoveDownloadFromCurrent(this);
>+      DownloadState state = nsIDownloadManager::DOWNLOAD_FINISHED;
>+      if (mDownloadState == nsIXPInstallManagerUI::INSTALL_INSTALLING)
>+        state = nsIXPInstallManagerUI::INSTALL_FINISHED;
>+      mDownloadManager->RemoveDownloadAndNotify(this, state, "dl-done");
Disregard the XPInstall stuff.  That is being removed in Bug 383224.  Also, cast to void as before.

Comment 3

[Ed Lee :Mardak]

Attachment: v2

(In reply to comment #2)
> eww... how about this:
> inline nsresult
> nsDownloadManager::RemoveAndNotify(nsDownload *aDownload, DownloadState aState,
> const char *aTopic) {

Ok. No inline though; hopefully the compiler will "do the right thing" as appropriate.

Renamed to FinishDownload because it's.. finishing the download ;), and it'll be used to break the mCancelable cycle for Bug 384180.

> (void)mCurrentDownloads.RemoveObject(aDownload);
> nsresult rv = aDownload->SetState(aState);
> if (NS_FAILED(rv)) return rv;
> (void)mObserverService->NotifyObservers(aDownload, aTopic, nsnull);
> return NS_OK;

ok + whitespace

> >+    mDownloadManager->RemoveDownloadAndNotify(this, nsIDownloadManager::DOWNLOAD_FAILED, "dl-failed");
> cast to void please when disregarding the return variable
> (void)mDownloadManager->...

done

> >+      if (mDownloadState == nsIXPInstallManagerUI::INSTALL_INSTALLING)
> >+        state = nsIXPInstallManagerUI::INSTALL_FINISHED;
> Disregard the XPInstall stuff. Also, cast to void as before.

yup

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 268555 [details] [diff] [review]
v2

This I like :)

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 268555 [details] [diff] [review]
v2

per mconnor, r from me is sufficient for this bug.

Comment 6

[Shawn Wilsher :sdwilsh]

I'm not sure if this is really testable (failing a download that is).  Perhaps Waldo can shed some light on this...

Comment 7

[Shawn Wilsher :sdwilsh]

Note: if Jeff knows how to make a test for this, you'll have to do that as well.

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.86; previous revision: 1.85
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.27; previous revision: 1.26