744004 - filelink doesn't handle being offline correctly.

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[David :Bienvenu]

The providers return an error code that says we're offline, when the compose window code tries to do an upload, but the compose window code expects to get the error in the onStopRequest call. I think the compose window code should expect the error right away.

Comment 1

[David :Bienvenu]

yikes, that compose attach code doesn't want to work that way. Maybe I will go the onStopRequest route, or just check if we're offline in the compose code and not allow filelink while offline.

Comment 2

[David :Bienvenu]

Attachment: proposed fix

I've decided to hide the filelink ui while offline because we don't have a string for telling the user that they can't do a filelink because we're offline. And I suspect this is probably a better experience anyway (but asking for UI review anyway).

I've also fixed the handling of the offline state in the providers and the nsMsgComposeCommands.js code for correctness sake, but if the ui is enabled while offline, we get the generic "an error occurred", which is not a nice experience.

Comment 3

[Jb Piacentino]

(In reply to David :Bienvenu from comment #2)
> Created attachment 613794 [details] [diff] [review]
> proposed fix
> 
> I've decided to hide the filelink ui while offline because we don't have a
> string for telling the user that they can't do a filelink because we're
> offline. And I suspect this is probably a better experience anyway (but
> asking for UI review anyway).
But are you considering to change this behaviour for upcoming releases, ie when the string had a chance to land ?

Comment 4

[David :Bienvenu]

(In reply to Jb Piacentino from comment #3)
> (In reply to David :Bienvenu from comment #2)
> > Created attachment 613794 [details] [diff] [review]
> > proposed fix
> > 
> > I've decided to hide the filelink ui while offline because we don't have a
> > string for telling the user that they can't do a filelink because we're
> > offline. And I suspect this is probably a better experience anyway (but
> > asking for UI review anyway).
> But are you considering to change this behaviour for upcoming releases, ie
> when the string had a chance to land ?

See my previous comment. I gave my opinion (no) but asked for Blake's opinion.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 613794 [details] [diff] [review]
proposed fix

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

This looks mostly good - though, I think there's another point where we use uploadFile that'll need to be wrapped in a try/catch - that's in convertListItemsToCloudAttachment.  That should probably be included.  Modulo that change, r=me.

As you and Jb mentioned, it'd probably be good to handle the offlineErr case with a new string.  I think we should prepare a separate patch for comm-central with that new string.

Also, this behaviour should be tested.  I can take care of that.

-Mike

Comment 6

[David :Bienvenu]

(In reply to David :Bienvenu from comment #4)

> See my previous comment. I gave my opinion (no) but asked for Blake's
> opinion.

Also, I did everything *but* add a string, so if we decide we want to leave the UI enabled, but show an error if the user uses the UI, then we just need to add a string and a case to the error handling switch statement.

But I'm completely unconvinced that we should enable UI that we know isn't going to work. The general Thunderbird practice is to disable or hide UI that won't work when we're offline, though we're not consistent about it.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Oh, David - one last thing - it looks like we also return offlineErr in refreshUserInfo and deleteFile.  Will those need to be updated?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 613794 [details] [diff] [review]
proposed fix

Actually, remind me again why we're not calling aCallback.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.offlineErr) from within the uploadFile functions?

Comment 9

[David :Bienvenu]

(In reply to Mike Conley (:mconley) from comment #8)
> Comment on attachment 613794 [details] [diff] [review]
> proposed fix
> 
> Actually, remind me again why we're not calling
> aCallback.onStopRequest(null, null, Ci.nsIMsgCloudFileProvider.offlineErr)
> from within the uploadFile functions?

because the calling code needs to handle the call failing synchronously in general (not just the specific case of being offline); it can't count on the call failing and being so kind as to call onStopRequest for a synchronous failure (e.g., some unexpected js exception). In this case, the request hasn't even started, so requiring onStopRequest calls seems like a byzantine way of doing synchronous error notification.

Yeah, thx, I noticed the deleteFile and refreshUserInfo cases and changed them to throws.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 613794 [details] [diff] [review]
proposed fix

When I'm offline, attaching a large file still shows the notification bar.

Hitting "Link" gives:
JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 1234: '<error>' when calling method: [nsIMsgCloudFileProvider::uploadFile]
JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 1227: NS_ERROR_MALFORMED_URI: Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIFileProtocolHandler.getFileFromURLSpec]

And going offline while trying to attach a file seems to result in the spinner forever spinning…

/me mumbles something about tests to catch this kind of "switch to offline in the middle of the operation" behaviour.  ;)

After failing to upload a file, when I go back online, the "Convert to" menu item remains greyed out.

I think I'm going to have to say ui-r- because of those problems, but I'm sure you'll fix them for the next patch.

Thanks,
Blake.

Comment 11

[David :Bienvenu]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #10)

> When I'm offline, attaching a large file still shows the notification bar.
I've got a fix for that.

> And going offline while trying to attach a file seems to result in the
> spinner forever spinning…

I could be wrong, but that sounds like a necko issue - I suspect it's beyond this bug to fix. If we're not getting told that the request failed, it's hard for us to do anything. But I'll try it and see.

> 
> /me mumbles something about tests to catch this kind of "switch to offline
> in the middle of the operation" behaviour.  ;)
> 
> After failing to upload a file, when I go back online, the "Convert to" menu
> item remains greyed out.

failing to upload because you went offline in the middle? That just sounds an other symptom of not being told the request failed, not anything to do with this patch.

Comment 12

[David :Bienvenu]

Attachment: disable attachment notification when offline

I could not duplicate the going offline while uploading error you saw - I got an error message about not being able to upload the file, and the spinner stopped. This was on windows; perhaps the mac networking code isn't as good with error propagation :-)

Comment 13

[David :Bienvenu]

Attachment: fix enabling of attachment context menu after errors

This may not help you, Blake, if you're not getting the spinners to stop, but when I try this, cancel remains enabled, among other things. Turns out the compose window isn't handling errors on upload other than cancel. This seems wrong to me - the only difference between cancel and the other errors is that we shouldn't put up an error message. That's what this patch does; mconley, do you think you could review the interdiff between this patch and the one that Blake ui minused (sob!)?

Comment 14

[Blake Winton (:bwinton) (:☕️)]

(In reply to David :Bienvenu from comment #12)
> Created attachment 614215 [details] [diff] [review]
> disable attachment notification when offline
> 
> I could not duplicate the going offline while uploading error you saw - I
> got an error message about not being able to upload the file, and the
> spinner stopped. This was on windows; perhaps the mac networking code isn't
> as good with error propagation :-)

I wonder if the difference is that I'm uploading two files at the time I'm going offline?  (I just noticed that.)

Comment 15

[David :Bienvenu]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #14)

> 
> I wonder if the difference is that I'm uploading two files at the time I'm
> going offline?  (I just noticed that.)

When I do that, the first one does give an error, but the second file spins (though I can cancel it). I'll see if I can fix it.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 614219 [details] [diff] [review]
fix enabling of attachment context menu after errors

Okay, ui-r=me with the eternal-spinning fixed.  :)

Thanks,
Blake.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 614219 [details] [diff] [review]
fix enabling of attachment context menu after errors

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

So this test causes two tests to fail in test-cloudfile-attachment-item.js (woo - the tests paid off! :D )

STR:

1)  Upload a file
2)  Try to cancel the upload

What happens?

An empty confirm dialog comes up.

What's expected?

No dialog - the upload should just stop.

::: mail/components/cloudfile/nsIMsgCloudFileProvider.idl
@@ +66,5 @@
>     *
>     * @param aFile File we previously uploaded in this session.
>     * @param aCallback callback when finished
>     *
>     * throws NS_ERROR_FAILURE if we don't know about the file.

Maybe fix this to be a @throws while you're here?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1085,5 @@
>                                          [displayName,
>                                           this.attachment.name]);
>          break;
> +      case this.cloudProvider.uploadCanceled:
> +        break;

Here's the problem - when we break out of this switch, we still spawn a confirm no matter what.  In the case of uploadCanceled, it's an empty confirm.

@@ +1233,5 @@
>      }
>  
> +    try {
> +      let listener = new uploadListener(item.attachment, file,
> +                                                    aProvider);

Can we line up aProvider so that the "a" falls beneath the "i" in item.attachment?

Comment 18

[David :Bienvenu]

sorry, I should have mentioned that I fixed the empty confirm last night...

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Hm, remember those tests I were talking about writing?

I don't think I'll be able to do them.  Messing with Services.io.offline is bad because Mozmill / JSBridge depends on a network connection being open to communicate.

Mocking out nsIIOService2.offline also seems to be an all-around bad idea.

So I think we might just have to take this one on manual testing.  :/

Comment 20

[David :Bienvenu]

(In reply to Mike Conley (:mconley) from comment #19)
> Hm, remember those tests I were talking about writing?
> 
> I don't think I'll be able to do them.  Messing with Services.io.offline is
> bad because Mozmill / JSBridge depends on a network connection being open to
> communicate.

I thought that was fixed somehow in mozmill.

Comment 21

[David :Bienvenu]

Attachment: fix mozmill errors, going offline with pending uploads

for some reason, MockCloudfileAccount didn't have the nsIMsgCloudProvider constants defined, so when I made cancel go through the switch statement in MsgComposeCommands.js uploadListener onStopRequest, it complained about this.cloudProvider.authErr not being defined. Ah, and the reason it worked for cancel is that the MsgComposeCommands.js code used Ci.nsIMsgCloudFileProvider.userCanceled instead of this.provider.userCanceled. The tests giveth, and the tests taketh away :-)

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 614952 [details] [diff] [review]
fix mozmill errors, going offline with pending uploads

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

Just one nit.  Other than that, the tests all pass, and it seems to operate as advertised.  I'm happy with this.

::: mail/test/mozmill/shared-modules/test-cloudfile-helpers.js
@@ +23,5 @@
>    accountKey: null,
>    settingsURL: "",
>    managementURL: "",
> +  // These are taken from nsIMsgCloudFileProvider
> +  authErr: 0x8055001e,

To avoid repeating ourselves, we can retrieve these from the interface, like:

authErr:  Ci.nsIMsgCloudFileProvider.authErr,
offlineErr: Ci.nsIMsgCloudFileProvider.uploadErr,

etc

Comment 23

[David :Bienvenu]

fixed on trunk - http://hg.mozilla.org/comm-central/rev/f7f5eeb7b12a

Comment 24

[David :Bienvenu]

Comment on attachment 614952 [details] [diff] [review]
fix mozmill errors, going offline with pending uploads

[Triage Comment] - we want this for aurora

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Committed to comm-aurora as http://hg.mozilla.org/releases/comm-aurora/rev/fc5dec27443c