Last Comment Bug 740956 - Cancelling uploads to Dropbox or YouSendIt does not work, and is unavailable after first attempt
: Cancelling uploads to Dropbox or YouSendIt does not work, and is unavailable ...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Message Compose Window (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: Thunderbird 14.0
Assigned To: David :Bienvenu
:
Mentors:
Depends on:
Blocks: BigFiles 738299 739986
  Show dependency treegraph
 
Reported: 2012-03-30 13:49 PDT by Mike Conley (:mconley) - (Needinfo me!)
Modified: 2012-04-05 07:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
No cancel menuitem when uploading to YouSendIt (8.85 KB, image/png)
2012-03-30 13:49 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details
get cancel working (5.73 KB, patch)
2012-03-30 17:05 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review
get cancel, and don't alert the user (7.36 KB, patch)
2012-04-02 10:21 PDT, David :Bienvenu
mconley: review+
Details | Diff | Splinter Review
Make cancelling and uploading repeatable (1.99 KB, patch)
2012-04-03 07:42 PDT, Mike Conley (:mconley) - (Needinfo me!)
mozilla: review+
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review
Tests for all of the above. (10.94 KB, patch)
2012-04-03 07:42 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
fix for cancelling uploads that haven't started (7.61 KB, patch)
2012-04-03 08:39 PDT, David :Bienvenu
mconley: review+
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review
Tests for the above. (31.46 KB, patch)
2012-04-03 13:27 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Tests v3 (28.29 KB, patch)
2012-04-03 13:35 PDT, Mike Conley (:mconley) - (Needinfo me!)
no flags Details | Diff | Splinter Review
Tests v4 (31.61 KB, patch)
2012-04-04 08:20 PDT, Mike Conley (:mconley) - (Needinfo me!)
mozilla: review+
mozilla: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (Needinfo me!) 2012-03-30 13:49:14 PDT
Created attachment 611015 [details]
No cancel menuitem when uploading to YouSendIt

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...
Comment 1 David :Bienvenu 2012-03-30 14:25:11 PDT
presumably item.uploading isn't set for yousendit uploads?
Comment 2 David :Bienvenu 2012-03-30 14:33:29 PDT
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.
Comment 3 David :Bienvenu 2012-03-30 16:15:13 PDT
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.
Comment 4 David :Bienvenu 2012-03-30 17:05:33 PDT
Created attachment 611092 [details] [diff] [review]
get cancel working

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.
Comment 5 Mike Conley (:mconley) - (Needinfo me!) 2012-04-02 08:08:40 PDT
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
Comment 6 David :Bienvenu 2012-04-02 08:13:49 PDT
(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.
Comment 7 David :Bienvenu 2012-04-02 10:21:00 PDT
Created attachment 611507 [details] [diff] [review]
get cancel, and don't alert the user

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.
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2012-04-03 07:42:05 PDT
Created attachment 611801 [details] [diff] [review]
Make cancelling and uploading repeatable

Fixes the UI to allow us to re-upload a file that has been cancelled.
Comment 9 Mike Conley (:mconley) - (Needinfo me!) 2012-04-03 07:42:43 PDT
Created attachment 611802 [details] [diff] [review]
Tests for all of the above.

And tests!  Huzzah!
Comment 10 Mike Conley (:mconley) - (Needinfo me!) 2012-04-03 07:49:55 PDT
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 11 David :Bienvenu 2012-04-03 07:50:03 PDT
Comment on attachment 611801 [details] [diff] [review]
Make cancelling and uploading repeatable

makes sense.
Comment 12 David :Bienvenu 2012-04-03 08:08:24 PDT
(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.
Comment 13 Mike Conley (:mconley) - (Needinfo me!) 2012-04-03 08:13:25 PDT
(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 14 Mike Conley (:mconley) - (Needinfo me!) 2012-04-03 08:13:43 PDT
Comment on attachment 611507 [details] [diff] [review]
get cancel, and don't alert the user

I'm happy with this.
Comment 15 David :Bienvenu 2012-04-03 08:39:35 PDT
Created attachment 611830 [details] [diff] [review]
fix for cancelling uploads that haven't started

same as 611507, w/ addition of notifying front end of cancels for uploads that haven't started.
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2012-04-03 13:27:50 PDT
Created attachment 611953 [details] [diff] [review]
Tests for the above.

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.
Comment 17 Mike Conley (:mconley) - (Needinfo me!) 2012-04-03 13:35:27 PDT
Created attachment 611958 [details] [diff] [review]
Tests v3

Disentangled from my patch for bug 738299.
Comment 18 Mike Conley (:mconley) - (Needinfo me!) 2012-04-04 06:39:55 PDT
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?
Comment 19 David :Bienvenu 2012-04-04 07:05:45 PDT
(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 20 Mike Conley (:mconley) - (Needinfo me!) 2012-04-04 07:15:15 PDT
Attachment 611801 [details] [diff] committed to comm-central as http://hg.mozilla.org/comm-central/rev/76d613340388
Comment 21 Mike Conley (:mconley) - (Needinfo me!) 2012-04-04 07:16:07 PDT
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.
Comment 22 Mike Conley (:mconley) - (Needinfo me!) 2012-04-04 07:16:28 PDT
Comment on attachment 611830 [details] [diff] [review]
fix for cancelling uploads that haven't started

Same as above.
Comment 23 Mike Conley (:mconley) - (Needinfo me!) 2012-04-04 07:22:33 PDT
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
Comment 24 Mike Conley (:mconley) - (Needinfo me!) 2012-04-04 08:20:52 PDT
Created attachment 612197 [details] [diff] [review]
Tests v4

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
Comment 25 David :Bienvenu 2012-04-04 09:57:52 PDT
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.
Comment 26 Mike Conley (:mconley) - (Needinfo me!) 2012-04-04 10:02:33 PDT
Comment on attachment 612197 [details] [diff] [review]
Tests v4

Pretty safe for Aurora.  The only risk is introducing intermittent oranges.
Comment 27 David :Bienvenu 2012-04-04 10:16:41 PDT
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.
Comment 28 Mike Conley (:mconley) - (Needinfo me!) 2012-04-04 10:17:36 PDT
(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.
Comment 29 Mike Conley (:mconley) - (Needinfo me!) 2012-04-05 06:38:53 PDT
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
Comment 30 David :Bienvenu 2012-04-05 06:43:59 PDT
(In reply to Mike Conley (:mconley) from comment #29)

> Am I OK to push to comm-aurora?
yes, definitely, thx!
Comment 31 Mike Conley (:mconley) - (Needinfo me!) 2012-04-05 06:52:49 PDT
Ok, Attachment 612197 [details] [diff] pushed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/a4075036955b.

Note You need to log in before you can comment on or make changes to this bug.