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

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 53.0

Thunderbird Tracking Flags

(thunderbird52+ fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 months ago
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
(Assignee)

Updated

5 months ago
tracking-thunderbird52: --- → +
(Assignee)

Updated

5 months ago
Blocks: 1322155
(Assignee)

Comment 1

4 months ago
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.
(Assignee)

Comment 2

4 months ago
Created attachment 8821675 [details] [diff] [review]
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?
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8821675 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 3

4 months ago
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.
(Assignee)

Comment 4

4 months ago
Green try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6eeb14c76ee28a38e5d8793e357bf099d5eba657
(Assignee)

Comment 5

4 months ago
Created attachment 8822284 [details] [diff] [review]
1322103-originalMsgURI.patch (v1) + SM part

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 6

4 months ago
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+
(Assignee)

Comment 7

4 months ago
https://hg.mozilla.org/comm-central/rev/f63a0b7059c7e99e7a3435d22a0d0cb9fa35857f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-thunderbird52: --- → affected
status-thunderbird53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Comment 8

4 months ago
FRG, can you please check this in SM. I landed something on suite/ here.
Flags: needinfo?(frgrahl)
(Assignee)

Comment 9

4 months ago
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+

Comment 10

4 months ago
'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.

Updated

4 months ago
Flags: needinfo?(frgrahl)
(Assignee)

Comment 11

4 months ago
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

4 months ago
Could you set me on cc on both. Marked as security relevant.
(Assignee)

Comment 13

4 months ago
Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/d9b7c9856c1a2537f33e3b7d99ba0fb365f31076
status-thunderbird52: affected → fixed

Comment 14

4 months ago
Jorg thanks for the cc.
(Assignee)

Comment 15

4 months ago
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.
You need to log in before you can comment on or make changes to this bug.