moz-do-not-send not always set for images inserted from http source

RESOLVED FIXED in Thunderbird 53.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
8 months ago
8 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)

(Assignee)

Description

8 months ago
Insert a picture from a http source.
Click "Attach image to message". Result: moz-do-not-send="false"

Insert another picture from a http source.
Don't click anything, default is not to include the image.
Result: moz-do-not-send not set.

If it's not set, then it will be attached based on pref mail.compose.attachHttp.

If that's set to true, the image will be attached despite the dialogue that showed "do not attach".

So I suggest to always write the attribute moz-do-not-send according to the checkbox setting.

What do you think Magnus, how did you envisage this?
(Assignee)

Updated

8 months ago
Blocks: 1322155
(Assignee)

Comment 1

8 months ago
Created attachment 8817838 [details] [diff] [review]
1322859-always-set-moz-do-not-send.patch (v1)

Another fine-tuning issue. The indentation needed fine-tuning, too.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8817838 - Flags: review?(mkmelin+mozilla)

Comment 2

8 months ago
Comment on attachment 8817838 [details] [diff] [review]
1322859-always-set-moz-do-not-send.patch (v1)

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

Yes, r=mkmelin

Would you mind also setting the initial checkbox for http according to the pref?
https://dxr.mozilla.org/comm-central/rev/f10e840262ddbbad24db1642906cf6643622665f/mailnews/compose/content/mailComposeEditorOverlay.xul#95

Something like

if (/^file:/i.test(gMsgCompInputElement.value)) {
  attach = true;
}
else if (/^https?:/i.test(gMsgCompInputElement.value)) {
  attach = Services.prefs.getBoolPref(""mail.compose.attach_http_images");
}
Attachment #8817838 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 3

8 months ago
Yes, I was thinking of this.

Your code isn't quite right since the panel is used for images and links. The preset only applies in the image case. See
https://dxr.mozilla.org/comm-central/rev/f10e840262ddbbad24db1642906cf6643622665f/mailnews/compose/content/mailComposeEditorOverlay.xul#34

I'll fix it ... tomorrow, today I'm working on bug 1288911.
(Assignee)

Comment 4

8 months ago
https://hg.mozilla.org/comm-central/rev/3afabe5c6f5d5659c401df23c43b7b393dcea58b
Landed with a few changes:
We need to tick the checkbox also for data URIs. Images can have been dragged in, so they have an empty attribute which we should set.

While playing around with it, I noticed the following: For data URIs you can uncheck the checkbox all you want, get moz-don-not-send="true", and nsMsgComposeAndSend::GetEmbeddedObjectInfo() can reject the object all it wants (I traced through it), but the image is still attached.

Did you know that? Is that expected? Surely, once the image is in the composition, it doesn't make much sense not to send it, but in this case the checkbox doesn't do what it says.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-thunderbird52: --- → affected
status-thunderbird53: --- → fixed
Flags: needinfo?(mkmelin+mozilla)
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Comment 5

8 months ago
Comment on attachment 8817838 [details] [diff] [review]
1322859-always-set-moz-do-not-send.patch (v1)

Not quite what was landed ;-)
Attachment #8817838 - Flags: approval-comm-aurora+

Comment 6

8 months ago
Looks like this may be causing xpcshell test failures?

TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetrySend.js | xpcshell return code: 0

    1275144 Intermittent test_TelemetrySend.js | test_backoffTimeout - [test_backoffTimeout : 245] Should have received the correct amount of type C pings - "undefined" == 1

TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetrySend.js | test_backoffTimeout - [test_backoffTimeout : 268] Should have recorded send failure times in histograms. - 0 > 0

1275144 Intermittent test_TelemetrySend.js | test_backoffTimeout - [test_backoffTimeout : 245] Should have received the correct amount of type C pings - "undefined" == 1
(Assignee)

Comment 7

8 months ago
(In reply to aleth [:aleth] from comment #6)
> Looks like this may be causing xpcshell test failures?
Absolutely not. Please check today's tree status which says:
  Z failure all platforms: Bug 1316256 (expected fix: mid-December) - X failure all platforms: Bug 1323628
Also note the M-C pushlog:
Thu Dec 15 03:39:38 2016 +0000	7652a58efa46	Phil Ringnalda — Bug 1323628 - disable test_TelemetrySend.js for sudden apparently time-based failure across all trunk trees, a=surprise
(Assignee)

Comment 8

8 months ago
Also note the Daily/Nightly run where the X is green again ;-)

Comment 9

8 months ago
good :-)
(Assignee)

Comment 10

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

Comment 11

8 months ago
Magnus, repeating the question:

A data-URI-based image is always attached, even with moz-do-not-send="true". That has been like this in TB 45 and hasn't changed.

Did you know that? Is that expected? Surely, once the image is in the composition, it doesn't make much sense not to send it, but in this case the checkbox doesn't do what it says.

Comment 12

8 months ago
I don't see that. When the attribute is actually set (to true) it's sent as is, which is what I'd expect. 
Wouldn't it have been this bug that it wasn't set. I'll have to re-check with latest...
(Assignee)

Comment 13

8 months ago
Maybe a misunderstanding here. TB 45 and Daily:
data URI with moz-do-not-send="true" will be sent out as attachment.

I traced through it, GetEmbeddedObjectInfo() returns acceptObject==false, but it still gets attached.

Comment 14

8 months ago
Retried with daily. For moz-do-not-send="true" it's not converted to cid: but sent as data:, as expected
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 15

8 months ago
Right, I see it now, I didn't look properly before. Due to the long HTML line the whole body gets turned into base64:

User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101
 Thunderbird/53.0a1
MIME-Version: 1.0
Content-Type: text/html; charset=windows-1252
Content-Language: de-DE
Content-Transfer-Encoding: base64

PGh0bWw+DQogIDxoZWFkPg0KDQogICAgPG1ldGEgaHR0cC1lcXVpdj0iY29udGVudC10eXBl
IiBjb250ZW50PSJ0ZXh0L2h0bWw7IGNoYXJzZXQ9d2luZG93cy0xMjUyIj4NCiAgPC9oZWFk
Pg0KICA8Ym9keSB0ZXh0PSIjMDAwMDAwIiBiZ2NvbG9yPSIjRkZGRkZGIj4NCiAgICA8cD48
^^^^-- that's the body, not an attachment.
You need to log in before you can comment on or make changes to this bug.