1045845 - Box.com attachment upload stopped working in Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: FileLink, defect)

Description

[klb3317]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140716183446

Steps to reproduce:

Attached file to letter, pressed "Link file button".


Actual results:

Attach could not uploaded to box.com
Window appeared: "Unable to upload <filename> to Box."
It worked in a previous version of Thunderbird.


Expected results:

Attach should be uploaded to box.com

Comment 1

[Cernak]

I have observed the same bug.

32.0.3 Mozilla Firefox for Ubuntu canonical 1.0

It is possible to attache file, after pushing link file button, however uplading does not start i.e. no transfer on in the network.

Expected results:

Attach should be uploaded to box.com

Comment 2

[rsx11m]

Apparently something in the updates to the API implementation was missed.
See screen shots in https://support.mozilla.org/en-US/questions/1031089#answer-654433

According to Box.com it /may/ be fixed in the 33 beta, but someone needs to try.
I was unable to identify any current bug report other than this one.

Comment 3

[Julio Garay]

I suspecto this is a duplicate of #1085006. Can you try starting TB from the CLI, and see if you get an error related to the creation of a folder? In my case (Linux Mint 17, x86_64), it was this:
[2014-12-14 14:45:41	BoxService	ERROR	Failed to create a new folder: 409]

Kind regards,
J. Garay

Comment 5

[Wayne Mery (:wsmwk)]

also https://support.box.com/hc/communities/public/questions/203983136-Thunderbird-authorization-for-BOX-as-filelink-storage-location-for-sharing-large-files-sent-in-email-

Comment 6

[Martin Pecka]

I confirm this bug in 31.5.0 . I had the Thunderbird folder in my Box account from the last year, and I was unable to use FileLink (I needed to re-authorize Box, since I changed computers in the meantime). When I deleted that folder, uploading started to work. 

I was even able to upload attachments several times (even after TB restart), so I don't know when TB tries to create the folder and fails to do so. Maybe the folder can't exist before you authorize Box.

Nevertheless, deleting the Thunderbird folder from your Box account and allowing TB to create seems currently as a workaround (I didn't even need to de-authorize Box and authorize again, which I expected).

Comment 7

[Patrick Cloke [:clokep]]

Thi sounds like bug 1085006, which I filed after updating the code to handle the new API. (Note that this bug was always there, it was created as part of that API change.) It has to do with if you switch profiles, etc. so there's no _cacheFolderId preference set. We should be looking to see if the folder exists before trying to create it (or alternately we can always try to create it and not fail on the error code that says "this is already created).

Starting with the code near _initFolder [1] should get someone going.

[1] https://dxr.mozilla.org/comm-central/source/mail/components/cloudfile/nsBox.js?from=nsbox.js&case=true#116-143

Comment 8

[Oliver Rovini]

I confirm this bug in 38.2.0. I used an existing and linked account which was working a few months before with no success. I can see the available and used storage space under the options->attachements. I deleted the box.com account and authorized it new. It hangs in the authorization process. After cancel of the process I can start the authorization again and this parts works then. After that process I'm at the beginning again. I can see the storage space but still can't upload files. The upload icon runs for minutes without success and there's no file found in the storage space when looking manually.

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to Patrick Cloke [:clokep] from comment #7)
>...
> Starting with the code near _initFolder [1] should get someone going.
> 
> [1]
> https://dxr.mozilla.org/comm-central/source/mail/components/cloudfile/nsBox.
> js?from=nsbox.js&case=true#116-143

Thanks for that info

Comment 10

[Patrick Cloke [:clokep]]

Attachment: box.diff

I hesitate between requesting feedback and review. This code hasn't been extensively tested (but it has been tested!)

With these changes I was able to:
- Hook my Box.com account up to a clean Thunderbird profile (i.e. it found the folder ID instead of failing to create it)
- Upload an image using the Box API

Some of the changes are unnecessary style changes.

This code desperately needs tests.

Comment 11

[Martin Pecka]

(In reply to Patrick Cloke [:clokep] from comment #10)
> Created attachment 8672667 [details] [diff] [review]
> box.diff
> 
> I hesitate between requesting feedback and review. This code hasn't been
> extensively tested (but it has been tested!)
> 
> With these changes I was able to:
> - Hook my Box.com account up to a clean Thunderbird profile (i.e. it found
> the folder ID instead of failing to create it)
> - Upload an image using the Box API
> 
> Some of the changes are unnecessary style changes.
> 
> This code desperately needs tests.

Awesome, it works! I tested the patch on my live TB profile (38.3.0 release). The patch applied seamlessly. I had Box connected before, so in my Box account, the folder Thunderbird has already existed. Displaying the Attachment options works and shows the free space. The pre-existing Thunderbird folder remained in its place along with its contents. Then, I was able to send a large attachment and the upload to Box succeeded. Kudos!

I don't feel like reviewing TB code, just a quick remark:

Components.utils.reportError("Initing folder");

Shouldn't it be both more informative contents and less critical error type?

Comment 12

[Julio Garay]

Forgive my ignorance, but... Could you give me (us) some instructions on how to apply the patch? I will be happy to test and provide feedback. I also use TB 38.3.0 under Linux.

Comment 13

[Martin Pecka]

(In reply to Julio Garay from comment #12)
> Forgive my ignorance, but... Could you give me (us) some instructions on how
> to apply the patch? I will be happy to test and provide feedback. I also use
> TB 38.3.0 under Linux.

It's not your ignorance, this is almost impossible stuff for all people except TB/addon developers.

I created a package you can download to test it.

In folder https://app.box.com/s/c4ggjibl1c8k9ys55hf10i84lcz8nuc9 select "linux64" for Linux TB installation, or "win64" for Windows TB installation. Download the omni.ja file, say, to ~/Download/omni.ja .

On Linux, close TB and issue the following commands:
cd /usr/lib/thunderbird
sudo mv omni.ja omni.ja.orig
sudo cp ~/Download/omni.ja .
sudo chown root:root omni.ja

This will backup the original omni.ja file and substitute it with the patched one.
On Windows, the omni.ja file is located under C:\Program Files (x86)\Mozilla Thunderbird, and the procedure is much the same.

Do never delete the omni.ja.orig backup, as you might find the patched archive buggy or corrupt. It is also not that much optimized as the officially distributed archive.

Comment 14

[Patrick Cloke [:clokep]]

Attachment: Patch v2

(In reply to Martin Pecka from comment #11)
> Awesome, it works! I tested the patch on my live TB profile (38.3.0
> release). The patch applied seamlessly. I had Box connected before, so in my
> Box account, the folder Thunderbird has already existed. Displaying the
> Attachment options works and shows the free space. The pre-existing
> Thunderbird folder remained in its place along with its contents. Then, I
> was able to send a large attachment and the upload to Box succeeded. Kudos!

Awesome! Thanks for testing this so quickly. This sounds like fairly similar conditions to the way I tested it. I need to do more testing to ensure the folder is created if it doesn't exist.

> I don't feel like reviewing TB code, just a quick remark:
> 
> Components.utils.reportError("Initing folder");
> 
> Shouldn't it be both more informative contents and less critical error type?

Thanks, left over from debugging. There's a log line right above it anyway. I've removed it in this patch.

Comment 15

[Julio Garay]

(In reply to Martin Pecka from comment #13)
> (In reply to Julio Garay from comment #12)
> > Forgive my ignorance, but... Could you give me (us) some instructions on how
> > to apply the patch? I will be happy to test and provide feedback. I also use
> > TB 38.3.0 under Linux.
> 
> It's not your ignorance, this is almost impossible stuff for all people
> except TB/addon developers.
> 
> I created a package you can download to test it.
> 
> In folder https://app.box.com/s/c4ggjibl1c8k9ys55hf10i84lcz8nuc9 select
> "linux64" for Linux TB installation, or "win64" for Windows TB installation.
> Download the omni.ja file, say, to ~/Download/omni.ja .
> 
> On Linux, close TB and issue the following commands:
> cd /usr/lib/thunderbird
> sudo mv omni.ja omni.ja.orig
> sudo cp ~/Download/omni.ja .
> sudo chown root:root omni.ja
> 
> This will backup the original omni.ja file and substitute it with the
> patched one.
> On Windows, the omni.ja file is located under C:\Program Files (x86)\Mozilla
> Thunderbird, and the procedure is much the same.
> 
> Do never delete the omni.ja.orig backup, as you might find the patched
> archive buggy or corrupt. It is also not that much optimized as the
> officially distributed archive.

Thank you! I have downloaded and installed as instructed. Will test it shortly.

Comment 17

[jameslee303]

I downloaded the omni.ja and replaced with the original. However, that changed the language for UI. Is there English version?
Or, is there an instruction how to replace the code without replacing the entire omni.ja?
Thanks,

Comment 18

[Martin Pecka]

Oh, I'm sorry for that. Yes, the omni.ja is an (almost normal) ZIP file. So you copy it somewhere, unzip, edit the files and zip it back.

You need to delete file jsloader/resource/gre/components/nsBox.js .
Then you patch the files referenced in the patch. They are, however, in different locations in this archive - components/nsBox.js, chrome/messenger/content/messenger/cloudfile/addAccountDialog.{js,xul}. So you need to either edit the paths in the patch file, or apply the patch "manually".

Another option could be the "Quick Locale Switcher" addon. Would that help?

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

While I'm happy to hear you'd love to test this, I'd appreciate if you could move any discussion related to applying patches to private email. A number of people are getting bugmail for each comment made here. Thank you for your understanding!

That said, I'll get to the review on this shorly, please ping me in case I forget.

Comment 20

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8673074 [details] [diff] [review]
Patch v2

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

r+ with these comments considered. I haven't tested the patch because there's already been comments that it works fine.

::: mail/components/cloudfile/content/addAccountDialog.js
@@ +43,5 @@
>      }
>    },
> +
> +  QueryInterface:
> +    XPCOMUtils.generateQI([Ci.nsIRequestObserver, Ci.nsISupports]),

By convention QueryInterface is usually at the top of an object, and nsISupports doesn't need to be added because it is default. That will also allow you to put it on one line.

@@ +84,5 @@
>  
> +    // Hook up the selection handler.
> +    this._accountType.addEventListener("select", function(e) {
> +      addAccountDialog.accountTypeSelected();
> +    });

The event listener needs to be removed eventually. The same goes for the other already existing handlers, maybe you can remove them too if they aren't already.

::: mail/components/cloudfile/nsBox.js
@@ +16,5 @@
>  Cu.import("resource:///modules/cloudFileAccounts.js");
>  Cu.import("resource:///modules/OAuth2.jsm");
>  Cu.import("resource://gre/modules/Http.jsm");
>  
> +Cu.importGlobalProperties(["File"]);

I don't see File actually being used, but maybe this was just missing before?

@@ +429,5 @@
> +                                        aSuccessCallback,
> +                                        aFailureCallback) {
> +    this.log.info("Getting folder: " + aName);
> +    if (Services.io.offline)
> +      throw Ci.nsIMsgCloudFileProvider.offlineErr;

mail/ style might be slightly different, so please ignore my style nits if needed.

use brackets even for one-line if's.

@@ +460,5 @@
> +            break;
> +          }
> +        }
> +      }
> +      catch(e) {

} catch (e) {

@@ +470,5 @@
> +      if (folderId) {
> +        this.log.info("folder id = " + folderId);
> +        aSuccessCallback(folderId)
> +      }
> +      else {

} else {

Comment 21

[Martin Pecka]

Since it hasn't been verified here yet, I'd like to add that creation of the Thunderbird folder in a brand new Box account works well.

Not sure if it should be solved here or not, but if I try to upload a file whose name collides with a file already existing under the Thunderbird Box folder, then the FileLink upload fails. Shouldn't it instead try to rename the uploaded file, or ask the user if he intends to overwrite the cloud file?

Comment 22

[Patrick Cloke [:clokep]]

(In reply to Martin Pecka from comment #21)
> Since it hasn't been verified here yet, I'd like to add that creation of the
> Thunderbird folder in a brand new Box account works well.

Thanks for checking! :)

> Not sure if it should be solved here or not, but if I try to upload a file
> whose name collides with a file already existing under the Thunderbird Box
> folder, then the FileLink upload fails. Shouldn't it instead try to rename
> the uploaded file, or ask the user if he intends to overwrite the cloud file?

I noticed this while looking at the code, but felt it was more important to get it back to a working state. Please file a new bug for this, you can CC me. Thanks!

Comment 23

[jameslee303]

Future plan to be included in any release?

Comment 24

[Patrick Cloke [:clokep]]

I need to fix the review comments. It's on my TODO list. Once it's done, we'll get this merged (and hopefully put into ESR too).

Comment 25

[Patrick Cloke [:clokep]]

(In reply to Philipp Kewisch [:Fallen] from comment #20)
> Comment on attachment 8673074 [details] [diff] [review]
> @@ +84,5 @@
> >  
> > +    // Hook up the selection handler.
> > +    this._accountType.addEventListener("select", function(e) {
> > +      addAccountDialog.accountTypeSelected();
> > +    });
> 
> The event listener needs to be removed eventually. The same goes for the
> other already existing handlers, maybe you can remove them too if they
> aren't already.

Good catch!

> ::: mail/components/cloudfile/nsBox.js
> @@ +16,5 @@
> >  Cu.import("resource:///modules/cloudFileAccounts.js");
> >  Cu.import("resource:///modules/OAuth2.jsm");
> >  Cu.import("resource://gre/modules/Http.jsm");
> >  
> > +Cu.importGlobalProperties(["File"]);
> 
> I don't see File actually being used, but maybe this was just missing before?

File used to automatically be included as a global, it needs to be manually imported like this now. It's already used in this file.

> @@ +429,5 @@
> > +                                        aSuccessCallback,
> > +                                        aFailureCallback) {
> > +    this.log.info("Getting folder: " + aName);
> > +    if (Services.io.offline)
> > +      throw Ci.nsIMsgCloudFileProvider.offlineErr;
> 
> mail/ style might be slightly different, so please ignore my style nits if
> needed.

This function was copy & pasted and slightly modified from _createFolder, this makes me inclined to leave it as the style will match the rest of the file.

Comment 26

[Patrick Cloke [:clokep]]

Attachment: box.diff

This patch only removes the listeners, it does not do any of the reformatting. I tested this again and everything seems to work.

Comment 27

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

Comment on attachment 8680576 [details] [diff] [review]
box.diff

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

The nsBox changes look fine, but the removal of those event handlers is not going to work like you expect, I don't think. See suggestion.

::: mail/components/cloudfile/content/addAccountDialog.js
@@ +103,5 @@
>  
> +  onUnInit: function AAD_onUnInit() {
> +    // Clean-up the event listeners.
> +    this._settings.removeEventListener("DOMContentLoaded",
> +                                       this.onIFrameLoaded.bind(this),

I don't think this is going to work. this.onIFrameLoaded.bind(this) will actually return different results every time, and Gecko is going to treat them as unequivalent.

What'd probably be better is to have a general "handleEvent" method on addAccountDialog, with a switch inside for each event type, which then calls into the right thing. Example:

this._settings.addEventListener("DOMContentLoaded", this);
this._settings.addEventListener("overflow", this);
...

handleEvent: function(aEvent) {
  switch (aEvent.type) {
    case "DOMContentLoaded": {
      this.onIFrameLoaded();
      break;
    }
    case "select": {
      this.accountTypeSelected();
      break;
    }
    // ...
  }
},

And then removing the event listeners like:

this._settings.removeEventListener("DOMContentLoaded", this);

Will do what you expect.

Comment 28

[Patrick Cloke [:clokep]]

Attachment: box.diff v3

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #27)
> The nsBox changes look fine, but the removal of those event handlers is not
> going to work like you expect, I don't think. See suggestion.

Oops, you're right. I forgot that bind creates a new function. Thanks for the suggestion, it's super clean!

Comment 29

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8682690 [details] [diff] [review]
box.diff v3

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

Codewise r+ with these comments, I haven't tested it because I am in a meeting :-P

::: mail/components/cloudfile/content/addAccountDialog.js
@@ +95,5 @@
>  
>      addAccountDialog.fitIFrame();
>    },
>  
> +  onUnInit: function AAD_onUnInit() {

You can remove the function name now that they are guessed in stacks anyway.

::: mail/components/cloudfile/nsBox.js
@@ +462,5 @@
> +        }
> +      }
> +      catch(e) {
> +        // most likely bad JSON
> +        this.log.error("Failed to get the folder:\n" + e);

Is there a chance that the other things will fail? It looks to me like we should rather just try/catch around the JSON.parse call and then use if()'s to check for result.item_collection.entries and that it is an array.

Comment 30

[Patrick Cloke [:clokep]]

(In reply to Philipp Kewisch [:Fallen] from comment #29)
> Comment on attachment 8682690 [details] [diff] [review]
> box.diff v3
> 
> Review of attachment 8682690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Codewise r+ with these comments, I haven't tested it because I am in a
> meeting :-P
> 
> ::: mail/components/cloudfile/content/addAccountDialog.js
> @@ +95,5 @@
> >  
> >      addAccountDialog.fitIFrame();
> >    },
> >  
> > +  onUnInit: function AAD_onUnInit() {
> 
> You can remove the function name now that they are guessed in stacks anyway.

Will do.


> ::: mail/components/cloudfile/nsBox.js
> @@ +462,5 @@
> > +        }
> > +      }
> > +      catch(e) {
> > +        // most likely bad JSON
> > +        this.log.error("Failed to get the folder:\n" + e);
> 
> Is there a chance that the other things will fail? It looks to me like we
> should rather just try/catch around the JSON.parse call and then use if()'s
> to check for result.item_collection.entries and that it is an array.

There shouldn't really be, this file seems to use this style of putting large try-catch around anything that touches network data. It's a bit frustrating as it eats errors while developing. I can change it if you feel strongly.

Comment 31

[Patrick Cloke [:clokep]]

I pushed this with the function name change

https://hg.mozilla.org/comm-central/rev/c73ce0b62910

Comment 36

[Kent James (:rkent)]

Comment on attachment 8682690 [details] [diff] [review]
box.diff v3

Landed for Thunderbird 38.8

http://hg.mozilla.org/releases/comm-esr38/rev/3911bc209525

Comment 37

[Børge Olsen]

To Martin Pecka or any other expert

I use Linux Elementary Loki and I just followed the instructions from answer #13, i.e.

"In folder https://app.box.com/s/c4ggjibl1c8k9ys55hf10i84lcz8nuc9 select "linux64" for Linux TB installation, or "win64" for Windows TB installation. Download the omni.ja file, say, to ~/Download/omni.ja .

On Linux, close TB and issue the following commands:
cd /usr/lib/thunderbird
sudo mv omni.ja omni.ja.orig
sudo cp ~/Download/omni.ja .
sudo chown root:root omni.ja

This will backup the original omni.ja file and substitute it with the patched one."

Result: Thunderbird refuses to start. 
How do I go back?

Comment 38

[Martin Pecka]

(In reply to Børge Olsen from comment #37)
> Result: Thunderbird refuses to start. 
> How do I go back?

sudo cp omni.ja.orig omni.ja

But why do you need that? The current stable TB has this issue fixed, and the omni.ja that is offered to be downloaded should only work with an old version of TB (that was stable at the time #13 was written).

Comment 39

[Børge Olsen]

Sorry for asking. I found out myself.

Comment 40

[Børge Olsen]

I renamed back to the original file, so that was it.

I then still cannot upload files to the filelink (I use Box.com) which was the reason why I tried this fix. My TB version is 45.5.1.
I then suspected my firewall and opened ports 80 and 443, but to no avail.

Comment 41

[Patrick Cloke [:clokep]]

Børge Olsen: I'd suggest filing a new bug (you can feel free to CC me on it), I doubt it's the same issue as this was fixed for Thunderbird 45. Thanks!

Comment 42

[hamelg]

Since thunderbird 52, uploading attachment via box fails. When right clicking on a attachment : Convert to >> box, thunderbird displays theses errors :

2017-05-01 14:06:36     BoxService      ERROR   Failed to parse account info response: TypeError: File.createFromNsIFile(...).then is not a function
2017-05-01 14:06:36     BoxService      ERROR   Account info response: {"type":"user","id":"231728495","name":"Gilles H","login":"[e-mail]","created_at":"2015-02-01T09:27:42-08:00","modified_at":"2017-05-01T00:30:14-07:00","language":"fr","timezone":"America\/Los_Angeles","space_amount":10737418240,"space_used":247383533,"max_upload_size":262144000,"status":"active","job_title":"","phone":"0634812416","address":"","avatar_url":"https:\/\/app.box.com\/api\/avatar\/large\/231748475"}
JavaScript strict warning: resource://gre/components/nsBox.js, line 216: ReferenceError: reference to undefined property "_urlListener"
2017-05-01 14:06:36     BoxService      ERROR   user info failed, status = 200
2017-05-01 14:06:36     BoxService      ERROR   response text = {"type":"user","id":"231748475","name":"Gilles H","login":"[e-mail]","created_at":"2015-02-01T09:27:42-08:00","modified_at":"2017-05-01T00:30:14-07:00","language":"fr","timezone":"America\/Los_Angeles","space_amount":10737418240,"space_used":247383533,"max_upload_size":262144000,"status":"active","job_title":"","phone":"0622418216","address":"","avatar_url":"https:\/\/app.box.com\/api\/avatar\/large\/231748475"}
2017-05-01 14:06:36     BoxService      ERROR   exception = TypeError: this._urlListener is undefined
JavaScript error: resource://gre/components/nsBox.js, line 216: TypeError: this._urlListener is undefined

Is it broken again ?

Comment 43

[hamelg]

https://bugzilla.mozilla.org/show_bug.cgi?id=1332940

Comment 44

[Patrick Cloke [:clokep]]

hamelg, Thanks for reporting that you're having issues, but please do not post comments on closed bugs -- the proper place for this is bug 1332940 or filing a new bug.