Closed Bug 1322103 Opened 4 years ago Closed 4 years ago

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


(Thunderbird :: Message Compose Window, defect)

Not set


(thunderbird52+ fixed, thunderbird53 fixed)

Thunderbird 53.0
Tracking Status
thunderbird52 + fixed
thunderbird53 --- fixed


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




(1 file, 1 obsolete file)

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:

img src=mailbox:///..../Drafts?number=45
Blocks: 1322155
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: pMsgComposeParams->SetOriginalMsgURI(originalMsgURI);

This comes from
CreateTheComposeWindow(fields, newAttachData, nsIMsgCompType::Draft, composeFormat, mdd->identity, mdd->originalMsgURI, mdd->origMsgHdr);

Looks like it's populated here:

So maybe a MIME problem after all, it wouldn't be the first one.
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?
Assignee: nobody → jorgk
Attachment #8821675 - Flags: review?(mkmelin+mozilla)
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.
SM had the same typo, so let's fix this as a courtesy.
Attachment #8821675 - Attachment is obsolete: true
Attachment #8821675 - Flags: review?(mkmelin+mozilla)
Attachment #8822284 - Flags: review?(mkmelin+mozilla)
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
Attachment #8822284 - Flags: review?(mkmelin+mozilla) → review+
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
FRG, can you please check this in SM. I landed something on suite/ here.
Flags: needinfo?(frgrahl)
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.
Attachment #8822284 - Flags: approval-comm-aurora+
'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.
Flags: needinfo?(frgrahl)
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.
Could you set me on cc on both. Marked as security relevant.
Jorg thanks for the cc.
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.
Depends on: 1379912
You need to log in before you can comment on or make changes to this bug.