Closed Bug 740956 Opened 9 years ago Closed 9 years ago

Cancelling uploads to Dropbox or YouSendIt does not work, and is unavailable after first attempt

Categories

(Thunderbird :: Message Compose Window, defect)

x86
Windows 7
defect
Not set
normal

Tracking

(thunderbird13 fixed)

RESOLVED FIXED
Thunderbird 14.0
Tracking Status
thunderbird13 --- fixed

People

(Reporter: mconley, Assigned: Bienvenu)

References

Details

Attachments

(4 files, 5 obsolete files)

Using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20120330 Thunderbird/13.0a2

Steps to reproduce for Dropbox:

1)  Create both a Dropbox Filelink account.
2)  Start uploading a large file to Dropbox, and then cancel the upload.

What happens?

The icon reverts back to the non-uploading icon, but the upload continues in the background, and the URL is inserted upon completion.

What's expected?

The upload should have been cancelled.


Steps to reproduce for YouSendIt:

1)  Create a YouSendIt Filelink account
2)  Upload a large file to YouSendIt, and right-click on the attachment item to cancel it

What happens?

There's not Cancel option available for the uploading file.

What's expected?

There should be a cancel menuitem there...
Blocks: BigFiles
presumably item.uploading isn't set for yousendit uploads?
cancel is available for yousendit (I tried it here). It doesn't seem to work; I believe that's because the yousendit uploader doesn't implement cancel. I can fix that.
I think cancel isn't enabled after the first attempt; it doesn't have to do with yousendit per se, I don't think. I have a fix for that, I believe. I'm still working on the cancel stuff.
Attached patch get cancel working (obsolete) — Splinter Review
this gets cancel basically working, at least to the extent that we cancel the channel. The compose window still seems to think that the upload succeeded, but I'm pretty sure we're cancelling the channel now.
Assignee: nobody → dbienvenu
David:

I just noticed that for both YouSendIt and Dropbox, we use:

req.channel.cancel();

Passing zero arguments.  Should we not pass some error status, like "NS_BINDING_ABORTED"?  (http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/public/nsIRequest.idl#77)

-Mike
(In reply to Mike Conley (:mconley) from comment #5)

> Passing zero arguments.  Should we not pass some error status, like
> "NS_BINDING_ABORTED"? 

thx, that might be why the compose window thinks the upload succeeded. I'll add that to the patch.
this should get it working all the way through, and stops the front end from telling you that there was an error, if you explicitly cancel.
Attachment #611092 - Attachment is obsolete: true
Attachment #611507 - Flags: review?(mconley)
Fixes the UI to allow us to re-upload a file that has been cancelled.
Attached patch Tests for all of the above. (obsolete) — Splinter Review
And tests!  Huzzah!
Comment on attachment 611507 [details] [diff] [review]
get cancel, and don't alert the user

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

One little problem I noticed - we don't seem to do any cancellation when cancelling one of a group of uploads.

STR:

1)  Upload a series of files in one shot.
2)  Try cancelling any of the files that are NOT the first to be uploaded

It's possible that the channels are actually being cancelled and the UI is not responding properly.  Can you confirm?

::: mail/components/cloudfile/nsDropbox.js
@@ +199,5 @@
>        this._uploader.cancel();
>      }
>      else {
>        for (let i = 0; i < this._uploads.length; i++)
> +        if (this._uploads[i].file.equals(aFile)) {

I don't see any cancellation happening in this else clause - and manual testing seems to indicate that when we have multiple files being uploaded, we can only cancel the first one.

Same goes for nsYouSendIt.
Comment on attachment 611801 [details] [diff] [review]
Make cancelling and uploading repeatable

makes sense.
Attachment #611801 - Flags: review+
(In reply to Mike Conley (:mconley) from comment #10)
> 
> STR:
> 
> 1)  Upload a series of files in one shot.
> 2)  Try cancelling any of the files that are NOT the first to be uploaded
uploads are serialized, so the code simply removes the upload request if it hasn't been started. We most likely need to update the UI by telling it that the request was cancelled, even though it hasn't started.
> 
> It's possible that the channels are actually being cancelled

there shouldn't be any channels for uploads that haven't started yet.
(In reply to David :Bienvenu from comment #12)
> (In reply to Mike Conley (:mconley) from comment #10)
> > 
> 
> there shouldn't be any channels for uploads that haven't started yet.

Ah, of course.

> uploads are serialized, so the code simply removes the upload request if it
> hasn't been started. We most likely need to update the UI by telling it that
> the request was cancelled, even though it hasn't started.

Cool.  I can take care of that, if you'd like.  I'll write some tests while I'm at it.
Comment on attachment 611507 [details] [diff] [review]
get cancel, and don't alert the user

I'm happy with this.
Attachment #611507 - Flags: review?(mconley) → review+
same as 611507, w/ addition of notifying front end of cancels for uploads that haven't started.
Attachment #611507 - Attachment is obsolete: true
Attachment #611830 - Flags: review?(mconley)
Attached patch Tests for the above. (obsolete) — Splinter Review
Wow, that took forever, sorry about that.

Before I ask for review, going to push this to try and let it back overnight.  I'm adding some timer-y things, and I want some assurance that I haven't introduced some random oranges.
Attachment #611802 - Attachment is obsolete: true
Blocks: 738299
Attached patch Tests v3 (obsolete) — Splinter Review
Disentangled from my patch for bug 738299.
Attachment #611953 - Attachment is obsolete: true
Blocks: 739986
Comment on attachment 611830 [details] [diff] [review]
fix for cancelling uploads that haven't started

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

This looks reasonable to me.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +727,5 @@
>          }
>  
>          let bucket = document.getElementById("attachmentBucket");
>          for (let [,item] in Iterator(bucket.selectedItems)) {
> +          if (item && item.uploading)

I'm just curious - in what cases is item not defined or null?
Attachment #611830 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) from comment #18)
> 
> I'm just curious - in what cases is item not defined or null?

After we do the first cancel, then the code failed, presumably because item.uploading threw an exception. Seems like selectedItems shouldn't return something bogus, but I didn't drill down on it.

patch landed on trunk - http://hg.mozilla.org/comm-central/rev/6d73f1858352
Comment on attachment 611801 [details] [diff] [review]
Make cancelling and uploading repeatable

We certainly want to be able to cancel uploads, so a? for TB 13.
Attachment #611801 - Flags: approval-comm-aurora?
Comment on attachment 611830 [details] [diff] [review]
fix for cancelling uploads that haven't started

Same as above.
Attachment #611830 - Flags: approval-comm-aurora?
Attachment #611830 - Flags: approval-comm-aurora? → approval-comm-aurora+
Attachment #611801 - Flags: approval-comm-aurora? → approval-comm-aurora+
Attachment 611830 [details] [diff] committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/45bc09d75c4f

Attachment 611803 [details] committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/e7c41caf1f08
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 14.0
Attached patch Tests v4Splinter Review
David:

Ok, I've cleaned these up a fair bit.  Can you take them for a spin on your Windows box?  I'm having troubles getting my Windows build going, over here...

Thanks,

-Mike
Attachment #611958 - Attachment is obsolete: true
Attachment #612197 - Flags: review?(dbienvenu)
Comment on attachment 612197 [details] [diff] [review]
Tests v4

good job! I'm more than a little worried about intermittent oranges due to machine speed differences so we'll have to keep an eye out for that. Great to have tests for this, though.
Attachment #612197 - Flags: review?(dbienvenu) → review+
Comment on attachment 612197 [details] [diff] [review]
Tests v4

Pretty safe for Aurora.  The only risk is introducing intermittent oranges.
Attachment #612197 - Flags: approval-comm-aurora?
Comment on attachment 612197 [details] [diff] [review]
Tests v4

I'll approve for aurora, but I would love 24 hours of cycling on tinderbox trunk before landing this on aurora.
Attachment #612197 - Flags: approval-comm-aurora? → approval-comm-aurora+
(In reply to David :Bienvenu from comment #27)
> Comment on attachment 612197 [details] [diff] [review]
> Tests v4
> 
> I'll approve for aurora, but I would love 24 hours of cycling on tinderbox
> trunk before landing this on aurora.

I think that's wise.  Agreed.
David:

I haven't seen any oranges from these tests pop up.  In fact, I'm reasonably certain this patch fixed a random orange we were seeing on Windows (bug 739986).

Am I OK to push to comm-aurora?

-Mike
(In reply to Mike Conley (:mconley) from comment #29)

> Am I OK to push to comm-aurora?
yes, definitely, thx!
Summary: Cancelling uploads to Dropbox does not work, and is unavailable for YouSendIt → Cancelling uploads to Dropbox or YouSendIt does not work, and is unavailable after first attempt
You need to log in before you can comment on or make changes to this bug.