Bug 880947 - Make CreateTransfer re-entrant, remove InitializeDownload. r=paolo
authorMonica Chew <mmc@mozilla.com>
Tue, 11 Jun 2013 09:07:44 -0700
changeset 134654 81dd6f59a13394169992ef21c694d01313d4cbf2
parent 134653 5dc87c816975fbe2b1adb5b392719be2d43a3059
child 134655 82add815a173f99d276b3ad9de2555070af0d021
push id29312
push usermchew@mozilla.com
push dateTue, 11 Jun 2013 16:08:01 +0000
treeherdermozilla-inbound@81dd6f59a133 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspaolo
bugs880947
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
Bug 880947 - Make CreateTransfer re-entrant, remove InitializeDownload. r=paolo
uriloader/exthandler/nsExternalHelperAppService.cpp
uriloader/exthandler/nsExternalHelperAppService.h
--- a/uriloader/exthandler/nsExternalHelperAppService.cpp
+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
@@ -1831,19 +1831,18 @@ nsExternalAppHandler::OnSaveComplete(nsI
     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 and InitializeDownload hasn't
-  // been called, the progress listener (nsITransfer) will be notified in
-  // CreateTransfer.
+  // 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);
   }
 
   return NS_OK;
 }
 
@@ -1893,27 +1892,46 @@ NS_IMETHODIMP nsExternalAppHandler::GetS
 }
 
 NS_IMETHODIMP nsExternalAppHandler::GetSuggestedFileName(nsAString& aSuggestedFileName)
 {
   aSuggestedFileName = mSuggestedFileName;
   return NS_OK;
 }
 
-nsresult nsExternalAppHandler::InitializeDownload(nsITransfer* aTransfer)
+nsresult nsExternalAppHandler::CreateTransfer()
 {
+  MOZ_ASSERT(NS_IsMainThread(), "Must create transfer on main thread");
+  // 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;
-  
+
+  // 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. Work on
+  // a local copy rather than mTransfer until we know we succeeded, to make it
+  // clearer that this function is re-entrant.
+  nsCOMPtr<nsITransfer> transfer = do_CreateInstance(
+    NS_TRANSFER_CONTRACTID, &rv);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // Initialize the download
   nsCOMPtr<nsIURI> target;
   rv = NS_NewFileURI(getter_AddRefs(target), mFinalFileDestination);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<nsIChannel> channel = do_QueryInterface(mRequest);
 
-  rv = aTransfer->Init(mSourceUrl, target, EmptyString(),
+  rv = transfer->Init(mSourceUrl, target, EmptyString(),
                        mMimeInfo, mTimeDownloadStarted, mTempFile, this,
                        channel && NS_UsePrivateBrowsing(channel));
   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;
@@ -1922,54 +1940,45 @@ nsresult nsExternalAppHandler::Initializ
       NS_GetReferrerFromChannel(channel, getter_AddRefs(referrer));
     }
 
     if (channel && !NS_UsePrivateBrowsing(channel)) {
       dh->AddDownload(mSourceUrl, referrer, mTimeDownloadStarted, target);
     }
   }
 
-  return rv;
-}
-
-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.
-  // 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");
+  // If we were cancelled since creating the transfer, just return. It is
+  // always ok to return NS_OK if we are cancelled. Callers of this function
+  // must call Cancel if CreateTransfer fails, but there's no need to cancel
+  // twice.
+  if (mCanceled) {
+    return NS_OK;
   }
-  nsresult rv;
-
-  // 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);
-
-  rv = mTransfer->OnStateChange(nullptr, mRequest,
+  rv = transfer->OnStateChange(nullptr, mRequest,
     nsIWebProgressListener::STATE_START |
     nsIWebProgressListener::STATE_IS_REQUEST |
     nsIWebProgressListener::STATE_IS_NETWORK, NS_OK);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  if (mCanceled) {
+    return NS_OK;
+  }
+
+  mRequest = nullptr;
+  // Finally, save the transfer to mTransfer.
+  mTransfer = transfer;
+  transfer = nullptr;
+
   // 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) {
+  if (mStopRequestIssued && !mSaver && mTransfer) {
     return NotifyTransfer();
   }
 
-  mRequest = nullptr;
-
   return rv;
 }
 
 nsresult nsExternalAppHandler::SaveDestinationAvailable(nsIFile * aFile)
 {
   if (aFile)
     ContinueSave(aFile);
   else
@@ -2089,17 +2098,22 @@ nsresult nsExternalAppHandler::ContinueS
       }
 
       mTempFile = movedFile;
     }
   }
 
   // The helper app dialog has told us what to do and we have a final file
   // destination.
-  CreateTransfer();
+  rv = CreateTransfer();
+  // If we fail to create the transfer, Cancel.
+  if (NS_FAILED(rv)) {
+    Cancel(rv);
+    return rv;
+  }
 
   // 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.
   ProcessAnyRefreshTags();
 
   return NS_OK;
@@ -2168,17 +2182,20 @@ 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.
-    CreateTransfer();
+    rv = CreateTransfer();
+    if (NS_FAILED(rv)) {
+      Cancel(rv);
+    }
   }
   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);
@@ -2214,17 +2231,17 @@ NS_IMETHODIMP nsExternalAppHandler::Canc
 }
 
 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 refresh header associated with the download. This refresh header points to a page
    // the content provider wants the user to see after they download the content. How do we
-   // pass this refresh information back to the caller? For now, try to get the refresh URI 
+   // pass this refresh information back to the caller? For now, try to get the refresh URI
    // interface. If the window context where the request originated came from supports this
    // then we can force it to process the refresh information (if there is any) from this channel.
    if (mWindowContext && mOriginalChannel)
    {
      nsCOMPtr<nsIRefreshURI> refreshHandler (do_GetInterface(mWindowContext));
      if (refreshHandler) {
         refreshHandler->SetupRefreshURI(mOriginalChannel);
      }
--- a/uriloader/exthandler/nsExternalHelperAppService.h
+++ b/uriloader/exthandler/nsExternalHelperAppService.h
@@ -331,25 +331,25 @@ protected:
    */
   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); 
+  void RetargetLoadNotifications(nsIRequest *request);
   /**
    * Once the user tells us how they want to dispose of the content
-   * create an nsITransfer so they know what's going on...
+   * create an nsITransfer so they know what's going on. If this fails, the
+   * caller MUST call Cancel.
    */
   nsresult CreateTransfer();
 
-
-  /* 
+  /*
    * The following two functions are part of the split of SaveToDisk
    * to make it async, and works as following:
    *
    *    SaveToDisk    ------->   RequestSaveDestination
    *                                     .
    *                                     .
    *                                     v
    *    ContinueSave  <-------   SaveDestinationAvailable
@@ -386,21 +386,16 @@ protected:
   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
-   */
-  nsresult InitializeDownload(nsITransfer*);
-  
-  /**
    * Helper routine to ensure mSuggestedFileName is "correct";
    * this ensures that mTempFileExtension only contains an extension when it
    * is different from mSuggestedFileName's extension.
    */
   void EnsureSuggestedFileName();
 
   typedef enum { kReadError, kWriteError, kLaunchError } ErrorType;
   /**