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

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jorg K (CEST = GMT+2)]

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?

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1322859-always-set-moz-do-not-send.patch (v1)

Another fine-tuning issue. The indentation needed fine-tuning, too.

Comment 2

[Magnus Melin [:mkmelin]]

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");
}

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Jorg K (CEST = GMT+2)]

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.

Comment 5

[Jorg K (CEST = GMT+2)]

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

Not quite what was landed ;-)

Comment 6

[aleth [:aleth]]

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

Comment 7

[Jorg K (CEST = GMT+2)]

(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

Comment 8

[Jorg K (CEST = GMT+2)]

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

Comment 9

[aleth [:aleth]]

good :-)

Comment 10

[Jorg K (CEST = GMT+2)]

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

Comment 11

[Jorg K (CEST = GMT+2)]

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

[Magnus Melin [:mkmelin]]

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...

Comment 13

[Jorg K (CEST = GMT+2)]

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

[Magnus Melin [:mkmelin]]

Retried with daily. For moz-do-not-send="true" it's not converted to cid: but sent as data:, as expected

Comment 15

[Jorg K (CEST = GMT+2)]

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.