858234 - merge ExecuteDesiredAction paths into nsDownloadManager

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

This happens in both nsExternalAppHandler and nsDownloadManager. That's too many times!

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=837199#c12

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Try: https://tbpl.mozilla.org/?tree=Try&rev=822e49bcc104

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Try: https://tbpl.mozilla.org/?tree=Try&rev=2f2e93252b65

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

The browser_save_link-perwindowpb test passes locally for me, so I'm not sure quite what's going on.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

The breakages seem to be the same as this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=857427

But all of the runs on my try failed, so somehow this patch is triggering that orange more consistently.

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

The error is because the downloaded file is empty in the broken test, so nsIFile.Available() throws NS_BASE_STREAM_CLOSED. I can't reproduce manually, right-clicking "Save Link As" does save the correct file.

The mochitest is reading from an sjs which writes an HTTP response, and flakes with exactly the same failure in the other bug. The request handler does not call nsIHttpResponse.finish(), but I'm not sure that's required. Maybe it's not flushing?

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Duh, it is because the unittest is creating a mockTransfer instead of a real nsDownload (which moves the file now) and so of course the file is not getting saved -- though I don't know why it exists at 0 length. OK, so now all callers of mockTransferService that expect the file to exist may be broken.

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

Thankfully none of the other callers of mockTransfer service are expecting the file to exist. I wonder if the best way to fix this test is to make it count "Set-Cookie" and "Cookie" headers instead of checking the content of the downloaded file.

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Try: https://tbpl.mozilla.org/?tree=Try&rev=e6c1c69362de

Comment 10

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Now with qref. Try: https://tbpl.mozilla.org/?tree=Try&rev=bb68005dcd3b

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Paolo, you seem like the best choice of reviewer to me. If you disagree, please let me know and I will strive to find someone as good as you are :)

Try seems mostly green: https://tbpl.mozilla.org/?tree=Try&rev=78a760feaee4

This patch includes all the changes (I think) to nsExternalHelperAppService and friends required for nsDownloadManager to be able to perform the application reputation checks.

It changes nsExternalAppHandler::ExecuteDesiredAction to nsExternalAppHandler::NotifyProgressListener, and causes nsDownload::ExecuteDesiredAction to actually save the file to disk, or launch the application.

It also changes nsITransfer.idl to include onTransferComplete, which nsExternalAppHandler::NotifyProgressListener calls in order to notify nsDownload of the hash of the downloaded file. NotifyProgressListener and onTransferComplete must be called from the main thread, so that we can ensure that nsDownload receives the hash before querying application reputation (basically, all file hashing operations are called on the main thread).

It also changes test browser_save_link_perwindowpb.js to inspect cookie events rather than the contents of the downloaded file, since that test uses a mock transfer service (in lieu of nsDownload) that does not actually have a handle the downloaded file. I will flag the author of this file for review.

Tested=tbpl, manual

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 734909 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

Hi Josh and Ehsan,

Does one of you have time to check changes to browser/base/content/test/browser_save_link-perwindowpb.js?

This patch moves the logic for saving files to disk or launching helper applications to nsDownload, which means that the mock transfer service that this test touches won't actually be able to provide the file contents. Instead the test checks for set cookie events -- this is the dumbest way I could think of to fix the problem, suggestions welcome.

Btw, this test is orange anyway (bug 857427) and this patch will, hm, certainly cause the test not to flake in that way anymore.

Thanks,
Monica

Comment 13

[:Paolo Amadini]

Comment on attachment 734909 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

Review of attachment 734909 [details] [diff] [review]:
-----------------------------------------------------------------

This is a first pass review only, I expect one or two other passes to be necessary because we're working on old code that needs some care. I'll be able to get to the next pass later this week or next week.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +2585,5 @@
>  #endif
>      case nsIDownloadManager::DOWNLOAD_FINISHED:
>      {
> +      // Do what exthandler would have done. Does this work in the
> +      // cancel/resume case?

I expect this to work because we save the relevant choices for actions to the database, and reload them when we retry the download. But it's a good idea to test if this is actually true.

@@ +3190,5 @@
> +  // contentAreaUtils.internalPersist which uses nsIWebBrowserPersist to save
> +  // the file. Execute the desired action according to the mime info calculated
> +  // by the external helper app service.
> +  PR_LOG(dm_prlog, PR_LOG_DEBUG, ("ExecuteDesiredAction"));
> +  if (!mTempFile) {

This comment should be split in two parts, and explain the "hack" for which we only execute the action if we have mTempFile because nsExternalHelperAppHandler is the only caller of AddDownload that sets the aTempFile parameter.

I'd also update the IDL documentation of nsIDownloadManager::AddDownload to make this explicit. Add-ons should generally specify null for the aTempFile parameter.

@@ +3409,5 @@
>    if (!IsPaused() || !IsResumable())
>      return NS_ERROR_UNEXPECTED;
>  
>    nsresult rv;
> +  // We should use BackgroundFileSaver here.

I'd generally ask you to just ensure a bug is on file for the issues you find, there is no need for a code comment. In this case, there is already bug 836437 filed against the new API, and we won't probably fix this instance. You can remove the comment.

@@ +3446,5 @@
>    if (NS_FAILED(targetLocalFile->Clone(getter_AddRefs(clone))) ||
>        NS_FAILED(clone->GetFileSize(&fileSize)))
>      fileSize = 0;
>  
> +  // TODO(mmc): we need to calculate the hash of the resumed file.

Ditto, though you may want to ensure a bug is on file for the general issue of calculating the hash for resumed downloads.

::: uriloader/base/nsITransfer.idl
@@ +62,5 @@
> +     * Used to notify the transfer object of the hash of the downloaded file.
> +     * Must be called on the main thread.
> +     * @param aHash The SHA-256 hash in raw bytes of the downloaded file.
> +     */
> +    void onTransferComplete(in ACString aHash);

What about a more specific SetSha256Hash method?

Also, please specify in the IDL at which point this should be called.

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1867,5 @@
> +  LOG(("Notifying progress listener"));
> +  if (!mProgressListenerInitialized || mCanceled) {
> +    // This may happen if NotifyProgressListener is called from OnSaveComplete,
> +    // but InitializeDownload hasn't been called yet.
> +    return NS_OK;

If this can happen only when called by OnSaveComplete, then let's move this check there, and just assert inside NotifyProgressListener.

Let's also streamline the logic in this file a little bit, so that when reviewing I can feel comfortable in asserting that all the things happen in the right order in all cases.

For clarity, please rename the existing mReceivedDispositionInfo field to mActionChosen (the former name is misleading, makes me think of the Content-Disposition header).

Now, we use mWebProgressListener two times: before the action is chosen, but only for SendStatusChange to display errors and close the dialog, and after the action is chosen, for the nsITransfer.

So, I'd create a separate mTransfer field to hold the nsITransfer, and update SendStatusChange so that it is the only method that uses the old mWebProgressListener (that I'd rename to mDialogProgressListener) in addition to mTransfer.

At this point CreateProgressListener will be called when mActionChosen becomes true, probably making a few other state checks unnecessary. I'm also quite sure that mProgressListenerInitialized and mActionChosen will become true at the same time, so mProgressListenerInitialized can be removed and replaced by mActionChosen.

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

Thanks, Paolo. I like the logic simplification, I'll work on that and updating all the comments.

Comment 15

[(no longer active)]

Can you please ping me on IRC?  I'm not sure if I understand your test changes at all...

Comment 16

[(no longer active)]

Comment on attachment 734909 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

Review of attachment 734909 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the test part.

Comment 17

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Comment 18

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Paolo,

There's something wrong with this patch but I don't know if I'm going in the right direction with this comment anyway: 

> For clarity, please rename the existing mReceivedDispositionInfo field to
> mActionChosen (the former name is misleading, makes me think of the
> Content-Disposition header).
> 
> Now, we use mWebProgressListener two times: before the action is chosen, but
> only for SendStatusChange to display errors and close the dialog, and after
> the action is chosen, for the nsITransfer.
> 
> So, I'd create a separate mTransfer field to hold the nsITransfer, and
> update SendStatusChange so that it is the only method that uses the old
> mWebProgressListener (that I'd rename to mDialogProgressListener) in
> addition to mTransfer.

The mActionChosen change is done, but I don't think we can stop using mDialogProgressListener after the action is chosen. The reason for this is that nsExternalAppHandler::OnDataAvailable gets called at any time (before onSaveComplete) so we can notify the dialog window of progress, and we have to keep around mDialogProgressListener even if we already have mTransfer.

Is this still what you want?

Thanks,
Monica

Comment 19

[:Paolo Amadini]

Comment on attachment 736587 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

Review of attachment 736587 [details] [diff] [review]:
-----------------------------------------------------------------

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1160,5 @@
>  NS_IMETHODIMP nsExternalAppHandler::SetWebProgressListener(nsIWebProgressListener2 * aWebProgressListener)
>  { 
> +  // Go ahead and register the progress listener. aWebProgressListener may be
> +  // NULL if this is called from nsHelperAppDlg.js. We can't always expect a
> +  // transfer object here.

This is always called by nsHelperAppDlg.js, and always before saveToDisk/launchWithApplication, so we don't have mTransfer for sure, is this correct?

@@ +1212,5 @@
>  NS_IMETHODIMP nsExternalAppHandler::CloseProgressWindow()
>  {
>    // release extra state...
> +  mDialogProgressListener = nullptr;
> +  mTransfer = nullptr;

Feel free to remove CloseProgressWindow from the interface if it is never called.

@@ +1742,3 @@
>                {
>                  // We have a listener, let it handle the error.
> +                mDialogProgressListener->OnStatusChange(nullptr, (type == kReadError) ? aRequest : nullptr, rv, msgText);

As an else condition, if you have mTransfer, notify mTransfer->OnStatusChange.

@@ +1786,2 @@
>        {
> +        mDialogProgressListener->OnProgressChange64(nullptr, request, mProgress,

Here and in all the other places you should only notify mTransfer. In fact, all methods of mDialogProgressListener except for OnStatusChange are empty.

@@ +1885,4 @@
>  
> +  // If we get here, we'd better have an nsITransfer.
> +  NS_ENSURE_ARG_POINTER(mTransfer);
> +  NS_ENSURE_ARG_POINTER(mDialogProgressListener);

mDialogProgressListener may be null here, right?

::: uriloader/exthandler/nsExternalHelperAppService.h
@@ -361,2 @@
>     */
> -  nsresult MoveFile(nsIFile * aNewFileLocation);

Uhm, doesn't MoveFile still need to be implemented and called before notifying the download manager that the operation finished? When OnSaveComplete happens, the file can be in the system temporary directory or in the final directory with a ".part" extension. Does nsIDownloadManager handle both cases?

Comment 20

[[:mmc] Monica Chew (no longer reading bugmail)]

> ::: uriloader/exthandler/nsExternalHelperAppService.h
> @@ -361,2 @@
> >     */
> > -  nsresult MoveFile(nsIFile * aNewFileLocation);
> 
> Uhm, doesn't MoveFile still need to be implemented and called before
> notifying the download manager that the operation finished? When
> OnSaveComplete happens, the file can be in the system temporary directory or
> in the final directory with a ".part" extension. Does nsIDownloadManager
> handle both cases?

Yes, I think so. In the case where the file ends with .part, mTransfer is not initialized until the .part filename is known (call to CreateProgressListener after http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2295)

In the InitializeDownload call, mTempFile is pointing to the right thing, and nsDownload::ExecuteDesiredAction works by moving mTempFile to the final destination.

Comment 21

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Try: https://tbpl.mozilla.org/?tree=Try&rev=7e13f8d4e17d

Comment 22

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 739338 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

(In reply to Paolo Amadini [:paolo] from comment #19)
> >  { 
> > +  // Go ahead and register the progress listener. aWebProgressListener may be
> > +  // NULL if this is called from nsHelperAppDlg.js. We can't always expect a
> > +  // transfer object here.
> 
> This is always called by nsHelperAppDlg.js, and always before
> saveToDisk/launchWithApplication, so we don't have mTransfer for sure, is
> this correct?

Yes, fixed comment.
 
> @@ +1212,5 @@
> >  NS_IMETHODIMP nsExternalAppHandler::CloseProgressWindow()
> >  {
> >    // release extra state...
> > +  mDialogProgressListener = nullptr;
> > +  mTransfer = nullptr;
> 
> Feel free to remove CloseProgressWindow from the interface if it is never
> called.

Removed. Fixed logic around mTransfer/mDialogProgressListener and addressed comment 13.

I seem to have a non-deterministic memory leak in debug mode around BackgroundFileSaverStreamListener, but am puzzled because mSaver is not treated any differently in this patch. I'll look some more, but I think this is ready for review.

Thanks,
Monica

Comment 23

[:Paolo Amadini]

Comment on attachment 739338 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

Review of attachment 739338 [details] [diff] [review]:
-----------------------------------------------------------------

I want to thank you again for doing this, it is really helpful to make this code, that became more and more complicated over time, much more linear.

Now, even if there are many comments in this review (it was a pretty long one!), and it could look like I could have just made many small changes to the patch myself, I also appreciate a lot that someone else is going through the same changes to check for their correctness and validate them. So, thank you, a third time!

I'm skipping the part already handled in bug 857427 and going right into the C++ code itself.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +71,5 @@
>  #define PREF_BDM_RESUMEONWAKEDELAY "browser.download.manager.resumeOnWakeDelay"
>  #define PREF_BH_DELETETEMPFILEONEXIT "browser.helperApps.deleteTempFileOnExit"
>  
>  static const int64_t gUpdateInterval = 400 * PR_USEC_PER_MSEC;
> +static PRLogModuleInfo* dm_prlog = nullptr;

It is not clear to me if logging here is intended just as a temporary measure to help while debugging, in which case it can be fine to remove it when the patch is ready.

In fact, we are deprecating nsDownloadManager.cpp in favor of the new JavaScript API, so feel free not to spend too much time on this, though we should still keep this module in good shape with the changes we're making.

If you want to add logging to this module, it should follow the general naming conventions and be compiled conditionally, and optionally also use a local LOG macro:

http://mxr.mozilla.org/mozilla-central/search?string=PR_LOG
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#32

In addition, log lines should include at least the address of the nsDownload object to which they refer, otherwise they are less useful in production when there are several concurrent downloads.

@@ +2584,5 @@
>      }
>  #endif
>      case nsIDownloadManager::DOWNLOAD_FINISHED:
>      {
> +      // Do what exthandler would have done.

This comment doesn't make sense anymore unless you know the history of the code, you should either remove or expand it.

@@ +3187,5 @@
>  {
> +  // nsExternalHelperAppHandler is the only caller of AddDownload that sets a
> +  // tempfile parameter. In this case, we launch the desired action since
> +  // nsExternalHelperAppHandler does not.  Execute the desired action according
> +  // to the saved mime info.

"In this case, execute the desired action according to the saved MIME info."

::: toolkit/components/downloads/nsDownloadProxy.h
@@ +19,5 @@
> +// 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.

Ah, I see, that's why :-) Thanks for the explanation!

::: toolkit/components/downloads/nsIDownloadManager.idl
@@ +114,5 @@
>     * @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 DownloadManager, when the download is
> +   *                  finished). This will be null for all callers except for

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.

::: uriloader/base/nsITransfer.idl
@@ +62,5 @@
> +     * 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.

nit: whitespace at end of line, and indentation.

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1140,5 @@
>  
>  NS_IMETHODIMP nsExternalAppHandler::SetWebProgressListener(nsIWebProgressListener2 * aWebProgressListener)
>  { 
> +  // Go ahead and register the progress listener. aWebProgressListener will be
> +  // NULL if this is called from nsHelperAppDlg.js.

aWebProgressListener will be set to NULL when nsHelperAppDlg.js is closing. After this, launchWithApplication may be called immediately, or saveToDisk may be called on the next event loop tick.

In fact, when saveToDisk is called, the dialog has already been closed. So, if the download fails while we are displaying the file picker, it looks like we don't display any error message, is this correct?

@@ +1564,1 @@
>      mKeepRequestAlive = true;

No need to reinitialize mActionChosen to false here.

Also, the mKeepRequestAlive property can also go away entirely, because now we always go through either "Cancel" or "CreateTransfer", where we set mRequest to NULL. We don't mind if mRequest keeps it value when OnStopRequest is called.

@@ +1568,5 @@
>      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
> +    // CreateTransfer.

nit: indentation

@@ +1619,5 @@
>    return NS_OK;
>  }
>  
>  // Convert error info into proper message text and send OnStatusChange notification
> +// to the dialog progress listener.

"to the dialog progress listener or the nsITransfer implementation."

@@ +1827,5 @@
> +  // 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
> +  // SetWebProgressListener.
> +  if (!mCanceled && mActionChosen) {

Seems like we already checked mCanceled if we get here.

Also, it seems that we set mActionChosen to true just before calling CreateTransfer, and we only check it before calling NotifyTransfer.

So, we can probably get rid of mActionChosen and directly check whether mTransfer is null. Let's kill all those state variables! (renaming it in the previous iteration was still useful to understand this logic while refactoring)

@@ +1842,5 @@
> +  MOZ_ASSERT(!mCanceled && mActionChosen, "Can't notify if canceled or action "
> +             "hasn't been chosen");
> +
> +  // If we get here, we'd better have an nsITransfer.
> +  NS_ENSURE_ARG_POINTER(mTransfer);

MOZ_ASSERT, since now it's basically guaranteed.

@@ -1867,5 @@
> -        rv = OpenWithApplication();
> -      }
> -      else if(action == nsIMIMEInfo::saveToDisk)
> -      {
> -        mExtProtSvc->FixFilePermissions(mFinalFileDestination);

It looks like we only did this in this single code path, and not for all the other cases like restarted downloads, so it doesn't seem like removing this call will hurt.

We should probably get rid of the internal virtual FixFilePermissions function entirely. Feel free to either file a separate bug or do it here.

@@ +1929,4 @@
>    // Also, release our reference to mDialog. We don't need it anymore, and we
>    // need to break the reference cycle.
>    mDialog = nullptr;
> +  mDialogProgressListener = nullptr;

When we get here, this is already set to NULL by the dialog itself, unless some add-on replaces the dialog with a different implementation, right?

We may do something like "if (!mDialogProgressListener) NS_WARNING" instead.

@@ +1949,3 @@
>    // NOTE: This will set up a reference cycle (this nsITransfer has us set up as
> +  // its observer). This cycle will be broken in Cancel or NotifyTransfer.
> +  SetWebProgressListener(mTransfer);

We shouldn't call this anymore, just reserve this call to the progress dialog.

@@ +1953,5 @@
> +  // While we were bringing up the progress dialog, we actually finished
> +  // processing the url. If that's the case then mStopRequestIssued will be
> +  // true.
> +  if (mStopRequestIssued && !mCanceled && mActionChosen) {
> +    return NotifyTransfer();

It seems like mCanceled is always false here, except when entering though the new ContinueSave function (and we should probably introduce a new check for mCanceled at the beginning of ContinueSave like there is in SaveToDisk).

mActionChosen is also being removed as mentioned, it's always true at this point.

The remaining mStopRequestIssued check hides a race condition: in the rare case CreateTransfer is called between OnStopRequest and OnSaveComplete, we notify the transfer before the file is actually ready to be operated upon.

We may just check that OnSaveComplete was called, by checking that mSaver is NULL.

Comment 24

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Paolo,

You're welcome, and thanks as always for the fastidious review. They are always very helpful. I agree 100% on removing state variables whenever possible, so I'll try to implement all of your suggestions.

Sorry for the delay on this, I was out 2 days last week and will be out again for 2 days starting Friday, but I'll try my best to get another iteration out before I leave.

Monica

Comment 25

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Rebase (comments are unaddressed)

Comment 26

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Address Paolo's comments (except FixFilePermission, that is harder to remove than anticipated). Try: https://tbpl.mozilla.org/?tree=Try&rev=fe8cd0bc1df4

Not quite ready for review

Comment 27

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Move ExecuteDesiredAction to nsDownload

Hi Paolo,

My apologies for the long delay on this revision. Between PTO and other earlier deadlines I vastly underestimated the time I had to work on this.

I think I have addressed all your comments, except for removing FixFilePermissions (bug coming) because it is used elsewhere in a subclass, and removing the logging, which I will remove entirely before checking in.

> So, if the download fails while we are displaying the file picker, it looks like we don't display any error message, is this correct?

If DownloadManager knows there's a failure, we should get Cancel from http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#1824. Anyone else who knows there's a failure should be invoking nsICancelable on us as well.

I tested manually again and try looks sort of reasonable (errors are unrelated, I think). Please have another look.

Thanks,
Monica

Comment 28

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 755156 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

Review of attachment 755156 [details] [diff] [review]:
-----------------------------------------------------------------

Obviously I need to set my editor to highlight these, sorry.

::: toolkit/components/downloads/nsDownloadManager.cpp
@@ +3213,3 @@
>      return NS_OK;
> +  }
> + 

will remove whitespace

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +965,5 @@
>  void nsExternalHelperAppService::FixFilePermissions(nsIFile* aFile)
>  {
>    // This space intentionally left blank
>  }
> + 

will remove whitespace

@@ +1944,2 @@
>    nsresult rv;
> +   

will remove whitespace

Comment 29

[:Paolo Amadini]

Comment on attachment 755156 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

Review of attachment 755156 [details] [diff] [review]:
-----------------------------------------------------------------

> If DownloadManager knows there's a failure, we should get Cancel from
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/
> nsDownloadManager.cpp#1824. Anyone else who knows there's a failure should
> be invoking nsICancelable on us as well.

I referred to failures originating, for example, from the network, and I don't think nsICancelable is invoked in that case.

Anyway, missing a dialog box is not a big deal here. We're trying to reduce those notifications in any case :-)

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1158,5 @@
>  NS_IMETHODIMP nsExternalAppHandler::SetWebProgressListener(nsIWebProgressListener2 * aWebProgressListener)
>  { 
> +  // Go ahead and register the progress listener. aWebProgressListener will be
> +  // NULL if this is called from nsHelperAppDlg.js.
> +  mDialogProgressListener = aWebProgressListener;

This comment is still not true, can you update it?

@@ +1837,5 @@
>  
> +  // 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
> +  // SetWebProgressListener.

I missed this in the last iteration, but do you mean CreateTransfer instead of SetWebProgressListener? In any case, please make this comment as exact as possible, to avoid possible confusion to people trying to figure out what the code does.

@@ +1960,5 @@
> +  // processing the url. If that's the case then mStopRequestIssued will be
> +  // true.
> +  if (mStopRequestIssued) {
> +    return NotifyTransfer();
> +  }

Here we should determine whether OnSaveComplete was called, by checking if mSaver is NULL (and update the comment consequently).

@@ +2063,5 @@
>  
>    // 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 && mSaver && !mCanceled)

If mCanceled is true, we should also avoid calling CreateTransfer. We might just do like SaveToDisk does at the beginning:

  if (mCanceled)
    return NS_OK;

Also, I think the mStopRequestIssued check was correct here, in that we shouldn't call SetTarget on the saver if we called Finish on it, even if OnSaveComplete has not been called yet (it's worth annotating this in a comment).

r+ with these comments addressed and logging removed. Thanks!

Comment 30

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 755156 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

Review of attachment 755156 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for all your help on this, Paolo! Removed logging and re-testing before checking in.

Monica

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1158,5 @@
>  NS_IMETHODIMP nsExternalAppHandler::SetWebProgressListener(nsIWebProgressListener2 * aWebProgressListener)
>  { 
> +  // Go ahead and register the progress listener. aWebProgressListener will be
> +  // NULL if this is called from nsHelperAppDlg.js.
> +  mDialogProgressListener = aWebProgressListener;

Changed to 

// This is always called by nsHelperDlg.js. Go ahead and register the
// progress listener. At this point, we don't have mTransfer.

@@ +1837,5 @@
>  
> +  // 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
> +  // SetWebProgressListener.

Yes, thanks for catching that, updated comment.

@@ +1960,5 @@
> +  // processing the url. If that's the case then mStopRequestIssued will be
> +  // true.
> +  if (mStopRequestIssued) {
> +    return NotifyTransfer();
> +  }

Updated check and comment.

@@ +2063,5 @@
>  
>    // 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 && mSaver && !mCanceled)

Updated comment.

Comment 31

[[:mmc] Monica Chew (no longer reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/019382e24635

Comment 32

[[:mmc] Monica Chew (no longer reading bugmail)]

Filed bug 878144 on removing FixFilePermissions.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux mochitest-5 leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1623ffc069a0

https://tbpl.mozilla.org/php/getParsedLog.php?id=23644210&tree=Mozilla-Inbound

Comment 34

[Ryan VanderMeulen [:RyanVM]]

mochitest-bc leaks on OSX and Windows as well.
https://tbpl.mozilla.org/php/getParsedLog.php?id=23646457&tree=Mozilla-Inbound

Comment 35

[[:mmc] Monica Chew (no longer reading bugmail)]

I can't reproduce locally, or maybe I am misinterpreting mochitest-5 results. I think the problem may be that sometimes nsExternalHelperAppServer::Cancel is called and nsExternalHelperAppService::OnSaveComplete isn't (which is where mSaver=nullptr happens). So I am retrying with the change below.

Try: https://tbpl.mozilla.org/?tree=Try&rev=405b4b9a3d10

NS_IMETHODIMP nsExternalAppHandler::Cancel(nsresult aReason)
{
  NS_ENSURE_ARG(NS_FAILED(aReason));
  // XXX should not ignore the reason

  mCanceled = true;
  if (mSaver) {
    mSaver->Finish(aReason);
    mSaver = nullptr;
  }

Comment 36

[[:mmc] Monica Chew (no longer reading bugmail)]

That seemed to reduce the errors but not completely. There was a shortcut at the top of OnSaveComplete that may fail to set mSaver = nullptr if the hash can't be retrieved. I removed that check.

New try: https://tbpl.mozilla.org/?tree=Try&rev=606918a14af7

Comment 37

[[:mmc] Monica Chew (no longer reading bugmail)]

That seemed to work on all the debug builds that broke last time, so I checked in again.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6cce1e41253

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b6cce1e41253

Comment 39

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 755156 [details] [diff] [review]
Move ExecuteDesiredAction to nsDownload

Review of attachment 755156 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/downloads/nsDownloadProxy.h
@@ +19,5 @@
> +// 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.

Pointer to the followup you filed for this?

Comment 40

[:Paolo Amadini]

(In reply to :Ms2ger from comment #39)
> ::: toolkit/components/downloads/nsDownloadProxy.h
> @@ +19,5 @@
> > +// 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.
> 
> Pointer to the followup you filed for this?

I don't think we need to spend time in following up on this, because we're phasing
out DownloadManager.cpp entirely, in favor of the new JavaScript API for downloads.