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

VERIFIED FIXED in Thunderbird 10.0

Status

MailNews Core
Composition
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

Trunk
Thunderbird 10.0
x86_64
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
See bug 380372 for more info...
(Assignee)

Comment 1

7 years ago
This also affects editing a message as new, in some cases. We could just reload the saved message into the editor, which would fix everything up, but it could be quite slow, especially in the imap case. We'd like auto-save to be as inconspicuous as possible. 

I'm not sure how we can fix draft saving to let the compose window know about the correct mime part numbers.
(Assignee)

Comment 2

7 years ago
I poked around the message editing and saving code for quite a while and this could be very tricky. I thought I could come up with some mapping between mime parts numbers and cid's, and see if that changes from the time we bring the message into the editor, to when we save as draft, but we regenerate cid's every time we save a message, so we can't use the cids. I had thought about figuring out the mime part numbers by changing nsMsgSendPart to calculate and store the part number, and number the child parts correctly as we build up the tree of parts. But we still need some way of figuring out what the old parts were to see if part1.1.x is different. 

Perhaps we could set an attribute on the image elements when we load the message that is the part number, and then update that when we save the message. Or, perhaps just set an attribute when we save the message, and use that in the ResetEmbeddedUri's code.

Comment 3

7 years ago
Setting attribute on image node probably will fail when user edit HTML. I mean existing Insert/HTML option, or any extension that allows HTML editing.


I think that this parts numbers sould me generated by part of code which actually handle MIME encoding and generate all MIME parts. For example it may generate some mapping table, then some higher level code will use it to update all image URI in edited message.

Generated table with mapping may be: 
  key - URI in message, which have been turned into MIME part.
  value 
    - string with part number (ex 1.3, or 1.1.2).
    or
    - whole replacement URI, with part number too

Maybe nsMsgComposeAndSend::ProcessMultipartRelated can generate this maping table? 
Or even it may update HTML... It seems it already do some temporary updates to message DOM.
(Assignee)

Comment 4

7 years ago
The only place I can see to figure out the part numbers is in the SendPart handling, as I said earlier. You can't figure them out from the editor. What I might have to do is grab the new cid from the nsMsgAttachmentHandler when each send part is created, and store that in the send part as well (or create a hash table mapping between cid and mime part number stored in the send part, and hang that hash table off the mMsgSend nsMsgComposeAndSend object), and when resetembeddeduri's is called, look up the mime part number from the cid.
(Assignee)

Comment 5

7 years ago
Created attachment 568580 [details] [diff] [review]
proposed fix

This is what I had in mind. It seems to be working fine for me. I'll generate a try server build with it, though I haven't had much luck with try server for the past month or so.

The basic approach is that whenever we count multipartrelated attachments, we remember the index in the editor's embedded object list of each domElement as a an attribute called nodeIndex. This gets called right before we save a message as a draft so it's up to date info, and handles new images, deleting old images, etc, before saving.

Then, when building up the mime multipart related message to save, we retrieve the node index from the dom node, and stick it in the nsMsgAttachmentData for the attachment. This gets transferred to the nsMsgAttachmentHandler for the attachment. 

Next, when we're building up the nsMsgSendParts for the message, we store the mime part number in each nsMsgSendPart. In nsMsgComposeAndSend::PreProcessPart, we store the mapping between the dom node index for the nsMsgAttachmentHandler and the corresponding nsMsgSendPart part number in the nsMsgSend object.

Finally, after the draft is saved, we use the mapping to find the up to date part number for the embedded image and update the spec to use the new part number.
Assignee: nobody → dbienvenu
Status: NEW → ASSIGNED
Attachment #568580 - Flags: review?(neil)

Comment 7

7 years ago
That try build appears to be a debug ? build 28.8 MB looking for MSVCR80D.dll
(which I don't have on this test system.)
It may run on another system I have with the free MS visual studio tools loaded.
I'll try there later.

Comment 8

7 years ago
Well, although I was able to get that build to run, it is way too crashy to do any meaningful testing.

Comment 9

7 years ago
Comment on attachment 568580 [details] [diff] [review]
proposed fix

>+      m_attachments[i].mNodeIndex = attachment.mNodeIndex;
As far as I can tell, attachment.mNodeIndex is always the same as locCount at this point. I say that because I crashed the code here:
+  if (ma->mNodeIndex != -1)
+    m_partNumbers.InsertElementAt(ma->mNodeIndex, part->m_partNum);
when testing on a message with a) two attached images b) both images inserted twice in the message.
Attachment #568580 - Flags: review?(neil) → review-
(Assignee)

Comment 10

7 years ago
(In reply to neil@parkwaycc.co.uk from comment #9)
> Comment on attachment 568580 [details] [diff] [review] [diff] [details] [review]
> proposed fix
> 
> >+      m_attachments[i].mNodeIndex = attachment.mNodeIndex;
> As far as I can tell, attachment.mNodeIndex is always the same as locCount
> at this point. I say that because I crashed the code here:
> +  if (ma->mNodeIndex != -1)
> +    m_partNumbers.InsertElementAt(ma->mNodeIndex, part->m_partNum);
> when testing on a message with a) two attached images b) both images
> inserted twice in the message.

do you mean a message with two images as attachments, and then both those images inserted inline twice? Or just a message with two images inserted twice? I tried the latter w/o a problem; I'll try the former.

- David
(Assignee)

Comment 11

7 years ago
simple steps to reproduce the crash would be helpful. I'm not seeing it here.
(Assignee)

Comment 12

7 years ago
Joe, I don't know why the try server build would be debug, but that might account for the crashiness you saw.
(Assignee)

Comment 13

7 years ago
Joe, yeah, I gave you a link to the debug build, not realizing we do both debug and opt builds now, and the opt build seems to have not been generated. We have a new try server infrastructure now, and I'm just getting used to it.
(Assignee)

Comment 14

7 years ago
Ooops, I was running the wrong build - Neil, I do see the crash. Will investigate.
(Assignee)

Comment 15

7 years ago
OK, I can fix the crash, but the larger issue is that when we duplicate inline images, we only generate one cid part, but the code that's avoiding the duplicate parts doesn't communicate this downstream. I think I can remember it in the attachment data such that the duplicate parts know to use the part number of the original part.

Re locCount always being the same as nodeIndex, I believe if this code is hit:

http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#1570

the counts can get off.
(Assignee)

Comment 16

7 years ago
Created attachment 569391 [details] [diff] [review]
fix crash, handle duplicate images

wow, this code is fun to work with. This fixes the crash, and fixes the duplicate images problem.
Attachment #568580 - Attachment is obsolete: true
Attachment #569391 - Flags: review?(neil)

Comment 18

7 years ago
This might be a better link to the try builds:
http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/bienvenu@nventure.com-4ae823da8af6/
I prefer the zip builds for testing myself
Tested the following scenarios:
Inserted images before, after and in between existing images
Tested with multiple insertions of a single attachment
(Important for limiting message size for multiple references..thanks for noticing that Niel)

All of the above seem to work fine.

Probably beyond the scope of this bug , there remains problems with attachment content other than images.

Linked text attachments for one.
Steps to reproduce that condition:
Highlight some text in an html composition
Insert>>Link and embed some content (maybe a sound file)

Notice that link attachment does not get converted to the Draft uri when saved

Don't get me wrong here, this fix is a great step in the right direction and much appreciated. But it seems there is more to be done.

Comment 19

7 years ago
I also run couple of tests, and all seems to work corectly. Good work :-)

As I understand, this fix will be in TB 10 final version?
(Assignee)

Comment 20

7 years ago
(In reply to Arivald from comment #19)
> I also run couple of tests, and all seems to work corectly. Good work :-)
> 
> As I understand, this fix will be in TB 10 final version?

If it gets reviewed and landed before the next aurora branch, then, yes, TB 10, otherwise, TB 11.
Comment on attachment 569391 [details] [diff] [review]
fix crash, handle duplicate images

>+          // look for earlier part with the same content id. If we find it,
>+          // need to remember the mapping between our node index and the
>+          // part num of the earlier part.
>+          for (PRUint32 j = 0; j < i; j++)
>+            if (!m_attachments[j].m_contentId.IsEmpty() &&
>+                m_attachments[j].m_contentId.Equals(m_attachments[i].m_contentId))
>+            {
>+              PRInt32 nodeIndex = m_attachments[i].mNodeIndex;
>+              if (nodeIndex != -1)
>+              {
>+                if (nodeIndex >= (PRInt32) m_partNumbers.Length())
>+                  m_partNumbers.SetLength(nodeIndex + 1);
>+                if (m_attachments[j].mNodeIndex != -1)
>+                  m_partNumbers[nodeIndex] = m_partNumbers[m_attachments[j].mNodeIndex];
>+              }
>+            }
This part is very confusingly written. At the very least you should check that m_attachments[i].mNodeIndex != -1 before doing anything else, and it's probably worth checking m_attachments[j].mNodeIndex != -1 before the contentId.

>+    if (ma->mNodeIndex >= m_partNumbers.Length())
>+      m_partNumbers.SetLength(ma->mNodeIndex + 1);
We know how many embedded objects there are so can't we preallocate the array?

>+        if (node)
>+        {
>+          nsString nodeIndex;
>+          nodeIndex.AppendInt(i);
>+          domElement->SetAttribute(NS_LITERAL_STRING("nodeIndex"), nodeIndex);
>+        }
>         bool acceptObject = false;
>         rv = GetEmbeddedObjectInfo(node, &attachment, &acceptObject);
>         if (NS_SUCCEEDED(rv) && acceptObject)
>           count ++;
>         else
>           mEmbeddedObjectList->DeleteElementAt(i);
So, as you mentioned in your comment, this line means that the embedded object list gets out of sync. I think it would make sense to remove this, because:

>     bool acceptObject = false;
>     rv = GetEmbeddedObjectInfo(node, &attachment, &acceptObject);
>     NS_ENSURE_SUCCESS(rv, NS_ERROR_MIME_MPART_ATTACHMENT_ERROR);
>+    m_attachments[i].mNodeIndex = attachment.mNodeIndex;
> 
>     if (!acceptObject)
>         continue;
a) we check that the object is acceptable anyway, and
b) we can then simplify m_attachments[i].mNodeIndex = locIndex;
c) we can remove the mNodeIndex field on the attachment object.
The only place to be careful is to move the i++ from inside the for to the end of the block so that it doesn't happen for the !acceptObject case.

> NS_IMETHODIMP
>+nsMsgComposeAndSend::GetPartForDomIndex(PRInt32 aDomIndex, nsACString &aPartNum)
>+{
>+  aPartNum = m_partNumbers[aDomIndex];
m_partNumbers.SafeElementAt(aDomIndex, EmptyCString());
or NS_ENSURE_ARG_RANGE perhaps?

Comment 22

7 years ago
I have one concern about "nodeindex" attribute. 
It is not very good that this is added permamently to message. I think it should be removed at some point, when it is no longer needed.

Also it should be renamed, to _moz_nodeindex, like already used _moz_quote.
(Assignee)

Comment 23

7 years ago
Created attachment 570698 [details] [diff] [review]
addressed the simple comments

this addresses the simple comments; I'll address the rest later today.
Attachment #569391 - Attachment is obsolete: true
Attachment #569391 - Flags: review?(neil)
(Assignee)

Comment 24

7 years ago
(In reply to neil@parkwaycc.co.uk from comment #21)
> So, as you mentioned in your comment, this line means that the embedded
> object list gets out of sync. I think it would make sense to remove this,
> because:
> 
>> a) we check that the object is acceptable anyway, and
> b) we can then simplify m_attachments[i].mNodeIndex = locIndex;
> c) we can remove the mNodeIndex field on the attachment object.
> The only place to be careful is to move the i++ from inside the for to the
> end of the block so that it doesn't happen for the !acceptObject case.
>

It would be great if that approach works - I think it means that I can get rid of the nodeIndex on the dom node as well. I think you mean locCount, not locIndex... I'll give it a try.
(Assignee)

Comment 25

7 years ago
Created attachment 570853 [details] [diff] [review]
address rest of comments
Attachment #570698 - Attachment is obsolete: true
Attachment #570853 - Flags: review?(neil)
Comment on attachment 570853 [details] [diff] [review]
address rest of comments

Time to link libxul again...

>+          // look for earlier part with the same content id. If we find it,
>+          // need to remember the mapping between our node index and the
>+          // part num of the earlier part.
>+          for (PRUint32 j = 0; j < i; j++)
>+          {
>+            PRInt32 nodeIndex = m_attachments[i].mNodeIndex;
>+            if (nodeIndex != -1 && (m_attachments[j].mNodeIndex != -1) &&
>+               !m_attachments[j].m_contentId.IsEmpty() &&
>+                m_attachments[j].m_contentId.Equals(m_attachments[i].m_contentId))
Nit: I would move the nodeIndex != -1 check out of the loop. Also I'm still not sure why you check for m_attachments[j].m_contentId.IsEmpty() - if you're worried about comparing two attachments with empty content IDs then you might as well check for m_attachments[i].m_contentId.IsEmpty() up front as well.

>-    j++;
>+    j++; 
Nit: trailing space crept in here.

Updated

7 years ago
Attachment #570853 - Flags: review?(neil) → review+
(Assignee)

Comment 27

7 years ago
sadly, I found a case that doesn't work with my newest patch (removing an old inline image and adding the same one later in the message, iirc, or vice versa). I'll see if that worked with my previous patch before I changed the way we handled the embeddedobject list.
(Assignee)

Comment 28

7 years ago
Created attachment 571854 [details] [diff] [review]
patch for checkin

this seems to be working better - it does a better job of ignoring the attachments we used to delete.
(Assignee)

Comment 29

7 years ago
fixed on trunk http://hg.mozilla.org/comm-central/rev/1dd58de982ee
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 10.0

Comment 30

7 years ago
I tested this fix for as many combinations of image changes that I could think of including drag and drop of existing images to change their order. All works fine.
Thanks a lot for sticking with this David.
I can now use auto-save for the very first time.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 31

7 years ago
Joe, thx to you and Arivald for you help with this.
Blocks: 487945
Duplicate of this bug: 718454
Arivald (Bug 754655 Comment 19) suspects that this might be related with or causing Bug 754655:

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

Bug 754655 uncovers that for multiple identical inline images, part number is missing for all but 1st image, hence subsequent images break.

David?

Updated

4 years ago
Depends on: 714825
You need to log in before you can comment on or make changes to this bug.