Bug 899102 - nsExternalHelperAppService should notify when cancellation is complete. r=enn

toolkit/components/jsdownloads/src/DownloadCore.jsm

@@ -1901,14 +1901,8 @@ DownloadLegacySaver.prototype = {
       return this.copySaver.cancel.apply(this.copySaver, arguments);
     }
 
-    // Synchronously cancel the operation as soon as the object is connected.
+    // Cancel the operation as soon as the object is connected.
     this.deferCanceled.resolve();
-
-    // We don't necessarily receive status notifications after we call "cancel",
-    // but cancellation through nsICancelable should be synchronous, thus force
-    // the rejection of the execution promise immediately.
-    this.deferExecuted.reject(new DownloadError(Cr.NS_ERROR_FAILURE,
-                                                "Download canceled."));
   },
 
   /**

toolkit/components/jsdownloads/src/DownloadLegacy.js

@@ -95,6 +95,22 @@ DownloadLegacyTransfer.prototype = {
         download.saver.onTransferStarted(
                          aRequest,
                          this._cancelable instanceof Ci.nsIHelperAppLauncher);
+
+        // To handle asynchronous cancellation properly, we should hook up the
+        // handler only after we have been notified that the main request
+        // started.  We will wait until the main request stopped before
+        // notifying that the download has been canceled.
+        return download.saver.deferCanceled.promise.then(() => {
+          // Only cancel if the object executing the download is still running.
+          if (this._cancelable && !this._componentFailed) {
+            this._cancelable.cancel(Cr.NS_ERROR_ABORT);
+            if (this._cancelable instanceof Ci.nsIWebBrowserPersist) {
+              // This component will not send the STATE_STOP notification.
+              download.saver.onTransferFinished(aRequest, Cr.NS_ERROR_ABORT);
+              this._cancelable = null;
+            }
+          }
+        });
       }).then(null, Cu.reportError);
     } else if ((aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) &&
         (aStateFlags & Ci.nsIWebProgressListener.STATE_IS_NETWORK)) {
@@ -196,14 +212,6 @@ DownloadLegacyTransfer.prototype = {
       contentType: contentType,
       launcherPath: launcherPath
     }).then(function DLT_I_onDownload(aDownload) {
-      // Now that the saver is available, hook up the cancellation handler.
-      aDownload.saver.deferCanceled.promise.then(() => {
-        // Only cancel if the object executing the download is still running.
-        if (!this._componentFailed) {
-          aCancelable.cancel(Cr.NS_ERROR_ABORT);
-        }
-      }).then(null, Cu.reportError);
-
       // Legacy components keep partial data when they use a ".part" file.
       if (aTempFile) {
         aDownload.tryToKeepPartialData = true;

toolkit/components/jsdownloads/test/unit/common_test_Download.js

@@ -626,12 +626,6 @@ add_task(function test_cancel_midway_restart_tryToKeepPartialData()
   let download = yield promiseStartDownload_tryToKeepPartialData();
   yield download.cancel();
 
-  // This time-based solution is a workaround to avoid intermittent failures,
-  // and will be removed when bug 899102 is resolved.
-  if (gUseLegacySaver) {
-    yield promiseTimeout(250);
-  }
-
   do_check_true(download.stopped);
   do_check_true(download.hasPartialData);
 
@@ -685,12 +679,6 @@ add_task(function test_cancel_midway_restart_removePartialData()
   let download = yield promiseStartDownload_tryToKeepPartialData();
   yield download.cancel();
 
-  // This time-based solution is a workaround to avoid intermittent failures,
-  // and will be removed when bug 899102 is resolved.
-  if (gUseLegacySaver) {
-    yield promiseTimeout(250);
-  }
-
   do_check_true(download.hasPartialData);
   yield promiseVerifyContents(download.target.partFilePath, TEST_DATA_SHORT);
 
@@ -722,12 +710,6 @@ add_task(function test_cancel_midway_restart_tryToKeepPartialData_false()
   let download = yield promiseStartDownload_tryToKeepPartialData();
   yield download.cancel();
 
-  // This time-based solution is a workaround to avoid intermittent failures,
-  // and will be removed when bug 899102 is resolved.
-  if (gUseLegacySaver) {
-    yield promiseTimeout(250);
-  }
-
   download.tryToKeepPartialData = false;
 
   // The above property change does not affect existing partial data.
@@ -750,12 +732,6 @@ add_task(function test_cancel_midway_restart_tryToKeepPartialData_false()
 
   yield download.cancel();
 
-  // This time-based solution is a workaround to avoid intermittent failures,
-  // and will be removed when bug 899102 is resolved.
-  if (gUseLegacySaver) {
-    yield promiseTimeout(250);
-  }
-
   // The ".part" file should be deleted now that the download is canceled.
   do_check_false(download.hasPartialData);
   do_check_false(yield OS.File.exists(download.target.partFilePath));
@@ -962,12 +938,6 @@ add_task(function test_finalize_tryToKeepPartialData()
   let download = yield promiseStartDownload_tryToKeepPartialData();
   yield download.finalize();
 
-  // This time-based solution is a workaround to avoid intermittent failures,
-  // and will be removed when bug 899102 is resolved.
-  if (gUseLegacySaver) {
-    yield promiseTimeout(250);
-  }
-
   do_check_true(download.hasPartialData);
   do_check_true(yield OS.File.exists(download.target.partFilePath));
 
@@ -978,12 +948,6 @@ add_task(function test_finalize_tryToKeepPartialData()
   download = yield promiseStartDownload_tryToKeepPartialData();
   yield download.finalize(true);
 
-  // This time-based solution is a workaround to avoid intermittent failures,
-  // and will be removed when bug 899102 is resolved.
-  if (gUseLegacySaver) {
-    yield promiseTimeout(250);
-  }
-
   do_check_false(download.hasPartialData);
   do_check_false(yield OS.File.exists(download.target.partFilePath));
 });

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -1812,63 +1812,55 @@ NS_IMETHODIMP
 nsExternalAppHandler::OnSaveComplete(nsIBackgroundFileSaver *aSaver,
                                      nsresult aStatus)
 {
-  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;
+  if (!mCanceled) {
+    // Save the hash
+    (void)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;
+    }
   }
 
   // Notify the transfer object that we are done if the user has chosen an
   // action. If the user hasn't chosen an action, the progress listener
   // (nsITransfer) will be notified in CreateTransfer.
   if (mTransfer) {
-    rv = NotifyTransfer();
-    NS_ENSURE_SUCCESS(rv, rv);
+    NotifyTransfer(aStatus);
   }
 
   return NS_OK;
 }
 
-nsresult nsExternalAppHandler::NotifyTransfer()
+void nsExternalAppHandler::NotifyTransfer(nsresult aStatus)
 {
   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);
+  if (NS_SUCCEEDED(aStatus)) {
+    (void)mTransfer->SetSha256Hash(mHash);
+    (void)mTransfer->OnProgressChange64(nullptr, nullptr, mProgress,
+      mContentLength, mProgress, mContentLength);
+  }
 
-  rv = mTransfer->OnProgressChange64(nullptr, nullptr, mProgress,
-    mContentLength, mProgress, mContentLength);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  rv = mTransfer->OnStateChange(nullptr, nullptr,
+  (void)mTransfer->OnStateChange(nullptr, nullptr,
     nsIWebProgressListener::STATE_STOP |
     nsIWebProgressListener::STATE_IS_REQUEST |
-    nsIWebProgressListener::STATE_IS_NETWORK, NS_OK);
-  NS_ENSURE_SUCCESS(rv, rv);
+    nsIWebProgressListener::STATE_IS_NETWORK, aStatus);
 
   // 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)
+  // to prevent leaking).  We do this even if the previous calls failed.
   mTransfer = nullptr;
-
-  return NS_OK;
 }
 
 NS_IMETHODIMP nsExternalAppHandler::GetMIMEInfo(nsIMIMEInfo ** aMIMEInfo)
@@ -1966,7 +1958,7 @@ nsresult nsExternalAppHandler::CreateTransfer()
   // processing the url. If that's the case then mStopRequestIssued will be
   // true and OnSaveComplete has been called.
   if (mStopRequestIssued && !mSaver && mTransfer) {
-    return NotifyTransfer();
+    NotifyTransfer(NS_OK);
   }
 
   return rv;
@@ -2203,17 +2195,31 @@ NS_IMETHODIMP nsExternalAppHandler::LaunchWithApplication(nsIFile * aApplication
 NS_IMETHODIMP nsExternalAppHandler::Cancel(nsresult aReason)
 {
   NS_ENSURE_ARG(NS_FAILED(aReason));
-  // XXX should not ignore the reason
 
+  if (mCanceled) {
+    return NS_OK;
+  }
   mCanceled = true;
+
   if (mSaver) {
+    // We are still writing to the target file.  Give the saver a chance to
+    // close the target file, then notify the transfer object if necessary in
+    // the OnSaveComplete callback.
     mSaver->Finish(aReason);
     mSaver = nullptr;
-  } else if (mStopRequestIssued && mTempFile) {
-    // This branch can only happen when the user cancels the helper app dialog
-    // when the request has completed. The temp file has to be removed here,
-    // because mSaver has been released at that time with the temp file left.
-    (void)mTempFile->Remove(false);
+  } else {
+    if (mStopRequestIssued && mTempFile) {
+      // This branch can only happen when the user cancels the helper app dialog
+      // when the request has completed. The temp file has to be removed here,
+      // because mSaver has been released at that time with the temp file left.
+      (void)mTempFile->Remove(false);
+    }
+
+    // Notify the transfer object that the download has been canceled, if the
+    // user has already chosen an action and we didn't notify already.
+    if (mTransfer) {
+      NotifyTransfer(aReason);
+    }
   }
 
   // Break our reference cycle with the helper app dialog (set up in
@@ -2225,7 +2231,6 @@ NS_IMETHODIMP nsExternalAppHandler::Cancel(nsresult aReason)
   // Release the listener, to break the reference cycle with it (we are the
   // observer of the listener).
   mDialogProgressListener = nullptr;
-  mTransfer = nullptr;
 
   return NS_OK;
 }

uriloader/exthandler/nsExternalHelperAppService.h

@@ -373,9 +373,14 @@ protected:
   void ProcessAnyRefreshTags();
 
   /**
-   * Notify our nsITransfer object that we are done with the download.
+   * Notify our nsITransfer object that we are done with the download.  This is
+   * always called after the target file has been closed.
+   *
+   * @param aStatus
+   *        NS_OK for success, or a failure code if the download failed.
+   *        A partially downloaded file may still be available in this case.
    */
-  nsresult NotifyTransfer();
+  void NotifyTransfer(nsresult aStatus);
 
   /**
    * Helper routine that searches a pref string for a given mime type