Closed
Bug 1322103
Opened 8 years ago
Closed 8 years ago
Opening a draft containing embedded image that was saved twice before gives blocked content notification
Categories
(Thunderbird :: Message Compose Window, defect)
Thunderbird
Message Compose Window
Tracking
(thunderbird52+ fixed, thunderbird53 fixed)
RESOLVED
FIXED
Thunderbird 53.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
Details
Attachments
(1 file, 1 obsolete file)
3.58 KB,
patch
|
mkmelin
:
review+
jorgk-bmo
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
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•8 years ago
|
tracking-thunderbird52:
--- → +
Assignee | ||
Comment 1•8 years 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•8 years ago
|
||
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•8 years 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•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
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•8 years 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•8 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-thunderbird52:
--- → affected
status-thunderbird53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
Assignee | ||
Comment 8•8 years ago
|
||
FRG, can you please check this in SM. I landed something on suite/ here.
Flags: needinfo?(frgrahl)
Assignee | ||
Comment 9•8 years 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•8 years 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•8 years ago
|
Flags: needinfo?(frgrahl)
Assignee | ||
Comment 11•8 years 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•8 years ago
|
||
Could you set me on cc on both. Marked as security relevant.
Assignee | ||
Comment 13•8 years ago
|
||
Comment 14•8 years ago
|
||
Jorg thanks for the cc.
Assignee | ||
Comment 15•8 years 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.
Description
•