Bug 382388 - nsDownload::OnStateChange doesn't check aStatus for failure. r=sdwilsh, a=mconnor

toolkit/components/downloads/src/nsDownloadManager.cpp

@@ -1879,35 +1879,8 @@ 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;
-
-    (void)SetState(nsIDownloadManager::DOWNLOAD_FAILED);
-
-    // Get title for alert.
-    nsXPIDLString title;
-
-    nsCOMPtr<nsIStringBundle> bundle = mDownloadManager->mBundle;
-    bundle->GetStringFromName(NS_LITERAL_STRING("downloadErrorAlertTitle").get(),
-                              getter_Copies(title));
-
-    // Get Download Manager window, to be parent of alert.
-    nsresult rv;
-    nsCOMPtr<nsIWindowMediator> wm =
-      do_GetService(NS_WINDOWMEDIATOR_CONTRACTID, &rv);
-    NS_ENSURE_SUCCESS(rv, rv);
-    nsCOMPtr<nsIDOMWindowInternal> dmWindow;
-    wm->GetMostRecentWindow(NS_LITERAL_STRING("Download:Manager").get(),
-                            getter_AddRefs(dmWindow));
-
-    // Show alert.
-    nsCOMPtr<nsIPromptService> prompter =
-      do_GetService("@mozilla.org/embedcomp/prompt-service;1", &rv);
-    NS_ENSURE_SUCCESS(rv, rv);
-    prompter->Alert(dmWindow, title, aMessage);
-  }
-
+  if (NS_FAILED(aStatus))
+    return FailDownload(aStatus, aMessage);
   return NS_OK;
 }
 
@@ -1937,8 +1910,8 @@ nsDownload::OnStateChange(nsIWebProgress *aWebProgress,
         (void)SetState(nsIDownloadManager::DOWNLOAD_BLOCKED);
       }
     }
-  } else if (aStateFlags & STATE_STOP) {
-    if (IsFinishable()) {
+  } else if ((aStateFlags & STATE_STOP) && IsFinishable()) {
+    if (NS_SUCCEEDED(aStatus)) {
       // We can't completely trust the bytes we've added up because we might be
       // missing on some/all of the progress updates (especially from cache).
       // Our best bet is the file itself, but if for some reason it's gone, the
@@ -1969,6 +1942,9 @@ nsDownload::OnStateChange(nsIWebProgress *aWebProgress,
 #else
       (void)SetState(nsIDownloadManager::DOWNLOAD_FINISHED);
 #endif
+    } else {
+      // We failed for some unknown reason -- fail with a generic message
+      (void)FailDownload(aStatus, nsnull);
     }
   }
 
@@ -2340,3 +2316,42 @@ nsDownload::UpdateDB()
 
   return stmt->Execute();
 }
+
+nsresult
+nsDownload::FailDownload(nsresult aStatus, const PRUnichar *aMessage)
+{
+  // Grab the bundle before potentially losing our member variables
+  nsCOMPtr<nsIStringBundle> bundle = mDownloadManager->mBundle;
+
+  (void)SetState(nsIDownloadManager::DOWNLOAD_FAILED);
+
+  // Get title for alert.
+  nsXPIDLString title;
+  nsresult rv = bundle->GetStringFromName(
+    NS_LITERAL_STRING("downloadErrorAlertTitle").get(), getter_Copies(title));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // Get a generic message if we weren't supplied one
+  nsXPIDLString message;
+  message = aMessage;
+  if (message.IsEmpty()) {
+    rv = bundle->GetStringFromName(
+      NS_LITERAL_STRING("downloadErrorGeneric").get(), getter_Copies(message));
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  // Get Download Manager window to be parent of alert
+  nsCOMPtr<nsIWindowMediator> wm =
+    do_GetService(NS_WINDOWMEDIATOR_CONTRACTID, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+  nsCOMPtr<nsIDOMWindowInternal> dmWindow;
+  rv = wm->GetMostRecentWindow(NS_LITERAL_STRING("Download:Manager").get(),
+                               getter_AddRefs(dmWindow));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // Show alert
+  nsCOMPtr<nsIPromptService> prompter =
+    do_GetService("@mozilla.org/embedcomp/prompt-service;1", &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+  return prompter->Alert(dmWindow, title, message);
+}

toolkit/components/downloads/src/nsDownloadManager.h

@@ -272,6 +272,12 @@ protected:
    */
   PRBool IsFinished();
 
+  /**
+   * Fail a download because of a failure status and prompt the provided
+   * message or use a generic download failure message if nsnull.
+   */
+  nsresult FailDownload(nsresult aStatus, const PRUnichar *aMessage);
+
   nsDownloadManager *mDownloadManager;
   nsCOMPtr<nsIURI> mTarget;
 

toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties

@@ -5,7 +5,10 @@ notStarted=Not Started
 failed=Failed
 finished=Finished
 canceled=Canceled
+
 downloadErrorAlertTitle=Download Error
+downloadErrorGeneric=The download cannot be saved because an unknown error occurred.\n\nPlease try again.
+
 quitCancelDownloadsAlertTitle=Cancel All Downloads?
 quitCancelDownloadsAlertMsg=If you exit now, 1 download will be canceled. Are you sure you want to exit?
 quitCancelDownloadsAlertMsgMultiple=If you exit now, %S downloads will be canceled. Are you sure you want to exit?