Closed Bug 1356306 Opened 4 years ago Closed 4 years ago

Large images (>500KB) inserted into draft get truncated when re-opeing draft

Categories

(Thunderbird :: Message Compose Window, defect)

52 Branch
defect
Not set
normal

Tracking

(thunderbird_esr5253+ fixed, thunderbird53 fixed, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 53+ fixed
thunderbird53 --- fixed
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1355913 +++

This was first reported in bug 1355913.

For STR see attachment 8858053 [details], attachment 8858054 [details] and attachment 8858055 [details].
Steps to reproduce:

Images inserted in emails. (not attachments).
Insert > Image
click on 'Choose file' - chose images stored on my computer.
This shows as file:///C:/Users etc etc
'attach this image to the message' is selected.
Image displayed in preview
All seems perfect.
click on OK
Image inserted.

Save as a Draft or as Template.


Actual results:

Click on 'Drafts'
select email.
Email shows correctly in Message Pane.
Select to Edit.
Opens in new Write window.
only top half of the image is shown.
double click on image to get 'Image properties'
Note the black outline of image box shows correct size.

Image shows only half of image - same as shown in email.
Save as draft and now only half of image is shown.
No error message etc.
Send an email; and only half of image is sent/displayed.


Using windows Vista OS
Thunderbird Version 52.0.

This happens everytime; fully reproducable.
Does not matter if image is full size or whether the user sets a 'Custom size' in Image Properties.

Note:
Open draft showing cropped image.
double click on image to see Image Properties.
reselect the 'Image Location' and click on Ok will re-insert as normal, but if you then save as draft, same issue occurs.
I can confirm this now:
  Save as a Draft after inserting image.
  Click on 'Drafts'
  select email.
  Email shows correctly in Message Pane.
  Select to Edit.
  Opens in new Write window.
  only top half of the image is shown, for me even a small stripe only, like 5%.

I can only get it to truncate a large image, small ones work fine. BTW, it's not a good idea to insert a large image and change its dimensions, it's better to resize it first. Or use the add-on "Shrunked Image Resizer".

Where do you store your drafts? In a local folder or an IMAP folder? And if IMAP, is it synchronised for offline use?

Anyway, I can reproduce this for a local folder.

Magnus?
Flags: needinfo?(mkmelin+mozilla)
(In reply to Anje from comment #2)
> This happens everytime; fully reproducable.
> Does not matter if image is full size or whether the user sets a 'Custom
> size' in Image Properties.
Do you see the problem with an image of medium or small size, like my "JK" icon here or something of the size of - say - 800px by 600px?
Flags: needinfo?(anjeyelf)
Component: Untriaged → Message Compose Window
Blocks: 1322155
No longer depends on: 1322155
All saved ok to drafts, but upon opening draft....

tested  variety of images and sizes.
'Drafts' folder in a pop mail account.

297 x 356    29.1kb   worked ok
576 x 432    60.0kb   worked ok
1024 x 768  148.0kb   worked ok
2304 x 1728 612.0kb   truncated
2048 x 1536   1.1MB   truncated

Tested on imap gmail drafts folder.
297 x 356    29.1kb   worked ok
2304 x 1728 612.0kb   truncated
2128 x 2832  0.98MB   truncated.
Flags: needinfo?(anjeyelf)
Thanks, as I suspected.
Summary: image inserted into draft get truncated when re-opeing draft → Large images (>500KB and >2000px) inserted into draft get truncated when re-opeing draft
I did a bit of debugging of this code:

  let stream = Components.classes["@mozilla.org/binaryinputstream;1"]
    .createInstance(Components.interfaces.nsIBinaryInputStream);
  stream.setInputStream(inputStream);
  let encoded = btoa(stream.readBytes(stream.available()));
  stream.close();

On a 7 MB image that results in a 10 MB message (since base64 encode) I get
557056 for stream.available() and
742744 for encoded.length.

So clearly, the stream is not reading back the full data. Maybe the data should be read async?
Attached patch 1356306-large-image.patch (v1). (obsolete) — Splinter Review
This fixes it. Not sure whether += is the best operator to concatenate binary data in JS "strings".

Aceman, I have enough bugs right now, perhaps you can take over if you have a better idea.
Flags: needinfo?(mkmelin+mozilla)
Attachment #8858448 - Flags: feedback?(acelists)
You seem to have found it out the hard way. See https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIInputStream:

available()
Determine number of bytes available in the stream.
Note: This method should not be used to determine the total size of a stream, even if the stream corresponds to a local file. Moreover, since a stream may make available more than 2^32 bytes of data, this method is incapable of expressing the entire size of the underlying data source.

There is also some talk about blocking streams, etc.
I need to look at this further tomorrow.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Summary: Large images (>500KB and >2000px) inserted into draft get truncated when re-opeing draft → Large images (>500KB) inserted into draft get truncated when re-opeing draft
(In reply to Jorg K (GMT+2) from comment #7)
> On a 7 MB image that results in a 10 MB message (since base64 encode) I get
> 557056 for stream.available() and

I got exactly the same number of bytes so it may be some block size in the nsIInputStream.
The next size is 589824 and then it is alternating between the two.

(In reply to Jorg K (GMT+2) from comment #8)
> This fixes it. Not sure whether += is the best operator to concatenate
> binary data in JS "strings".

It is done here too:
https://dxr.mozilla.org/comm-central/rev/95da4e9ee8dc6cbf96b056b3e0e70ae90d0fff46/mail/components/cloudfile/nsHightail.js#1030
 
> Aceman, I have enough bugs right now, perhaps you can take over if you have
> a better idea.

Yes, I'm playing with it and will attach the final version soon.

It's a pity even if we load the file properly, there is still the Security error in the console about not loading the image. Only after that the code fixes it up and the image is displayed.
Attached patch 1356306-large-image.patch (v2) (obsolete) — Splinter Review
I run with this on a 39MB jpeg image and all seems fine now. It took 60 seconds just to reopen the draft:)

I thought about reading the stream in chunks (e.g. 512KB) to not allocate the memory for the full file in readBytes() but as stream.available() chunks it already, we probably don't need to. Also I tried to read in 100KB chunks and it made no speed difference (but in the test it was most probably reading from OS filesystem cache).

So I just added a try {} catch {} block as readBytes() throws in case not all available bytes could be read (I tested that it really does throw, if too many bytes are requested). Reads could maybe be short on disk/network errors.
Attachment #8858490 - Flags: review+
Attachment #8858490 - Flags: review?(jorgk)
Comment on attachment 8858448 [details] [diff] [review]
1356306-large-image.patch (v1).

The pattern seems correct, can also be seen at https://dxr.mozilla.org/comm-central/rev/abf145ebd05fe105efbc78b761858c34f7690154/mozilla/addon-sdk/source/lib/sdk/io/byte-streams.js#36 .
Attachment #8858448 - Flags: feedback?(acelists) → feedback+
(In reply to :aceman from comment #10)
> I got exactly the same number of bytes so it may be some block size in the
> nsIInputStream.
> The next size is 589824 and then it is alternating between the two.
Same here, alternating.

> > This fixes it. Not sure whether += is the best operator to concatenate
> > binary data in JS "strings".
> It is done here too:
> https://dxr.mozilla.org/comm-central/rev/
> 95da4e9ee8dc6cbf96b056b3e0e70ae90d0fff46/mail/components/cloudfile/
> nsHightail.js#1030
Not quite. That code reads and writes, it doesn't assemble the result. But your other example assembles.

> It's a pity even if we load the file properly, there is still the Security
> error in the console about not loading the image. Only after that the code
> fixes it up and the image is displayed.
That's the whole idea, we get notified via the security error that we need to convert the images.

> Yes, I'm playing with it and will attach the final version soon.
I was thinking:

Instead of accumulating the raw data, can we base64 the chunks?
But then base64(a + b) != base64(a) + base64(b), right?
(In reply to Jorg K (GMT+2) from comment #13)
> Instead of accumulating the raw data, can we base64 the chunks?
> But then base64(a + b) != base64(a) + base64(b), right?

I think so too.
Comment on attachment 8858490 [details] [diff] [review]
1356306-large-image.patch (v2)

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

Please change #User to
Jorg K <jorgk@jorgk.com> and Aceman
to acknowledge double-authorship. I've always done this in my collaborations with Kent.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5434,5 @@
>          event.target.classList.add("loading-internal");
> +        try {
> +          loadBlockedImage(src);
> +        } catch (e) {
> +          // Couldn't load the referenced images.

This should be logged to the console.
Attachment #8858490 - Flags: review?(jorgk) → review+
(In reply to :aceman from comment #14)
> (In reply to Jorg K (GMT+2) from comment #13)
> > Instead of accumulating the raw data, can we base64 the chunks?
> > But then base64(a + b) != base64(a) + base64(b), right?
> I think so too.
I tried it, it doesn't work, which is no surprise reading https://en.wikipedia.org/wiki/Base64 ;-)
Oh, and how about adding try/catch to the other call site as well?
This should be it.
Attachment #8858448 - Attachment is obsolete: true
Attachment #8858490 - Attachment is obsolete: true
Attachment #8858496 - Flags: review+
https://hg.mozilla.org/comm-central/rev/47f6c719478575b711b0a8c70e5c80d297f4a175
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Attachment #8858496 - Flags: approval-comm-esr52?
Attachment #8858496 - Flags: approval-comm-aurora+
Blocks: 1355913
No longer depends on: 1355913
Blocks: 1356780
Attached patch similar caseSplinter Review
This looks like a copy of the same code but in mailComposeEditorOverlay.xul and has the same problematic pattern. I don't know when it is used but it is also new from one of the patches of Magnus.
Attachment #8858658 - Flags: review?(jorgk)
Comment on attachment 8858658 [details] [diff] [review]
similar case

That's where the image or other attachment gets converted to a data: URL in the first place. Since the source of the data is a file here, it seems that the file is always delivered in one chunk. In the other case we fixed, that data came from a message via MIME in twisted ways.

Anyway, yes, this should be fixed. I haven't tested it, but as long as you can still attach a image or other file, we're cool.
Attachment #8858658 - Flags: review?(jorgk) → review+
Attachment #8858658 - Flags: approval-comm-esr52?
Attachment #8858658 - Flags: approval-comm-aurora+
Attachment #8858496 - Flags: approval-comm-esr52? → approval-comm-esr52+
Attachment #8858658 - Flags: approval-comm-esr52? → approval-comm-esr52+
You need to log in before you can comment on or make changes to this bug.