Move execution from nsExternalAppHandler to nsDownload (b=858234, r=paolo)
authorMonica Chew <mmc@mozilla.com>
Mon, 27 May 2013 18:33:39 -0700
changeset 133930 b6cce1e412531ef22f5143f6b8d24359208afc30
parent 133929 ca43cd65708b6d2f47d9bad38745e8d40bc592bd
child 133931 a60ac0888ab178533c1a0069e740b4036353483b
push id28971
push usermchew@mozilla.com
push dateTue, 04 Jun 2013 16:23:51 +0000
treeherdermozilla-inbound@b6cce1e41253 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspaolo
bugs858234
milestone24.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Move execution from nsExternalAppHandler to nsDownload (b=858234, r=paolo)
toolkit/components/downloads/nsDownloadManager.cpp
toolkit/components/downloads/nsDownloadManager.h
toolkit/components/downloads/nsDownloadProxy.h
toolkit/components/downloads/nsIDownloadManager.idl
toolkit/content/tests/browser/common/mockTransfer.js
uriloader/base/nsITransfer.idl
uriloader/exthandler/nsExternalHelperAppService.cpp
uriloader/exthandler/nsExternalHelperAppService.h
uriloader/exthandler/nsIExternalHelperAppService.idl
--- a/toolkit/components/downloads/nsDownloadManager.cpp
+++ b/toolkit/components/downloads/nsDownloadManager.cpp
@@ -2519,16 +2519,23 @@ nsDownload::nsDownload() : mDownloadStat
                            mAutoResume(DONT_RESUME)
 {
 }
 
 nsDownload::~nsDownload()
 {
 }
 
+NS_IMETHODIMP nsDownload::SetSha256Hash(const nsACString& aHash) {
+  MOZ_ASSERT(NS_IsMainThread(), "Must call SetSha256Hash on main thread");
+  // This will be used later to query the application reputation service.
+  mHash = aHash;
+  return NS_OK;
+}
+
 #ifdef MOZ_ENABLE_GIO
 static void gio_set_metadata_done(GObject *source_obj, GAsyncResult *res, gpointer user_data)
 {
   GError *err = NULL;
   g_file_set_attributes_finish(G_FILE(source_obj), res, NULL, &err);
   if (err) {
 #ifdef DEBUG
     NS_DebugBreak(NS_DEBUG_WARNING, "Set file metadata failed: ", err->message, __FILE__, __LINE__);
@@ -2581,17 +2588,16 @@ nsDownload::SetState(DownloadState aStat
       // If we failed, then fall through to 'download finished'
       if (NS_SUCCEEDED(rv))
         break;
       mDownloadState = aState = nsIDownloadManager::DOWNLOAD_FINISHED;
     }
 #endif
     case nsIDownloadManager::DOWNLOAD_FINISHED:
     {
-      // Do what exthandler would have done if necessary
       nsresult rv = ExecuteDesiredAction();
       if (NS_FAILED(rv)) {
         // We've failed to execute the desired action.  As a result, we should
         // fail the download so the user can try again.
         (void)FailDownload(rv, nullptr);
         return rv;
       }
 
@@ -2923,16 +2929,18 @@ nsDownload::OnStatusChange(nsIWebProgres
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsDownload::OnStateChange(nsIWebProgress *aWebProgress,
                           nsIRequest *aRequest, uint32_t aStateFlags,
                           nsresult aStatus)
 {
+  MOZ_ASSERT(NS_IsMainThread(), "Must call OnStateChange in main thread");
+
   // We don't want to lose access to our member variables
   nsRefPtr<nsDownload> kungFuDeathGrip = this;
 
   // Check if we're starting a request; the NETWORK flag is necessary to not
   // pick up the START of *each* file but only for the whole request
   if ((aStateFlags & STATE_START) && (aStateFlags & STATE_IS_NETWORK)) {
     nsresult rv;
     nsCOMPtr<nsIHttpChannel> channel = do_QueryInterface(aRequest, &rv);
@@ -3182,20 +3190,22 @@ nsDownload::Finalize()
 
   // Make sure we do not automatically resume
   mAutoResume = DONT_RESUME;
 }
 
 nsresult
 nsDownload::ExecuteDesiredAction()
 {
-  // If we have a temp file and we have resumed, we have to do what the
-  // external helper app service would have done.
-  if (!mTempFile || !WasResumed())
+  // nsExternalHelperAppHandler is the only caller of AddDownload that sets a
+  // tempfile parameter. In this case, execute the desired action according to
+  // the saved mime info.
+  if (!mTempFile) {
     return NS_OK;
+  }
 
   // We need to bail if for some reason the temp file got removed
   bool fileExists;
   if (NS_FAILED(mTempFile->Exists(&fileExists)) || !fileExists)
     return NS_ERROR_FILE_NOT_FOUND;
 
   // Assume an unknown action is save to disk
   nsHandlerInfoAction action = nsIMIMEInfo::saveToDisk;
--- a/toolkit/components/downloads/nsDownloadManager.h
+++ b/toolkit/components/downloads/nsDownloadManager.h
@@ -416,12 +416,17 @@ private:
    * the download manager or restoring from a crash.
    *
    * DONT_RESUME: Don't automatically resume the download
    * AUTO_RESUME: Automaically resume the download
    */
   enum AutoResume { DONT_RESUME, AUTO_RESUME };
   AutoResume mAutoResume;
 
+  /**
+   * Stores the SHA-256 hash associated with the downloaded file.
+   */
+  nsAutoCString mHash;
+
   friend class nsDownloadManager;
 };
 
 #endif
--- a/toolkit/components/downloads/nsDownloadProxy.h
+++ b/toolkit/components/downloads/nsDownloadProxy.h
@@ -11,16 +11,21 @@
 #include "nsIPrefService.h"
 #include "nsIMIMEInfo.h"
 #include "nsIFileURL.h"
 #include "nsIDownloadManagerUI.h"
 
 #define PREF_BDM_SHOWWHENSTARTING "browser.download.manager.showWhenStarting"
 #define PREF_BDM_FOCUSWHENSTARTING "browser.download.manager.focusWhenStarting"
 
+// This class only exists because nsDownload cannot inherit from nsITransfer
+// directly. The reason for this is that nsDownloadManager (incorrectly) keeps
+// an nsCOMArray of nsDownloads, and nsCOMArray is only intended for use with
+// abstract classes. Using a concrete class that multiply inherits from classes
+// deriving from nsISupports will throw ambiguous base class errors.
 class nsDownloadProxy : public nsITransfer
 {
 public:
 
   nsDownloadProxy() { }
   virtual ~nsDownloadProxy() { }
 
   NS_DECL_ISUPPORTS
@@ -138,16 +143,22 @@ public:
 
   NS_IMETHODIMP OnSecurityChange(nsIWebProgress *aWebProgress,
                                  nsIRequest *aRequest, uint32_t aState)
   {
     NS_ENSURE_TRUE(mInner, NS_ERROR_NOT_INITIALIZED);
     return mInner->OnSecurityChange(aWebProgress, aRequest, aState);
   }
 
+  NS_IMETHODIMP SetSha256Hash(const nsACString& aHash)
+  {
+    NS_ENSURE_TRUE(mInner, NS_ERROR_NOT_INITIALIZED);
+    return mInner->SetSha256Hash(aHash);
+  }
+
 private:
   nsCOMPtr<nsIDownload> mInner;
 };
 
 NS_IMPL_ISUPPORTS3(nsDownloadProxy, nsITransfer,
                    nsIWebProgressListener, nsIWebProgressListener2)
 
 #endif
--- a/toolkit/components/downloads/nsIDownloadManager.idl
+++ b/toolkit/components/downloads/nsIDownloadManager.idl
@@ -109,18 +109,22 @@ interface nsIDownloadManager : nsISuppor
    *                  including MIME type and helper app when appropriate.
    *                  This parameter is optional.
    *
    * @param startTime Time when the download started
    *
    * @param aTempFile The location of a temporary file; i.e. a file in which
    *                  the received data will be stored, but which is not
    *                  equal to the target file. (will be moved to the real
-   *                  target by the caller, when the download is finished)
-   *                  May be null.
+   *                  target by the DownloadManager, when the download is
+   *                  finished). This will be null for all callers except for
+   *                  nsExternalHelperAppHandler. Addons should generally pass
+   *                  null for aTempFile. This will be moved to the real target
+   *                  by the download manager when the download is finished,
+   *                  and the action indicated by aMIMEInfo will be executed.
    *
    * @param aCancelable An object that can be used to abort the download.
    *                    Must not be null.
    *
    * @param aIsPrivate Used to determine the privacy status of the new download.
    *                   If true, the download is stored in a manner that leaves
    *                   no permanent trace outside of the current private session.
    *
--- a/toolkit/content/tests/browser/common/mockTransfer.js
+++ b/toolkit/content/tests/browser/common/mockTransfer.js
@@ -47,17 +47,18 @@ MockTransfer.prototype = {
   },
   onSecurityChange: function () {},
 
   /* nsIWebProgressListener2 */
   onProgressChange64: function () {},
   onRefreshAttempted: function () {},
 
   /* nsITransfer */
-  init: function () {}
+  init: function() {},
+  setSha256Hash: function() {}
 };
 
 // Create an instance of a MockObjectRegisterer whose methods can be used to
 // temporarily replace the default "@mozilla.org/transfer;1" object factory with
 // one that provides the mock implementation above. To activate the mock object
 // factory, call the "register" method. Starting from that moment, all the
 // transfer objects that are requested will be mock objects, until the
 // "unregister" method is called.
--- a/uriloader/base/nsITransfer.idl
+++ b/uriloader/base/nsITransfer.idl
@@ -5,17 +5,17 @@
 
 #include "nsIWebProgressListener2.idl"
 
 interface nsIURI;
 interface nsICancelable;
 interface nsIMIMEInfo;
 interface nsIFile;
 
-[scriptable, uuid(08b0b78c-6d7d-4d97-86c9-be5a695813c9)]
+[scriptable, uuid(b1c81100-9d66-11e2-9e96-0800200c9a66)]
 interface nsITransfer : nsIWebProgressListener2 {
 
     /**
      * Initializes the transfer with certain properties.  This function must
      * be called prior to accessing any properties on this interface.
      *
      * @param aSource The source URI of the transfer. Must not be null.
      *
@@ -52,16 +52,24 @@ interface nsITransfer : nsIWebProgressLi
     void init(in nsIURI aSource,
               in nsIURI aTarget,
               in AString aDisplayName,
               in nsIMIMEInfo aMIMEInfo,
               in PRTime startTime,
               in nsIFile aTempFile,
               in nsICancelable aCancelable,
               in boolean aIsPrivate);
+
+    /*
+     * Used to notify the transfer object of the hash of the downloaded file.
+     * Must be called on the main thread, only after the download has finished
+     * successfully.
+     * @param aHash The SHA-256 hash in raw bytes of the downloaded file.
+     */
+    void setSha256Hash(in ACString aHash);
 };
 
 %{C++
 /**
  * A component with this contract ID will be created each time a download is
  * started, and nsITransfer::Init will be called on it and an observer will be set.
  *
  * Notifications of the download progress will happen via
--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
@@ -136,18 +136,18 @@ enum {
 #ifdef PR_LOGGING
 PRLogModuleInfo* nsExternalHelperAppService::mLog = nullptr;
 #endif
 
 // Using level 3 here because the OSHelperAppServices use a log level
 // of PR_LOG_DEBUG (4), and we want less detailed output here
 // Using 3 instead of PR_LOG_WARN because we don't output warnings
 #undef LOG
-#define LOG(args) PR_LOG(mLog, 3, args)
-#define LOG_ENABLED() PR_LOG_TEST(mLog, 3)
+#define LOG(args) PR_LOG(nsExternalHelperAppService::mLog, 3, args)
+#define LOG_ENABLED() PR_LOG_TEST(nsExternalHelperAppService::mLog, 3)
 
 static const char NEVER_ASK_FOR_SAVE_TO_DISK_PREF[] =
   "browser.helperApps.neverAsk.saveToDisk";
 static const char NEVER_ASK_FOR_OPEN_FILE_PREF[] =
   "browser.helperApps.neverAsk.openFile";
 
 // Helper functions for Content-Disposition headers
 
@@ -1106,24 +1106,23 @@ nsExternalAppHandler::nsExternalAppHandl
                                            uint32_t aReason, bool aForceSave)
 : mMimeInfo(aMIMEInfo)
 , mWindowContext(aWindowContext)
 , mWindowToClose(nullptr)
 , mSuggestedFileName(aSuggestedFilename)
 , mForceSave(aForceSave)
 , mCanceled(false)
 , mShouldCloseWindow(false)
-, mReceivedDispositionInfo(false)
 , mStopRequestIssued(false)
-, mProgressListenerInitialized(false)
 , mReason(aReason)
 , mContentLength(-1)
 , mProgress(0)
 , mSaver(nullptr)
-, mKeepRequestAlive(false)
+, mDialogProgressListener(nullptr)
+, mTransfer(nullptr)
 , mRequest(nullptr)
 , mExtProtSvc(aExtProtSvc)
 {
 
   // make sure the extention includes the '.'
   if (!aTempFileExtension.IsEmpty() && aTempFileExtension.First() != '.')
     mTempFileExtension = PRUnichar('.');
   AppendUTF8toUTF16(aTempFileExtension, mTempFileExtension);
@@ -1139,48 +1138,33 @@ nsExternalAppHandler::nsExternalAppHandl
     PRUnichar(0x202c), // Pop Directional Formatting
     PRUnichar(0x202d), // Left-to-Right Override
     PRUnichar(0x202e)  // Right-to-Left Override
   };
   for (uint32_t i = 0; i < ArrayLength(unsafeBidiCharacters); ++i) {
     mSuggestedFileName.ReplaceChar(unsafeBidiCharacters[i], '_');
     mTempFileExtension.ReplaceChar(unsafeBidiCharacters[i], '_');
   }
-  
+
   // Make sure extension is correct.
   EnsureSuggestedFileName();
 
   mBufferSize = Preferences::GetUint("network.buffer.cache.size", 4096);
 }
 
 nsExternalAppHandler::~nsExternalAppHandler()
 {
   MOZ_ASSERT(!mSaver, "Saver should hold a reference to us until deleted");
 }
 
 NS_IMETHODIMP nsExternalAppHandler::SetWebProgressListener(nsIWebProgressListener2 * aWebProgressListener)
-{ 
-  // this call back means we've successfully brought up the 
-  // progress window so set the appropriate flag, even though
-  // aWebProgressListener might be null
-  
-  if (mReceivedDispositionInfo)
-    mProgressListenerInitialized = true;
-
-  // Go ahead and register the progress listener....
-  mWebProgressListener = aWebProgressListener;
-
-  // while we were bringing up the progress dialog, we actually finished processing the
-  // url. If that's the case then mStopRequestIssued will be true. We need to execute the
-  // operation since we are actually done now.
-  if (mStopRequestIssued && aWebProgressListener)
-  {
-    return ExecuteDesiredAction();
-  }
-
+{
+  // This is always called by nsHelperDlg.js. Go ahead and register the
+  // progress listener. At this point, we don't have mTransfer.
+  mDialogProgressListener = aWebProgressListener;
   return NS_OK;
 }
 
 NS_IMETHODIMP nsExternalAppHandler::GetTargetFile(nsIFile** aTarget)
 {
   if (mFinalFileDestination)
     *aTarget = mFinalFileDestination;
   else
@@ -1208,23 +1192,16 @@ NS_IMETHODIMP nsExternalAppHandler::GetT
 }
 
 NS_IMETHODIMP nsExternalAppHandler::GetContentLength(int64_t *aContentLength)
 {
   *aContentLength = mContentLength;
   return NS_OK;
 }
 
-NS_IMETHODIMP nsExternalAppHandler::CloseProgressWindow()
-{
-  // release extra state...
-  mWebProgressListener = nullptr;
-  return NS_OK;
-}
-
 void nsExternalAppHandler::RetargetLoadNotifications(nsIRequest *request)
 {
   // we are going to run the downloading of the helper app in our own little docloader / load group context. 
   // so go ahead and force the creation of a load group and doc loader for us to use...
   nsCOMPtr<nsIChannel> aChannel = do_QueryInterface(request);
   if (!aChannel)
     return;
 
@@ -1395,16 +1372,20 @@ nsresult nsExternalAppHandler::SetUpTemp
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = mSaver->SetObserver(this);
   if (NS_FAILED(rv)) {
     mSaver = nullptr;
     return rv;
   }
 
+  rv = mSaver->EnableSha256();
+  NS_ENSURE_SUCCESS(rv, rv);
+  LOG(("Enabled hashing"));
+
   rv = mSaver->SetTarget(mTempFile, false);
   NS_ENSURE_SUCCESS(rv, rv);
 
   return rv;
 }
 
 NS_IMETHODIMP nsExternalAppHandler::OnStartRequest(nsIRequest *request, nsISupports * aCtxt)
 {
@@ -1589,35 +1570,28 @@ NS_IMETHODIMP nsExternalAppHandler::OnSt
   // before this is irrelevant; override it
   if (mForceSave) {
     alwaysAsk = false;
     action = nsIMIMEInfo::saveToDisk;
   }
   
   if (alwaysAsk)
   {
-    // do this first! make sure we don't try to take an action until the user tells us what they want to do
-    // with it...
-    mReceivedDispositionInfo = false; 
-    mKeepRequestAlive = true;
-
     // invoke the dialog!!!!! use mWindowContext as the window context parameter for the dialog request
     mDialog = do_CreateInstance( NS_HELPERAPPLAUNCHERDLG_CONTRACTID, &rv );
     NS_ENSURE_SUCCESS(rv, rv);
 
     // this will create a reference cycle (the dialog holds a reference to us as
-    // nsIHelperAppLauncher), which will be broken in Cancel or
-    // CreateProgressListener.
+    // nsIHelperAppLauncher), which will be broken in Cancel or CreateTransfer.
     rv = mDialog->Show( this, mWindowContext, mReason );
 
     // what do we do if the dialog failed? I guess we should call Cancel and abort the load....
   }
   else
   {
-    mReceivedDispositionInfo = true; // no need to wait for a response from the user
 
     // We need to do the save/open immediately, then.
 #ifdef XP_WIN
     /* We need to see whether the file we've got here could be
      * executable.  If it could, we had better not try to open it!
      * We can skip this check, though, if we have a setting to open in a
      * helper app.
      * This code mirrors the code in
@@ -1652,18 +1626,18 @@ NS_IMETHODIMP nsExternalAppHandler::OnSt
     {
         rv = SaveToDisk(nullptr, false);
     }
   }
 
   return NS_OK;
 }
 
-// Convert error info into proper message text and send OnStatusChange notification
-// to the web progress listener.
+// Convert error info into proper message text and send OnStatusChange
+// notification to the dialog progress listener or nsITransfer implementation.
 void nsExternalAppHandler::SendStatusChange(ErrorType type, nsresult rv, nsIRequest *aRequest, const nsAFlatString &path)
 {
     nsAutoString msgId;
     switch(rv)
     {
     case NS_ERROR_OUT_OF_MEMORY:
         // No memory
         msgId.AssignLiteral("noMemory");
@@ -1719,36 +1693,38 @@ void nsExternalAppHandler::SendStatusCha
         case kLaunchError:
           msgId.AssignLiteral("launchError");
           break;
         }
         break;
     }
     PR_LOG(nsExternalHelperAppService::mLog, PR_LOG_ERROR,
         ("Error: %s, type=%i, listener=0x%p, rv=0x%08X\n",
-         NS_LossyConvertUTF16toASCII(msgId).get(), type, mWebProgressListener.get(), rv));
+         NS_LossyConvertUTF16toASCII(msgId).get(), type, mDialogProgressListener.get(), rv));
     PR_LOG(nsExternalHelperAppService::mLog, PR_LOG_ERROR,
         ("       path='%s'\n", NS_ConvertUTF16toUTF8(path).get()));
 
     // Get properties file bundle and extract status string.
     nsCOMPtr<nsIStringBundleService> stringService =
         mozilla::services::GetStringBundleService();
     if (stringService)
     {
         nsCOMPtr<nsIStringBundle> bundle;
         if (NS_SUCCEEDED(stringService->CreateBundle("chrome://global/locale/nsWebBrowserPersist.properties", getter_AddRefs(bundle))))
         {
             nsXPIDLString msgText;
             const PRUnichar *strings[] = { path.get() };
             if(NS_SUCCEEDED(bundle->FormatStringFromName(msgId.get(), strings, 1, getter_Copies(msgText))))
             {
-              if (mWebProgressListener)
+              if (mDialogProgressListener)
               {
                 // We have a listener, let it handle the error.
-                mWebProgressListener->OnStatusChange(nullptr, (type == kReadError) ? aRequest : nullptr, rv, msgText);
+                mDialogProgressListener->OnStatusChange(nullptr, (type == kReadError) ? aRequest : nullptr, rv, msgText);
+              } else if (mTransfer) {
+                mTransfer->OnStatusChange(nullptr, (type == kReadError) ? aRequest : nullptr, rv, msgText);
               }
               else
               if (XRE_GetProcessType() == GeckoProcessType_Default) {
                 // We don't have a listener.  Simply show the alert ourselves.
                 nsCOMPtr<nsIPrompt> prompter(do_GetInterface(mWindowContext));
                 nsXPIDLString title;
                 bundle->FormatStringFromName(NS_LITERAL_STRING("title").get(),
                                              strings,
@@ -1779,21 +1755,20 @@ nsExternalAppHandler::OnDataAvailable(ns
   {
     mProgress += count;
 
     nsCOMPtr<nsIStreamListener> saver = do_QueryInterface(mSaver);
     rv = saver->OnDataAvailable(request, aCtxt, inStr, sourceOffset, count);
     if (NS_SUCCEEDED(rv))
     {
       // Send progress notification.
-      if (mWebProgressListener)
-      {
-        mWebProgressListener->OnProgressChange64(nullptr, request, mProgress,
-                                                 mContentLength, mProgress,
-                                                 mContentLength);
+      if (mTransfer) {
+        mTransfer->OnProgressChange64(nullptr, request, mProgress,
+                                      mContentLength, mProgress,
+                                      mContentLength);
       }
     }
     else
     {
       // An error occurred, notify listener.
       nsAutoString tempFilePath;
       if (mTempFile)
         mTempFile->GetPath(tempFilePath);
@@ -1801,24 +1776,21 @@ nsExternalAppHandler::OnDataAvailable(ns
 
       // Cancel the download.
       Cancel(rv);
     }
   }
   return rv;
 }
 
-NS_IMETHODIMP nsExternalAppHandler::OnStopRequest(nsIRequest *request, nsISupports *aCtxt, 
+NS_IMETHODIMP nsExternalAppHandler::OnStopRequest(nsIRequest *request, nsISupports *aCtxt,
                                                   nsresult aStatus)
 {
   mStopRequestIssued = true;
 
-  if (!mKeepRequestAlive)
-    mRequest = nullptr;
-
   // Cancel if the request did not complete successfully.
   if (!mCanceled && NS_FAILED(aStatus))
   {
     // Send error notification.
     nsAutoString tempFilePath;
     if (mTempFile)
       mTempFile->GetPath(tempFilePath);
     SendStatusChange( kReadError, aStatus, request, tempFilePath );
@@ -1839,80 +1811,74 @@ nsExternalAppHandler::OnTargetChange(nsI
 {
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsExternalAppHandler::OnSaveComplete(nsIBackgroundFileSaver *aSaver,
                                      nsresult aStatus)
 {
-  // Free the reference that the saver keeps on us.
-  mSaver = nullptr;
-
   if (mCanceled)
     return NS_OK;
 
+  // Save the hash
+  nsresult rv = mSaver->GetSha256Hash(mHash);
+  // Free the reference that the saver keeps on us, even if we couldn't get the
+  // hash.
+  mSaver = nullptr;
+
   if (NS_FAILED(aStatus)) {
     nsAutoString path;
     mTempFile->GetPath(path);
     SendStatusChange(kWriteError, aStatus, nullptr, path);
     if (!mCanceled)
       Cancel(aStatus);
     return NS_OK;
   }
 
-  // Do what the user asked for
-  ExecuteDesiredAction();
-
-  // This nsITransfer object holds a reference to us (we are its observer), so
-  // we need to release the reference to break a reference cycle (and therefore
-  // to prevent leaking)
-  mWebProgressListener = nullptr;
+  // Notify the transfer object that we are done if the user has chosen an
+  // action. If the user hasn't chosen an action and InitializeDownload hasn't
+  // been called, the progress listener (nsITransfer) will be notified in
+  // CreateTransfer.
+  if (mTransfer) {
+    rv = NotifyTransfer();
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
 
   return NS_OK;
 }
 
-nsresult nsExternalAppHandler::ExecuteDesiredAction()
+nsresult nsExternalAppHandler::NotifyTransfer()
 {
-  nsresult rv = NS_OK;
-  if (mProgressListenerInitialized && !mCanceled)
-  {
-    rv = MoveFile(mFinalFileDestination);
-    if (NS_SUCCEEDED(rv))
-    {
-      nsHandlerInfoAction action = nsIMIMEInfo::saveToDisk;
-      mMimeInfo->GetPreferredAction(&action);
-      if (action == nsIMIMEInfo::useHelperApp ||
-          action == nsIMIMEInfo::useSystemDefault)
-      {
-        rv = OpenWithApplication();
-      }
-      else if(action == nsIMIMEInfo::saveToDisk)
-      {
-        mExtProtSvc->FixFilePermissions(mFinalFileDestination);
-      }
-    }
+  MOZ_ASSERT(NS_IsMainThread(), "Must notify on main thread");
+  MOZ_ASSERT(!mCanceled, "Can't notify if canceled or action "
+             "hasn't been chosen");
+  MOZ_ASSERT(mTransfer, "We must have an nsITransfer");
+
+  LOG(("Notifying progress listener"));
+
+  nsresult rv = mTransfer->SetSha256Hash(mHash);
+  NS_ENSURE_SUCCESS(rv, rv);
 
-    // Notify dialog that download is complete.
-    // By waiting till this point, it ensures that the progress dialog doesn't indicate
-    // success until we're really done.
-    if(mWebProgressListener)
-    {
-      if (!mCanceled)
-      {
-        mWebProgressListener->OnProgressChange64(nullptr, nullptr, mProgress, mContentLength, mProgress, mContentLength);
-      }
-      mWebProgressListener->OnStateChange(nullptr, nullptr,
-        nsIWebProgressListener::STATE_STOP |
-        nsIWebProgressListener::STATE_IS_REQUEST |
-        nsIWebProgressListener::STATE_IS_NETWORK, NS_OK);
-    }
-  }
+  rv = mTransfer->OnProgressChange64(nullptr, nullptr, mProgress,
+    mContentLength, mProgress, mContentLength);
+  NS_ENSURE_SUCCESS(rv, rv);
 
-  return rv;
+  rv = mTransfer->OnStateChange(nullptr, nullptr,
+    nsIWebProgressListener::STATE_STOP |
+    nsIWebProgressListener::STATE_IS_REQUEST |
+    nsIWebProgressListener::STATE_IS_NETWORK, NS_OK);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // This nsITransfer object holds a reference to us (we are its observer), so
+  // we need to release the reference to break a reference cycle (and therefore
+  // to prevent leaking)
+  mTransfer = nullptr;
+
+  return NS_OK;
 }
 
 NS_IMETHODIMP nsExternalAppHandler::GetMIMEInfo(nsIMIMEInfo ** aMIMEInfo)
 {
   *aMIMEInfo = mMimeInfo;
   NS_ADDREF(*aMIMEInfo);
   return NS_OK;
 }
@@ -1932,24 +1898,24 @@ NS_IMETHODIMP nsExternalAppHandler::GetS
 }
 
 nsresult nsExternalAppHandler::InitializeDownload(nsITransfer* aTransfer)
 {
   nsresult rv;
   
   nsCOMPtr<nsIURI> target;
   rv = NS_NewFileURI(getter_AddRefs(target), mFinalFileDestination);
-  if (NS_FAILED(rv)) return rv;
+  NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<nsIChannel> channel = do_QueryInterface(mRequest);
 
   rv = aTransfer->Init(mSourceUrl, target, EmptyString(),
                        mMimeInfo, mTimeDownloadStarted, mTempFile, this,
                        channel && NS_UsePrivateBrowsing(channel));
-  if (NS_FAILED(rv)) return rv;
+  NS_ENSURE_SUCCESS(rv, rv);
 
   // Now let's add the download to history
   nsCOMPtr<nsIDownloadHistory> dh(do_GetService(NS_DOWNLOADHISTORY_CONTRACTID));
   if (dh) {
     nsCOMPtr<nsIURI> referrer;
     nsCOMPtr<nsIChannel> channel = do_QueryInterface(mRequest);
     if (channel) {
       NS_GetReferrerFromChannel(channel, getter_AddRefs(referrer));
@@ -1958,41 +1924,48 @@ nsresult nsExternalAppHandler::Initializ
     if (channel && !NS_UsePrivateBrowsing(channel)) {
       dh->AddDownload(mSourceUrl, referrer, mTimeDownloadStarted, target);
     }
   }
 
   return rv;
 }
 
-nsresult nsExternalAppHandler::CreateProgressListener()
+nsresult nsExternalAppHandler::CreateTransfer()
 {
-  // we are back from the helper app dialog (where the user chooses to save or open), but we aren't
-  // done processing the load. in this case, throw up a progress dialog so the user can see what's going on...
+  // We are back from the helper app dialog (where the user chooses to save or
+  // open), but we aren't done processing the load. in this case, throw up a
+  // progress dialog so the user can see what's going on.
   // Also, release our reference to mDialog. We don't need it anymore, and we
   // need to break the reference cycle.
   mDialog = nullptr;
+  if (!mDialogProgressListener) {
+    NS_WARNING("The dialog should nullify the dialog progress listener");
+  }
   nsresult rv;
-  
-  nsCOMPtr<nsITransfer> tr = do_CreateInstance(NS_TRANSFER_CONTRACTID, &rv);
-  if (NS_SUCCEEDED(rv))
-    InitializeDownload(tr);
+
+  // We must be able to create an nsITransfer object. If not, it doesn't matter
+  // much that we can't launch the helper application or save to disk.
+  mTransfer = do_CreateInstance(NS_TRANSFER_CONTRACTID, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = InitializeDownload(mTransfer);
+  NS_ENSURE_SUCCESS(rv, rv);
 
-  if (tr)
-    tr->OnStateChange(nullptr, mRequest, nsIWebProgressListener::STATE_START |
-      nsIWebProgressListener::STATE_IS_REQUEST |
-      nsIWebProgressListener::STATE_IS_NETWORK, NS_OK);
+  rv = mTransfer->OnStateChange(nullptr, mRequest,
+    nsIWebProgressListener::STATE_START |
+    nsIWebProgressListener::STATE_IS_REQUEST |
+    nsIWebProgressListener::STATE_IS_NETWORK, NS_OK);
+  NS_ENSURE_SUCCESS(rv, rv);
 
-  // note we might not have a listener here if the QI() failed, or if
-  // there is no nsITransfer object, but we still call
-  // SetWebProgressListener() to make sure our progress state is sane
-  // NOTE: This will set up a reference cycle (this nsITransfer has us set up as
-  // its observer). This cycle will be broken in Cancel, CloseProgressWindow or
-  // OnStopRequest.
-  SetWebProgressListener(tr);
+  // While we were bringing up the progress dialog, we actually finished
+  // processing the url. If that's the case then mStopRequestIssued will be
+  // true and OnSaveComplete has been called.
+  if (mStopRequestIssued && !mSaver) {
+    return NotifyTransfer();
+  }
 
   mRequest = nullptr;
 
   return rv;
 }
 
 nsresult nsExternalAppHandler::SaveDestinationAvailable(nsIFile * aFile)
 {
@@ -2044,88 +2017,26 @@ void nsExternalAppHandler::RequestSaveDe
                                            aDefaultFile.get(),
                                            aFileExtension.get(),
                                            mForceSave);
   } else {
     SaveDestinationAvailable(rv == NS_OK ? fileToUse : nullptr);
   }
 }
 
-nsresult nsExternalAppHandler::MoveFile(nsIFile * aNewFileLocation)
-{
-  nsresult rv = NS_OK;
-  NS_ASSERTION(mStopRequestIssued, "uhoh, how did we get here if we aren't done getting data?");
- 
-  // if the on stop request was actually issued then it's now time to actually perform the file move....
-  if (mStopRequestIssued && aNewFileLocation)
-  {
-    // Unfortunately, MoveTo will fail if a file already exists at the user specified location....
-    // but the user has told us, this is where they want the file! (when we threw up the save to file dialog,
-    // it told them the file already exists and do they wish to over write it. So it should be okay to delete
-    // fileToUse if it already exists.
-    bool equalToTempFile = false;
-    bool filetoUseAlreadyExists = false;
-    aNewFileLocation->Equals(mTempFile, &equalToTempFile);
-    aNewFileLocation->Exists(&filetoUseAlreadyExists);
-    if (filetoUseAlreadyExists && !equalToTempFile)
-      aNewFileLocation->Remove(false);
-
-     // extract the new leaf name from the file location
-     nsAutoString fileName;
-     aNewFileLocation->GetLeafName(fileName);
-     nsCOMPtr<nsIFile> directoryLocation;
-     rv = aNewFileLocation->GetParent(getter_AddRefs(directoryLocation));
-     if (directoryLocation)
-     {
-       rv = mTempFile->MoveTo(directoryLocation, fileName);
-     }
-     if (NS_FAILED(rv))
-     {
-       // Send error notification.        
-       nsAutoString path;
-       aNewFileLocation->GetPath(path);
-       SendStatusChange(kWriteError, rv, nullptr, path);
-       Cancel(rv); // Cancel (and clean up temp file).
-     }
-#if defined(XP_OS2)
-     else
-     {
-       // tag the file with its source URI
-       nsCOMPtr<nsILocalFileOS2> localFileOS2 = do_QueryInterface(aNewFileLocation);
-       if (localFileOS2)
-       {
-         nsAutoCString url;
-         mSourceUrl->GetSpec(url);
-         localFileOS2->SetFileSource(url);
-       }
-     }
-#endif
-  }
-
-  return rv;
-}
-
 // SaveToDisk should only be called by the helper app dialog which allows
-// the user to say launch with application or save to disk. It doesn't actually 
-// perform the save, it just prompts for the destination file name. The actual save
-// won't happen until we are done downloading the content and are sure we've 
-// shown a progress dialog. This was done to simplify the 
-// logic that was showing up in this method. Internal callers who actually want
-// to preform the save should call ::MoveFile
-
+// the user to say launch with application or save to disk. It doesn't actually
+// perform the save, it just prompts for the destination file name.
 NS_IMETHODIMP nsExternalAppHandler::SaveToDisk(nsIFile * aNewFileLocation, bool aRememberThisPreference)
 {
   if (mCanceled)
     return NS_OK;
 
   mMimeInfo->SetPreferredAction(nsIMIMEInfo::saveToDisk);
 
-  // The helper app dialog has told us what to do.
-  mReceivedDispositionInfo = true;
-
   if (!aNewFileLocation) {
     if (mSuggestedFileName.IsEmpty())
       RequestSaveDestination(mTempLeafName, mTempFileExtension);
     else
     {
       nsAutoString fileExt;
       int32_t pos = mSuggestedFileName.RFindChar('.');
       if (pos >= 0)
@@ -2138,136 +2049,78 @@ NS_IMETHODIMP nsExternalAppHandler::Save
   } else {
     ContinueSave(aNewFileLocation);
   }
 
   return NS_OK;
 }
 nsresult nsExternalAppHandler::ContinueSave(nsIFile * aNewFileLocation)
 {
+  if (mCanceled)
+    return NS_OK;
+
   NS_PRECONDITION(aNewFileLocation, "Must be called with a non-null file");
 
   nsresult rv = NS_OK;
   nsCOMPtr<nsIFile> fileToUse = do_QueryInterface(aNewFileLocation);
   mFinalFileDestination = do_QueryInterface(fileToUse);
 
   // Move what we have in the final directory, but append .part
-  // to it, to indicate that it's unfinished.
-  // do not do that if we're already done
-  if (mFinalFileDestination && !mStopRequestIssued)
+  // to it, to indicate that it's unfinished.  Do not call SetTarget on the
+  // saver if we are done (Finish has been called) but OnSaverComplete has not
+  // been called.
+  if (mFinalFileDestination && mSaver && !mStopRequestIssued)
   {
     nsCOMPtr<nsIFile> movedFile;
     mFinalFileDestination->Clone(getter_AddRefs(movedFile));
     if (movedFile) {
       // Get the old leaf name and append .part to it
       nsAutoString name;
       mFinalFileDestination->GetLeafName(name);
       name.AppendLiteral(".part");
       movedFile->SetLeafName(name);
 
-      if (mSaver)
-      {
-        rv = mSaver->SetTarget(movedFile, true);
-        if (NS_FAILED(rv)) {
-          nsAutoString path;
-          mTempFile->GetPath(path);
-          SendStatusChange(kWriteError, rv, nullptr, path);
-          Cancel(rv);
-          return NS_OK;
-        }
+      rv = mSaver->SetTarget(movedFile, true);
+      if (NS_FAILED(rv)) {
+        nsAutoString path;
+        mTempFile->GetPath(path);
+        SendStatusChange(kWriteError, rv, nullptr, path);
+        Cancel(rv);
+        return NS_OK;
       }
 
       mTempFile = movedFile;
     }
   }
 
-  if (!mProgressListenerInitialized)
-    CreateProgressListener();
+  // The helper app dialog has told us what to do and we have a final file
+  // destination.
+  CreateTransfer();
 
   // now that the user has chosen the file location to save to, it's okay to fire the refresh tag
   // if there is one. We don't want to do this before the save as dialog goes away because this dialog
   // is modal and we do bad things if you try to load a web page in the underlying window while a modal
-  // dialog is still up. 
+  // dialog is still up.
   ProcessAnyRefreshTags();
 
   return NS_OK;
 }
 
 
-nsresult nsExternalAppHandler::OpenWithApplication()
-{
-  nsresult rv = NS_OK;
-  if (mCanceled)
-    return NS_OK;
-  
-  // we only should have gotten here if the on stop request had been fired already.
-
-  NS_ASSERTION(mStopRequestIssued, "uhoh, how did we get here if we aren't done getting data?");
-  // if a stop request was already issued then proceed with launching the application.
-  if (mStopRequestIssued)
-  {
-
-    // Note for the default value:
-    // Mac users have been very verbal about temp files being deleted on
-    // app exit - they don't like it - but we'll continue to do this on
-    // other platforms for now.
-    bool deleteTempFileOnExit =
-      Preferences::GetBool("browser.helperApps.deleteTempFileOnExit",
-#if !defined(XP_MACOSX)
-                           true);
-#else
-                           false);
-#endif
-
-    // See whether the channel has been opened in private browsing mode
-    NS_ASSERTION(mRequest, "This should never be called with a null request");
-    nsCOMPtr<nsIChannel> channel = do_QueryInterface(mRequest);
-    bool inPrivateBrowsing = channel && NS_UsePrivateBrowsing(channel);
-
-    // make the tmp file readonly so users won't edit it and lose the changes
-    // only if we're going to delete the file
-    if (deleteTempFileOnExit || inPrivateBrowsing)
-      mFinalFileDestination->SetPermissions(0400);
-
-    rv = mMimeInfo->LaunchWithFile(mFinalFileDestination);
-    if (NS_FAILED(rv))
-    {
-      // Send error notification.
-      nsAutoString path;
-      mFinalFileDestination->GetPath(path);
-      SendStatusChange(kLaunchError, rv, nullptr, path);
-      Cancel(rv); // Cancel, and clean up temp file.
-    }
-    // Always schedule files to be deleted at the end of the private browsing
-    // mode, regardless of the value of the pref.
-    else if (deleteTempFileOnExit) {
-      mExtProtSvc->DeleteTemporaryFileOnExit(mFinalFileDestination);
-    }
-    else if (inPrivateBrowsing) {
-      mExtProtSvc->DeleteTemporaryPrivateFileWhenPossible(mFinalFileDestination);
-    }
-  }
-
-  return rv;
-}
-
-// LaunchWithApplication should only be called by the helper app dialog which allows
-// the user to say launch with application or save to disk. It doesn't actually 
-// perform launch with application. That won't happen until we are done downloading
-// the content and are sure we've shown a progress dialog. This was done to simplify the 
-// logic that was showing up in this method. 
+// LaunchWithApplication should only be called by the helper app dialog which
+// allows the user to say launch with application or save to disk. It doesn't
+// actually perform launch with application.
 NS_IMETHODIMP nsExternalAppHandler::LaunchWithApplication(nsIFile * aApplication, bool aRememberThisPreference)
 {
   if (mCanceled)
     return NS_OK;
 
   // user has chosen to launch using an application, fire any refresh tags now...
   ProcessAnyRefreshTags(); 
   
-  mReceivedDispositionInfo = true; 
   if (mMimeInfo && aApplication) {
     PlatformLocalHandlerApp_t *handlerApp =
       new PlatformLocalHandlerApp_t(EmptyString(), aApplication);
     mMimeInfo->SetPreferredApplicationHandler(handlerApp);
   }
 
   // Now check if the file is local, in which case we won't bother with saving
   // it to a temporary directory and just launch it from where it is
@@ -2287,22 +2140,23 @@ NS_IMETHODIMP nsExternalAppHandler::Laun
     nsAutoString path;
     if (file)
       file->GetPath(path);
     // If we get here, an error happened
     SendStatusChange(kLaunchError, rv, nullptr, path);
     return rv;
   }
 
-  // Now that the user has elected to launch the downloaded file with a helper app, we're justified in
-  // removing the 'salted' name.  We'll rename to what was specified in mSuggestedFileName after the
-  // download is done prior to launching the helper app.  So that any existing file of that name won't
-  // be overwritten we call CreateUnique() before calling MoveFile().  Also note that we use the same
-  // directory as originally downloaded to so that MoveFile() just does an in place rename.
-
+  // Now that the user has elected to launch the downloaded file with a helper
+  // app, we're justified in removing the 'salted' name.  We'll rename to what
+  // was specified in mSuggestedFileName after the download is done prior to
+  // launching the helper app.  So that any existing file of that name won't be
+  // overwritten we call CreateUnique().  Also note that we use the same
+  // directory as originally downloaded so nsDownload can rename in place
+  // later.
   nsCOMPtr<nsIFile> fileToUse;
   (void) GetDownloadDirectory(getter_AddRefs(fileToUse));
 
   if (mSuggestedFileName.IsEmpty())
   {
     // Keep using the leafname of the temp file, since we're just starting a helper
     mSuggestedFileName = mTempLeafName;
   }
@@ -2313,18 +2167,17 @@ NS_IMETHODIMP nsExternalAppHandler::Laun
   fileToUse->Append(mSuggestedFileName);  
 #endif
 
   nsresult rv = fileToUse->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
   if(NS_SUCCEEDED(rv))
   {
     mFinalFileDestination = do_QueryInterface(fileToUse);
     // launch the progress window now that the user has picked the desired action.
-    if (!mProgressListenerInitialized)
-      CreateProgressListener();
+    CreateTransfer();
   }
   else
   {
     // Cancel the download and report an error.  We do not want to end up in
     // a state where it appears that we have a normal download that is
     // pointing to a file that we did not actually create.
     nsAutoString path;
     mTempFile->GetPath(path);
@@ -2335,28 +2188,31 @@ NS_IMETHODIMP nsExternalAppHandler::Laun
 }
 
 NS_IMETHODIMP nsExternalAppHandler::Cancel(nsresult aReason)
 {
   NS_ENSURE_ARG(NS_FAILED(aReason));
   // XXX should not ignore the reason
 
   mCanceled = true;
-  if (mSaver)
+  if (mSaver) {
     mSaver->Finish(aReason);
+    mSaver = nullptr;
+  }
 
   // Break our reference cycle with the helper app dialog (set up in
   // OnStartRequest)
   mDialog = nullptr;
 
   mRequest = nullptr;
 
   // Release the listener, to break the reference cycle with it (we are the
   // observer of the listener).
-  mWebProgressListener = nullptr;
+  mDialogProgressListener = nullptr;
+  mTransfer = nullptr;
 
   return NS_OK;
 }
 
 void nsExternalAppHandler::ProcessAnyRefreshTags()
 {
    // one last thing, try to see if the original window context supports a refresh interface...
    // Sometimes, when you download content that requires an external handler, there is
--- a/uriloader/exthandler/nsExternalHelperAppService.h
+++ b/uriloader/exthandler/nsExternalHelperAppService.h
@@ -277,22 +277,19 @@ protected:
   /**
    * This is set based on whether the channel indicates that a new window
    * was opened specifically for this download.  If so, then we
    * close it.
    */
   bool mShouldCloseWindow;
 
   /**
-   * have we received information from the user about how they want to
-   * dispose of this content
+   * True if a stop request has been issued.
    */
-  bool mReceivedDispositionInfo;
   bool mStopRequestIssued; 
-  bool mProgressListenerInitialized;
 
   bool mIsFileChannel;
 
   /**
    * One of the REASON_ constants from nsIHelperAppLauncherDialog. Indicates the
    * reason the dialog was shown (unknown content type, server requested it,
    * etc).
    */
@@ -319,34 +316,37 @@ protected:
   /**
    * This object handles saving the data received from the network to a
    * temporary location first, and then move the file to its final location,
    * doing all the input/output on a background thread.
    */
   nsCOMPtr<nsIBackgroundFileSaver> mSaver;
 
   /**
+   * Stores the SHA-256 hash associated with the file that we downloaded.
+   */
+  nsAutoCString mHash;
+
+  /**
    * Creates the temporary file for the download and an output stream for it.
    * Upon successful return, both mTempFile and mSaver will be valid.
    */
   nsresult SetUpTempFile(nsIChannel * aChannel);
   /**
    * When we download a helper app, we are going to retarget all load
    * notifications into our own docloader and load group instead of
    * using the window which initiated the load....RetargetLoadNotifications
    * contains that information...
    */
   void RetargetLoadNotifications(nsIRequest *request); 
   /**
-   * If the user tells us how they want to dispose of the content and
-   * we still haven't finished downloading while they were deciding,
-   * then create a progress listener of some kind so they know
-   * what's going on...
+   * Once the user tells us how they want to dispose of the content
+   * create an nsITransfer so they know what's going on...
    */
-  nsresult CreateProgressListener();
+  nsresult CreateTransfer();
 
 
   /* 
    * The following two functions are part of the split of SaveToDisk
    * to make it async, and works as following:
    *
    *    SaveToDisk    ------->   RequestSaveDestination
    *                                     .
@@ -375,33 +375,21 @@ protected:
    * After we're done prompting the user for any information, if the original
    * channel had a refresh url associated with it (which might point to a
    * "thank you for downloading" kind of page, then process that....It is safe
    * to invoke this method multiple times. We'll clear mOriginalChannel after
    * it's called and this ensures we won't call it again....
    */
   void ProcessAnyRefreshTags();
 
-  /** 
-   * An internal method used to actually move the temp file to the final
-   * destination once we done receiving data AND have showed the progress dialog
+  /**
+   * Notify our nsITransfer object that we are done with the download.
    */
-  nsresult MoveFile(nsIFile * aNewFileLocation);
-  /**
-   * An internal method used to actually launch a helper app given the temp file
-   * once we are done receiving data AND have showed the progress dialog.
-   * Uses the application specified in the mime info.
-   */
-  nsresult OpenWithApplication();
-  
-  /**
-   * Helper routine which peaks at the mime action specified by mMimeInfo
-   * and calls either MoveFile or OpenWithApplication
-   */
-  nsresult ExecuteDesiredAction();
+  nsresult NotifyTransfer();
+
   /**
    * Helper routine that searches a pref string for a given mime type
    */
   bool GetNeverAskFlagFromPref(const char * prefName, const char * aContentType);
 
   /**
    * Initialize an nsITransfer object for use as a progress object
    */
@@ -422,17 +410,27 @@ protected:
 
   /**
    * Closes the window context if it does not have a refresh header
    * and it never displayed content before the external helper app
    * service was invoked.
    */
   nsresult MaybeCloseWindow();
 
-  nsCOMPtr<nsIWebProgressListener2> mWebProgressListener;
+  /**
+   * Set in nsHelperDlgApp.js. This is always null after the user has chosen an
+   * action.
+   */
+  nsCOMPtr<nsIWebProgressListener2> mDialogProgressListener;
+  /**
+   * Set once the user has chosen an action. This is null after the download
+   * has been canceled or completes.
+   */
+  nsCOMPtr<nsITransfer> mTransfer;
+
   nsCOMPtr<nsIChannel> mOriginalChannel; /**< in the case of a redirect, this will be the pre-redirect channel. */
   nsCOMPtr<nsIHelperAppLauncherDialog> mDialog;
 
   /**
    * Keep request alive in case when helper non-modal dialog shown.
    * Thus in OnStopRequest the mRequest will not be set to null (it will be set to null further).
    */
   bool mKeepRequestAlive;
--- a/uriloader/exthandler/nsIExternalHelperAppService.idl
+++ b/uriloader/exthandler/nsIExternalHelperAppService.idl
@@ -91,24 +91,26 @@ interface nsIHelperAppLauncher : nsICanc
   readonly attribute nsIURI source;
 
   /**
    * The suggested name for this file
    */
   readonly attribute AString suggestedFileName;
 
   /**
-   * Called when we want to just save the content to a particular file.
-   * NOTE: This will release the reference to the nsIHelperAppLauncherDialog.
-   * @param aNewFileLocation Location where the content should be saved
+   * Saves the final destination of the file. Does not actually perform the
+   * save.
+   * NOTE: This will release the reference to the
+   * nsIHelperAppLauncherDialog.
    */
   void saveToDisk(in nsIFile aNewFileLocation, in boolean aRememberThisPreference);
 
   /**
-   * Use aApplication to launch with this content.
+   * Remembers that aApplication should be used to launch this content. Does
+   * not actually launch the application.
    * NOTE: This will release the reference to the nsIHelperAppLauncherDialog.
    * @param aApplication nsIFile corresponding to the location of the application to use.
    * @param aRememberThisPreference TRUE if we should remember this choice.
    */
   void launchWithApplication(in nsIFile aApplication, in boolean aRememberThisPreference);
 
   /**
    * Callback invoked by nsIHelperAppLauncherDialog::promptForSaveToFileAsync
@@ -121,22 +123,16 @@ interface nsIHelperAppLauncher : nsICanc
    * The following methods are used by the progress dialog to get or set
    * information on the current helper app launcher download.
    * This reference will be released when the download is finished (after the
    * listener receives the STATE_STOP notification).
    */
   void setWebProgressListener(in nsIWebProgressListener2 aWebProgressListener);
 
   /**
-   * when the stand alone progress window actually closes, it calls this method
-   * so we can release any local state...
-   */
-  void closeProgressWindow();
-
-  /**
    * The file we are saving to
    */
   readonly attribute nsIFile targetFile;
 
   /**
    * The executable-ness of the target file
    */
   readonly attribute boolean targetFileIsExecutable;