1322103 - Opening a draft containing embedded image that was saved twice before gives blocked content notification

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

STR:
Create a new message. Paste an image from the clipboard. Save message as draft, close compose window.

Edit draft, save again (with or without changing the content), close compose window.

Edit draft again. Now you receive the blocked content notification.

Analysis: In MsgComposeCommands.js we do this:
  if (src.startsWith(originalMsgNeckoURI.value.spec)) {
where the originalMsgNeckoURI came from gMsgCompose.originalMsgURI

Sadly this contains a reference to a superseded draft whereas the picture contains a reference to the most recent draft. Example:

originalMsgNeckoURI.value.spec=mailbox:///.../Drafts?number=44
img src=mailbox:///..../Drafts?number=45

Comment 1

[Jorg K (CEST = GMT+2)]

Note that "edit draft" runs through RunMessageThroughMimeDraft()/mime_parse_stream_complete().

The latter sets up the "original URI" here after reading through the draft:
See: http://searchfox.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#243: pMsgComposeParams->SetOriginalMsgURI(originalMsgURI);

This comes from
http://searchfox.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#1503
CreateTheComposeWindow(fields, newAttachData, nsIMsgCompType::Draft, composeFormat, mdd->identity, mdd->originalMsgURI, mdd->origMsgHdr);

Looks like it's populated here:
http://searchfox.org/comm-central/source/mailnews/mime/src/mimedrft.cpp#2010
newPluginObj2->GetOriginalMsgURI(&mdd->originalMsgURI);

So maybe a MIME problem after all, it wouldn't be the first one.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1322103-originalMsgURI.patch (v1)

Sorry for blaming it on rock-solid MIME which was not the problem here.

MIME diligently returns the right message URL, as debugging at mimedrft.cpp#2010 showed.

There were two problems:
1) Due to an obvious typo, the originalMsgURI was not retrieved properly.
2) With that fixed, it still didn't work since it turned out that
   nsMsgCompose::CreateMessage() retrieved its version of the 
   "original URI" from a database attribute which apparently
   returned the "real original" "original URI"
   (which always has the number of when the draft was first created).

Now I'm not sure that 2) is a valid change. We've have to run the test suite over it. Maybe that was just a crook since the original URI wasn't passed in properly.

The alternative to 2) is to store params.originalMsgURI into a separate global.

What do you think?

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8821675 [details] [diff] [review]
1322103-originalMsgURI.patch (v1)

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ -1866,5 @@
>          nsCString queuedDisposition;
>          msgDBHdr->GetStringProperty(QUEUED_DISPOSITION_PROPERTY, getter_Copies(queuedDisposition));
> -        nsCString originalMsgURIs;
> -        msgDBHdr->GetStringProperty(ORIG_URI_PROPERTY, getter_Copies(originalMsgURIs));
> -        mOriginalMsgURI = originalMsgURIs;

This code was imported into Mercurial in 2008, so it's hard to tell when/why this was added initially.

Comment 4

[Jorg K (CEST = GMT+2)]

Green try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6eeb14c76ee28a38e5d8793e357bf099d5eba657

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1322103-originalMsgURI.patch (v1) + SM part

SM had the same typo, so let's fix this as a courtesy.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8822284 [details] [diff] [review]
1322103-originalMsgURI.patch (v1) + SM part

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

Great detective work! r=mkmelin

Comment 7

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/f63a0b7059c7e99e7a3435d22a0d0cb9fa35857f

Comment 8

[Jorg K (CEST = GMT+2)]

FRG, can you please check this in SM. I landed something on suite/ here.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8822284 [details] [diff] [review]
1322103-originalMsgURI.patch (v1) + SM part

We need this for Aurora TB 52 since "Thunderbird compose window is no longer an (app type) editor" landed there.

Comment 10

[Frank-Rainer Grahl (:frg)]

'Funny' error. The fix didn't fix it for SeaMonkey but also didn't make it worse.
Tested with an IMAP and local newsgroups account. If this is really fixed for TB it looks like we need to file a followup.

Comment 11

[Jorg K (CEST = GMT+2)]

Well, SM now lives in the world where bug 1151366 landed which smashed through a bunch of stuff. We tried to not smash SM completely. You have bug 1322172 to adopt to the new world.

Here I needed to pass in the original message URI to use in mailnews/ so the change to MsgComposeCommands.js which had an obvious typo was necessary.

Comment 12

[Frank-Rainer Grahl (:frg)]

Could you set me on cc on both. Marked as security relevant.

Comment 13

[Jorg K (CEST = GMT+2)]

Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/d9b7c9856c1a2537f33e3b7d99ba0fb365f31076

Comment 14

[Frank-Rainer Grahl (:frg)]

Jorg thanks for the cc.

Comment 15

[Jorg K (CEST = GMT+2)]

Enjoy reading a bug with 400 comments!

Anyway, just look at what was landed, it's pretty straight forward. However, there were a few follow-up problems, like this one. I've collected them here: bug 1322155. Fortunately this bug here was the last open problem.