Closed
Bug 1356306
Opened 8 years ago
Closed 8 years ago
Large images (>500KB) inserted into draft get truncated when re-opeing draft
Categories
(Thunderbird :: Message Compose Window, defect)
Tracking
(thunderbird_esr5253+ fixed, thunderbird53 fixed, thunderbird54 fixed, thunderbird55 fixed)
RESOLVED
FIXED
Thunderbird 55.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
3.81 KB,
patch
|
aceman
:
review+
jorgk-bmo
:
approval-comm-aurora+
jorgk-bmo
:
approval-comm-esr52+
|
Details | Diff | Splinter Review |
1.86 KB,
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-aurora+
jorgk-bmo
:
approval-comm-esr52+
|
Details | Diff | Splinter Review |
+++ 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].
Assignee | ||
Comment 1•8 years ago
|
||
Also see attachment 8858058 [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.
Assignee | ||
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
tracking-thunderbird_esr52:
--- → +
Keywords: regression
Assignee | ||
Updated•8 years ago
|
Component: Untriaged → Message Compose Window
Assignee | ||
Updated•8 years ago
|
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)
Assignee | ||
Comment 6•8 years ago
|
||
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
Assignee | ||
Comment 7•8 years ago
|
||
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?
Assignee | ||
Comment 8•8 years ago
|
||
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 | ||
Updated•8 years ago
|
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
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
![]() |
||
Comment 10•8 years ago
|
||
(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.
![]() |
||
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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+
Assignee | ||
Comment 13•8 years ago
|
||
(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?
![]() |
||
Comment 14•8 years ago
|
||
(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.
Assignee | ||
Comment 15•8 years ago
|
||
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+
Assignee | ||
Comment 16•8 years ago
|
||
(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 ;-)
Assignee | ||
Comment 17•8 years ago
|
||
Oh, and how about adding try/catch to the other call site as well?
![]() |
||
Comment 18•8 years ago
|
||
This should be it.
Attachment #8858448 -
Attachment is obsolete: true
Attachment #8858490 -
Attachment is obsolete: true
Attachment #8858496 -
Flags: review+
Assignee | ||
Comment 19•8 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-thunderbird53:
--- → wontfix
status-thunderbird54:
--- → affected
status-thunderbird55:
--- → fixed
status-thunderbird_esr52:
--- → affected
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Assignee | ||
Updated•8 years ago
|
Attachment #8858496 -
Flags: approval-comm-esr52?
Attachment #8858496 -
Flags: approval-comm-aurora+
Assignee | ||
Updated•8 years ago
|
![]() |
||
Comment 20•8 years ago
|
||
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)
Assignee | ||
Comment 21•8 years ago
|
||
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+
Assignee | ||
Comment 22•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8858658 -
Flags: approval-comm-esr52?
Attachment #8858658 -
Flags: approval-comm-aurora+
Assignee | ||
Comment 23•8 years ago
|
||
Assignee | ||
Comment 24•8 years ago
|
||
Assignee | ||
Comment 25•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8858496 -
Flags: approval-comm-esr52? → approval-comm-esr52+
Assignee | ||
Updated•8 years ago
|
Attachment #8858658 -
Flags: approval-comm-esr52? → approval-comm-esr52+
Assignee | ||
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•