Box.com attachment upload stopped working in Thunderbird

RESOLVED FIXED in Thunderbird 45.0

Status

defect
--
major
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: klb3317, Assigned: clokep)

Tracking

(Blocks 1 bug, {regression})

31 Branch
Thunderbird 45.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr3844+ fixed)

Details

(Whiteboard: [regression:TB33][relnote-thunderbird])

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

5 years ago
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

5 years ago
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

5 years ago
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.
Blocks: 1021684
Component: Untriaged → FileLink

Updated

5 years ago
OS: Windows 7 → All
Hardware: x86_64 → All
Keywords: regression

Comment 3

5 years ago
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

Updated

5 years ago
Duplicate of this bug: 1085006

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

4 years ago
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).
Assignee

Comment 7

4 years ago
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

4 years ago
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.
(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
Severity: normal → major
See Also: → 1179392
Whiteboard: [regression:TB33]
Assignee

Comment 10

4 years ago
Posted patch box.diff (obsolete) — Splinter Review
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.
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attachment #8672667 - Flags: review?(philipp)

Comment 11

4 years ago
(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

4 years ago
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

4 years ago
(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.
Assignee

Comment 14

4 years ago
Posted patch Patch v2 (obsolete) — Splinter Review
(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.
Attachment #8672667 - Attachment is obsolete: true
Attachment #8672667 - Flags: review?(philipp)
Attachment #8673074 - Flags: review?(philipp)

Comment 15

4 years ago
(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.
Duplicate of this bug: 1211174

Comment 17

4 years ago
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

4 years ago
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?
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 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 {
Attachment #8673074 - Flags: review?(philipp) → review+

Comment 21

4 years ago
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?
Assignee

Comment 22

4 years ago
(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!

Updated

4 years ago
Blocks: 1214845

Comment 23

4 years ago
Future plan to be included in any release?
Flags: needinfo?(clokep)
Assignee

Comment 24

4 years ago
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).
Flags: needinfo?(clokep)
Assignee

Comment 25

4 years ago
(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.
Assignee

Comment 26

4 years ago
Posted patch box.diff (obsolete) — Splinter Review
This patch only removes the listeners, it does not do any of the reformatting. I tested this again and everything seems to work.
Attachment #8673074 - Attachment is obsolete: true
Attachment #8680576 - Flags: review?(philipp)
Assignee

Updated

4 years ago
Attachment #8680576 - Flags: review?(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.
Attachment #8680576 - Flags: review?(mconley) → review-
Assignee

Updated

4 years ago
Attachment #8680576 - Flags: review?(philipp)
Assignee

Comment 28

4 years ago
Posted patch box.diff v3Splinter Review
(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!
Attachment #8680576 - Attachment is obsolete: true
Attachment #8682690 - Flags: review?(philipp)
Attachment #8682690 - Flags: review?(mconley)
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.
Attachment #8682690 - Flags: review?(philipp)
Attachment #8682690 - Flags: review?(mconley)
Attachment #8682690 - Flags: review+
Assignee

Comment 30

4 years ago
(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.
Assignee

Comment 31

4 years ago
I pushed this with the function name change

https://hg.mozilla.org/comm-central/rev/c73ce0b62910
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
Assignee

Updated

4 years ago
Depends on: 1231654
Assignee

Updated

4 years ago
Duplicate of this bug: 1179392
Assignee

Updated

3 years ago
Duplicate of this bug: 1202244
Assignee

Updated

3 years ago
Duplicate of this bug: 1240406

Updated

3 years ago
Whiteboard: [regression:TB33] → [regression:TB33][relnote-thunderbird]

Updated

3 years ago
Duplicate of this bug: 1255141
Comment on attachment 8682690 [details] [diff] [review]
box.diff v3

Landed for Thunderbird 38.8

http://hg.mozilla.org/releases/comm-esr38/rev/3911bc209525
Attachment #8682690 - Flags: approval-comm-esr38+

Comment 37

3 years ago
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

3 years ago
(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

3 years ago
Sorry for asking. I found out myself.

Comment 40

3 years ago
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.
Assignee

Comment 41

2 years ago
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

2 years ago
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":"xxxx@laposte.net","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":"xxxxx@laposte.net","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 ?
Assignee

Comment 44

2 years ago
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.
You need to log in before you can comment on or make changes to this bug.