754655 - Out of multiple identical inline images, all but 1st image break (broken image placeholder) when saving a message as draft or template multiple times (multipart numbering problem)

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Alexander Bergmann]

If you create a new message, insert one image multiple times and save the message multiple times as draft or template, the images get replaced by placeholder images.

1. Create a new message
2. Insert one image multiple times, e.g.:
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />
3. Save the message as draft or template
4. Open the saved message
5. Save the message as draft or template

Now the second image and the third image are replaced by placeholder images in the current editor. (However, opening the saved draft or template again shows the correct images.)

I've tested this with different versions of Thunderbird: This behaviour started in Thunderbird 10. (Using Thunderbird 3.1 up to Thunderbird 9 works without problems.)

Arch Linux / Thunderbird 12.0.1

Comment 1

[Hashem Masoud]

(In reply to myaddons from comment #0)
I followed your steps and I was able to save draft along with its images. No place holder was there. Draft was saved on an IMAP server drafts folder.

User Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20120428 Thunderbird/12.0.1
Application Build ID: 20120428123100

Comment 2

[Alexander Bergmann]

Hi Hashem,

thanks for looking into this! :-)

I am the developer of Mail Merge, an add-on for Thunderbird, and I do have reports from some users, who could reproduce this problem easily.

You did close the message compose window after you saved the message as draft the first time? And you did open the saved message and saved it as draft again?

When inserting the images, can you try to use "Insert -> HTML" and insert the following HTML:
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />

So, I guess I missed one step in the description:
1. Create a new message
2. Insert one image multiple times, e.g.:
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />
3. Save the message as draft or template
4. Close the message compose window
5. Open the saved message
6. Save the message as draft or template

And again, the placeholder image does only show in the current editor of the message compose window after you have saved the message as draft the second time. If you close the message compose window afterwards and take a look at the saved message, the original image shows fine.

Comment 3

[Thomas D. (:thomas8)]

Confirming on TB12.0.1 on WinXP
Immediately reproducable with steps from comment 2

This is the resulting code after step 6 of comment 2 (select images, then "Insert > HTML" to check)

<img
src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/Drafts?number=12345678&amp;part=1.2&amp;filename=favicon.ico">
<img
src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/Drafts?number=12345678&amp;part=&amp;filename=favicon.ico">
<img
src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/Drafts?number=12345678&amp;part=&amp;filename=favicon.ico">

Out of the three images, only the first one still shows right, 2 and 3 are broken. I notice that the working img has part=1.2, but broken images have part= and then nothing, so there is no part number. Could that be the cause of this bug?

Comment 4

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #3)
> Confirming on TB12.0.1 on WinXP
> Immediately reproducable with steps from comment 2

That's on a local POP account.

Comment 5

[Hashem Masoud]

(In reply to myaddons from comment #2)
Agreed. When you save the draft the 2nd time, only the 1st picture appears. But when you close and re-open it, all of them appear fine.

Comment 6

[Thomas D. (:thomas8)]

(In reply to Hashem Masoud from comment #5)
> (In reply to myaddons from comment #2)
> Agreed. When you save the draft the 2nd time, only the 1st picture appears.
> But when you close and re-open it, all of them appear fine.

That's right. The problem is, any time you save draft again manually, the other 2 pics will disappear again, and if you send from that situation, they are not sent. Hashem, did you see duplicates of this? Otherwise, we could confirm.

Comment 7

[cmoller]

(In reply to Thomas D. from comment #3)
... 
> <img
> src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/
> Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/
> Drafts?number=12345678&amp;part=1.2&amp;filename=favicon.ico">
> <img
> src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/
> Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/
> Drafts?number=12345678&amp;part=&amp;filename=favicon.ico">
> <img
> src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/
> Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/
> Drafts?number=12345678&amp;part=&amp;filename=favicon.ico">
> 
> Out of the three images, only the first one still shows right, 2 and 3 are
> broken. I notice that the working img has part=1.2, but broken images have
> part= and then nothing, so there is no part number. Could that be the cause
> of this bug?

I can confirm the same behaviour especially the "part=1.2" is replaced with only "part="

The result is not only the image not displaying correctly, but the email will not send to any recipient.

Windows 7, Thunderbird 12.01 on two machines.

Comment 8

[Thomas D. (:thomas8)]

Similar bugs, but not exactly dupes afaics:
bug 560210, bug 560877, bug 566120

Comment 9

[Thomas D. (:thomas8)]

The source code of that same msg in my Sent folder is also completely weird (messed up):

The HTML part has
    <img src="cid:[e-mail]"> <img
      src="cid:[e-mail]"> <img
      src="cid:[e-mail]">
...which looks wrong because the cid's should either all point to the same picture (one source for all), or, less efficiently, each to a different picture with its own source included (3 sources).

[e-mail] is correctly included as icon source

Then follows another HTML part (which itself has the ID of 2nd and 3rd graphic src from 1st HTML part above!?!) which still has references to the graphics from the draft message (which no longer exists, as this already been sent)!?? :

Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-ID: <[e-mail]>
[snip]
    <img src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/Drafts?number=28804591&part=1.2"> <img
      src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/Drafts?number=28804591&part=1.2"> <img
      src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/Drafts?number=28804591&part=1.2">

This looks totally bogus to me.
But the confusions seem obvious enough that it might be possible to track them down relatively "easily" for someone who knows ;)

Comment 10

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #9)
> src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/
> Thunderbird/Profiles/abc1abc2.default/Mail/pop.gmx.net/
> Drafts?number=28804591&part=1.2">

...I forgot to manually replace Drafts?number=28804591 with Drafts?number=12345678 as I did in comment 9 when sanitizing the source for privacy (surely the original draft numbers must have been the same).

Comment 11

[Thomas D. (:thomas8)]

According to comment 0, this regression happened between TB9 and TB10.
Confirming as this is reproducable and has something tangible in the source code of affected messages. It might still be a duplicate, but it's too hard to check.

Comment 12

[Thomas D. (:thomas8)]

Attachment: Draft1.eml (after 1st save; OK)

This is the source of the draft after 1st manual saving (from drafts folder). All identical images referenced correctly, using same source.

Comment 13

[Thomas D. (:thomas8)]

Attachment: Draft3.editor.html (after 2nd save+reopen; generally ok, but why reference part 1.2 which doesn't exist in draft?)

After reopening the saved draft (step 5 of comment 2) in editor, the img src for all images correctly points to the same part 1.2 of the draft in drafts folder - although I don't understand why we are using part 1.2 which does not seem to exist in the source of the draft. It seems to work though, all identical images are shown in editor.

Comment 14

[Thomas D. (:thomas8)]

Attachment: Draft4.editor.html (after 3rd save; broken: part no missing for 2nd and 3rd img)

This is where editor breaks the img references:
1st img has src "...Drafts?number=12345678&part=1.2&filename=favicon.ico"
2nd img has src "...Drafts?number=12345678&part=&filename=favicon.ico"
3rd img has src "...Drafts?number=12345678&part=&filename=favicon.ico"

Comment 15

[Thomas D. (:thomas8)]

Attachment: Draft5.outbox.eml (totally broken and messed up)

When trying to send jumbled draft4 (after step 6 of comment 2), of course we end up with something broken. However, the complete mess of parts we generate is really amazing:

--------------040306020108020705050309
Content-Type: multipart/related;
 boundary="------------020906030102020800040206"


--------------020906030102020800040206
Content-Type: text/html; charset=ISO-8859-1
[snip]
img1: src="cid:[e-mail]"
img2: src="cid:[e-mail]"
img3: src="cid:[e-mail]"
(src of img2 and img3 is wrong)

--------------020906030102020800040206
Content-Type: image/x-icon

(the mail should end here; instead, we have another text/html part which is completely bogus at this time! Note that the wrong src references/cid's of img2 and img3 point to the content ID of this extra html part!: [e-mail])

--------------020906030102020800040206
Content-Type: text/html; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-ID: <[e-mail]>

<html>
  <head>
    <meta http-equiv="content-type" content="text/html;
      charset=ISO-8859-1">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <u><img alt="" src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/Thunderbird/Profiles/abc1abc2.default/Mail/pop.bar.com/Drafts?number=28882265&part=1.2&filename=favicon.ico" height="16"
        width="16"><img alt="" src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/Thunderbird/Profiles/abc1abc2.default/Mail/pop.bar.com/Drafts?number=28882265&part=1.2&filename=favicon.ico"
        height="16" width="16"><img alt=""
        src="mailbox:///C:/Dokumente%20und%20Einstellungen/User/Anwendungsdaten/Thunderbird/Profiles/abc1abc2.default/Mail/pop.bar.com/Drafts?number=28882265&part=1.2&filename=favicon.ico" height="16" width="16"></u>
  </body>
</html>

Comment 16

[Thomas D. (:thomas8)]

Considering the complete multipart garbage structure that we create and send out, this looks major to me.

Comment 17

[Thomas D. (:thomas8)]

Ludo, who can find code of comment 14, where the following happens:
- when user manually saves reopened draft (with multiple identical images)
- in img src for subsequent identical images (except 1st img),
- part no. (part=1.2) gets omitted

Comment 18

[Thomas D. (:thomas8)]

Confirming for trunk, too (on WinXP, Pop3).

Ludo, any bug candidates within the regression range (per comment 0, between TB9 and TB10) that were tweaking drafts/multipart behaviour and could have caused this?

Comment 19

[Arivald]

(In reply to Thomas D. from comment #18)
> Confirming for trunk, too (on WinXP, Pop3).
> 
> Ludo, any bug candidates within the regression range (per comment 0, between
> TB9 and TB10) that were tweaking drafts/multipart behaviour and could have
> caused this?

Bug 692875 - editing an existing draft with inline images doesn't handle mime part renumbering when new images are added before existing images 

We did not test for multiple images with same URL, probably renumbered is only first one.

Comment 20

[Joe Sabash [:JoeS1]]

(In reply to Arivald from comment #19)
> (In reply to Thomas D. from comment #18)
> > Confirming for trunk, too (on WinXP, Pop3).
> > 
> > Ludo, any bug candidates within the regression range (per comment 0, between
> > TB9 and TB10) that were tweaking drafts/multipart behaviour and could have
> > caused this?
> 
> Bug 692875 - editing an existing draft with inline images doesn't handle
> mime part renumbering when new images are added before existing images 
> 
> We did not test for multiple images with same URL, probably renumbered is
> only first one.

Ah, but I did test this, but only in the case of locally embedded images, and not remote images.
So, for remote images in the form of <img src="http://www.mozilla.org/favicon.ico" />
it seems that the save draft/template assumes that it should be converted to an inline image.
Not a bad assumption in that the moz-do-not-send="true" attribute was not set.
So we should probably not make that assumption in the code and test for "http" in the image source.

Anyway...if you paste in html code with that attribute, things work fine.
example:
<img alt=""
      src="http://img408.imageshack.us/img408/1973/rss14iq7.gif"
      moz-do-not-send="true" height="14" width="14"><br>
    <img alt=""
      src="http://img408.imageshack.us/img408/1973/rss14iq7.gif"
      moz-do-not-send="true" height="14" width="14"><br>
    <img alt=""
      src="http://img408.imageshack.us/img408/1973/rss14iq7.gif"
      moz-do-not-send="true" height="14" width="14"><br>

There may be some cases where you do see placeholders instead of images.
But the "remoteness" is preserved if you include that attribute.

Comment 21

[David :Bienvenu]

nsMsgComposeAndSend::PreProcessPart ignores the duplicate image parts because they're marked as m_bogus_attachment, which means we don't put it in the m_partNumbers array. I think this is because we weed out the duplicate parts earlier. I think when we do this, we need to make sure we have an entry in m_partsNumber for the duplicate parts.

Comment 22

[David :Bienvenu]

Attachment: proposed fix

this fixes the STR's described in this case, though I think it probably needs a bit of user testing. I'll generate a try server build for JoeS and anyone else interested to try.

Comment 23

[David :Bienvenu]

I requested a try server build - windows and mac builds should show up here - http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/[e-mail]-5ff4941ce502

Comment 24

[Joe Sabash [:JoeS1]]

Testing the try build:
Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/15.0 Thunderbird/15.0a1 ID:20120525173756
Seems to work perfectly here.
It even pre-processes the duplicate remote source into one actual embed
+1 on that I didn't expect that to happen.

Comment 25

[Thomas D. (:thomas8)]

(In reply to David :Bienvenu from comment #22)
> Created attachment 627413 [details] [diff] [review]
> proposed fix
> 
> this fixes the STR's described in this case, though I think it probably
> needs a bit of user testing. I'll generate a try server build for JoeS and
> anyone else interested to try.

David, that's great and so quick, thank you!
Confirming that tryserver build fixes the bigger issues for me.

There are some smaller issues when more (same) images are added sequentially, say in different editing sessions of same draft on different days:

STR

1) insert 3 identical graphics as described in comment 0
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />
2) save draft, close(!), reopen (edit draft)

3) insert another set of same 3 identical graphics, so you now have 6 identical graphics
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />
<img src="http://www.mozilla.org/favicon.ico" />
4) save draft, close (!)
4b) View Source of draft (Ctrl+U)
4c) edit draft, view HTML source of draft (Ctrl+A, Insert > HTML)

Actual result:

4b) (tags are less nicely formatted in real life...)
<img src="cid:[e-mail]">
<img src="cid:[e-mail]">
<img src="cid:[e-mail]">
<br>
<br>
<img src="cid:[e-mail]">
<img src="cid:[e-mail]">
<img src="cid:[e-mail]">
...
--------------020907080805080906080004
Content-Type: image/x-icon
Content-Transfer-Encoding: base64
Content-ID: <[e-mail]>

AAABAAIAEBAAAAEACABoBQAAJgAAABAQAAABACAAaAQAAI4FAAAoAAAAEAAAACAAAAABAAgA
...
--------------020907080805080906080004
Content-Type: image/x-icon;
 name="favicon.ico"
Content-Transfer-Encoding: base64
Content-ID: <[e-mail]>
Content-Disposition: inline;
 filename="favicon.ico"

AAABAAIAEBAAAAEACABoBQAAJgAAABAQAAABACAAaAQAAI4FAAAoAAAAEAAAACAAAAABAAgA
...

4c) 
<img src="mailbox:///C:/.../Profiles/a1aaaa1a.Daily15tmp/Mail/pop.gmx.net/Drafts?number=12345&amp;part=1.2">
<img src="mailbox:///C:/.../Profiles/a1aaaa1a.Daily15tmp/Mail/pop.gmx.net/Drafts?number=12345&amp;part=1.2">
<img src="mailbox:///C:/.../Profiles/a1aaaa1a.Daily15tmp/Mail/pop.gmx.net/Drafts?number=12345&amp;part=1.2">
<br>
<br>
<img src="mailbox:///C:/.../Profiles/a1aaaa1a.Daily15tmp/Mail/pop.gmx.net/Drafts?number=12345&amp;part=1.3&amp;filename=favicon.ico"></u>
<img src="mailbox:///C:/.../Profiles/a1aaaa1a.Daily15tmp/Mail/pop.gmx.net/Drafts?number=12345&amp;part=1.3&amp;filename=favicon.ico">
<img src="mailbox:///C:/.../Profiles/a1aaaa1a.Daily15tmp/Mail/pop.gmx.net/Drafts?number=12345&amp;part=1.3&amp;filename=favicon.ico">

Problems:

A) Bug: Content-Disposition and filename gets lost
-> that should be fixed

> Content-Disposition: inline;
>  filename="favicon.ico"
This part (both content-disposition and filename) gets lost!
(if you save, close and reopen more often, it will get lost for all images!)

B) Inefficient, minor: For 6 (2x3) identical images (albeit added in different edit sessions), *one* embedded mime source would be enough
-> optional depending on work and costs

For users who add multiple identical images in different editing sessions, we still create multiple embedded instances of same image.
For optimum size of mail, it should be only one embedded source.
I don't know how hard it is, and what the costs are, to do this:

When preprocessing, test for identical mime sources:
- for all inline image sources, check size of source part (assuming that's faster than doing checksums)
- if size is identical
  - if only 2 identical image sizes: compare mime sources
    - if mime sources are identical: embed as single source
  - elseif more than 2 identical image sizes:
    - compare checksums (MD5 or something like that)
      - if checksums are identical, compare mime sources (to be sure)
        - if mime sources are identical, combine into single mime source and
          modify CID references accordingly
      - elseif checksums are different: nothing to optimize
- elseif size is different: nothing to optimize

C) Trivial: Be more coherent about part numbering
-> Cosmetic, but nice to have

We create multipart message:
Part1: html (parent part)
Part2: img (child part)
part3: img (child part)

In composition editor, these parts are coherently identified:
Part1: html
Part2: Drafts?...;part=1.2;...
Part3: Drafts?...;part=1.3;...
(That's a nice and intelligible numbering)

The same parts, from Draft Source or when sent, confusingly numbered like this:
Part1: html
Part2: cid:[e-mail] (why part1? this is the 2nd part)
Part3: cid:[e-mail] (why part4? there is no part 3!)

A nicer numbering, more coherent with parts numbering in editor, would be this:
Part1: html
Part2: cid:[e-mail] (2nd part, that's part=1.2 in editor)
Part3: cid:[e-mail] (3rd part, that's part=1.3 in editor)

Comment 26

[Alexander Bergmann]

I can confirm that the try server build fixes my initial problem! :-)
However, I can also confirm the (minor) issues mentioned by Thomas in comment #22.

Comment 27

[Alexander Bergmann]

I can confirm that the try server build fixes my initial problem! :-)
However, I can also confirm the (minor) issues mentioned by Thomas in comment #25 (not in comment #22, sorry).

Comment 28

[David :Bienvenu]

thx for testing the try build! Are the minor issues regressions caused by my patch, or were they pre-existing issues?

Comment 29

[Alexander Bergmann]

I've just run a quick test with the current Thunderbird 12.0.1 and the (minor) issues do also happen there. So I guess they are not caused by your patch.

Comment 30

[Alexander Bergmann]

And some other tests: The (minor) issues in comment #25 do also happen with Thunderbird 3.1.20(!) and Thunderbird 9.

Comment 31

[Joe Sabash [:JoeS1]]

This issue:
>A) Bug: Content-Disposition and filename gets lost
> that should be fixed
>
> Content-Disposition: inline;
>  filename="favicon.ico"
>This part (both content-disposition and filename) gets lost!

Has been around for a very long time (I can't find a dup, but I swear that I filed this on Netscape 7.2 :)

Let's just spin those minor bugs off after this one is reviewed and landed.

Comment 32

[Thomas D. (:thomas8)]

(In reply to Joe Sabash from comment #31)
> This issue:
> >A) Bug: Content-Disposition and filename gets lost
> > that should be fixed
> >
> > Content-Disposition: inline;
> >  filename="favicon.ico"
> >This part (both content-disposition and filename) gets lost!

I can also confirm for TB12.0.1 that issue of A) was there before this patch.

> Has been around for a very long time (I can't find a dup, but I swear that I
> filed this on Netscape 7.2 :)

From time immemorial... *sigh*... I should have guessed that, silly me!

> Let's just spin those minor bugs off after this one is reviewed and landed.

That's perfectly fine with me.

Comment 33

[David :Bienvenu]

fixed on trunk

Comment 34

[David :Bienvenu]

http://hg.mozilla.org/comm-central/rev/e9ae5fd80443

Comment 35

[Thomas D. (:thomas8)]

Due to the dataloss nature of this bug (image data lost when sending), and the straightforward, minimal fix - can we land this on at least aurora, too?

Comment 36

[David :Bienvenu]

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

[Triage Comment]
I'm fine with landing this on Aurora, though it's more datalossy than dataloss

Comment 37

[David :Bienvenu]

landed on aurora - http://hg.mozilla.org/releases/comm-aurora/rev/7c14c5924721