Closed Bug 1658890 Opened 4 years ago Closed 4 years ago

Adding custom headers to an email or a newsgroup article through mail.compose.other.header is broken

Categories

(Thunderbird :: Message Compose Window, defect, P1)

Tracking

(thunderbird_esr78+ fixed, thunderbird81 fixed)

RESOLVED FIXED
82 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird81 --- fixed

People

(Reporter: infofrommozilla, Assigned: rnons)

References

(Regression, )

Details

(Keywords: regression)

Attachments

(4 files, 6 obsolete files)

Adding custom headers to an email or a newsgroup article through mail.compose.other.header is broken.

STR:

  • Add an header to the pref: mail.compose.other.header e.g. X-Header1
  • Open a new compose window
  • add an additional header through clicking onto 》next to the BCC button.
  • select X-Header1
  • add some text eg: foo bar
  • send mail/article

Instead of the new header X-Header, a header...
Null: "foo bar"
... is added.

Confirming for 78.1.1 (64-bit). Works in 68.11.0 (32-Bit).
Alice, could you find the regression window?

Severity: -- → S3

(In reply to Alfred Peters from comment #0)

Adding custom headers to an email or a newsgroup article through mail.compose.other.header is broken.

Thank you Alfred! Good find!

This should be from the pills re-design.

No longer blocks: tb-enterprise

(In reply to Magnus Melin [:mkmelin] from comment #4)

This should be from the pills re-design.

Indeed. Bug 440377 and bug 1603166.

Thanks Alice!

Regressed by: tb-pills
Assignee: nobody → remotenonsense
Priority: -- → P1
Attached patch 1658890.patch (obsolete) — Splinter Review

This patch sets aria-labelledby to the input inside input-container, then when a pill is created, aria-labelledby will be set to pill.emailInput.
https://searchfox.org/comm-central/source/mail/base/content/mailWidgets.js#2337-2339
https://searchfox.org/comm-central/source/mail/components/compose/content/addressingWidgetOverlay.js#81-83

I think there is a separate issue, if I click the Send button directly without blurring the custom header input first, the header won't be included. Same for other address inputs.

Attachment #9170554 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED

There is another problem: if X-Header1 has value a; b; c or a, b, c, according to https://searchfox.org/comm-central/source/mail/components/compose/content/addressingWidgetOverlay.js#81-83, only the last pill will be used, so X-Header1: c will be included in the sent message. For other address pills, we use , to concatenate them, but in the case of custom headers, we can't be sure user typed , or ;. I think we should change the custom header input to be a plain input without pills, like the Subject input.

Attached patch 1658890-no-pill.patch (obsolete) — Splinter Review

This patch disables pill in the custom header input. This is better than the patch in comment 9, because it also fixes comment 10.

Attachment #9170580 - Flags: review?(mkmelin+mozilla)

(In reply to Ping Chen (:rnons) from comment #9)

I think there is a separate issue, if I click the Send button directly without blurring the custom header input first, the header won't be included. Same for other address inputs.

I guess that also causes bug 1658509?

(In reply to Ping Chen (:rnons) from comment #11)

Created attachment 9170580 [details] [diff] [review]
1658890-no-pill.patch

This patch disables pill in the custom header input. This is better than the patch in comment 9, because it also fixes comment 10.

Yes, it works - so far.

An important function of these additional headers is to set the 'Supersedes' header in newsgroups. This header is used to replace a newsgroup article with a revised version.
To do this, the Message-ID of the article to be replaced must be entered in the header:

Supersedes: <r17ebb.68g.1@gammasigma.xy>

But if I enter the Message-ID, TB cuts off the angle brackets.
This can be seen already in the Pill.

(In reply to Ping Chen (:rnons) from comment #11)

Created attachment 9170580 [details] [diff] [review]
1658890-no-pill.patch

I noticed something else: 8-bit characters are not handled correctly.

X-Header1: Test äöü
becomes:
X-Header1: =?UTF-8?B?VGVzdCDvv73vv73vv70=?=
but that decodes to:
X-Header1: Test
(= <u+FFFD>, the 'Replacement Character')

X-Header2: Test 😃
becomes:
X-Header2: =?UTF-8?Q?Test_=3d=03?=
which decodes to:
X-Header2: Test =

But this might be a completely different bug...

{Just a warning: If you wanna test Comment 13 or Comment 14 - Current Trunk builds do crash on viewing new news articles due Bug 1657493.
But you should be able to test it by email as well.}

(In reply to Alfred Peters from comment #14)

I noticed something else: 8-bit characters are not handled correctly.
But this might be a completely different bug...

Right, Gene and I would have fixed that in bug 1658361. Get a TB 78.2 pre-build an try it there:
https://mail.mozilla.org/pipermail/tb-planning/2020-August/007760.html

(In reply to Jorg K (CEST = GMT+2) from comment #16)

Right, Gene and I would have fixed that in bug 1658361. Get a TB 78.2 pre-build an try it there:

I tested it with current trunk. That build should already have the Bug 1658361 patch.
But TB 78.2 pre-build hasn't the patch of this bug.

Apologies, Alfred, the fix was related to addressing headers. Can your issue from comment #14 be reproduced with a normal mail message?
I created a message with

Subject: Test =?UTF-8?B?w6TDtsO8?=
X-Header-1: =?UTF-8?B?w6TDtsO8?=

and that's correctly displayed correctly as äöü. =?UTF-8?B?VGVzdCDvv73vv73vv70=?= appears to be wrong to start with.

(In reply to Jorg K (CEST = GMT+2) from comment #18)

Apologies, Alfred, the fix was related to addressing headers. Can your issue from comment #14 be reproduced with a normal mail message?

Yes.

I created a message with

Subject: Test =?UTF-8?B?w6TDtsO8?=
X-Header-1: =?UTF-8?B?w6TDtsO8?=

and that's correctly displayed correctly as äöü.

"create a message"? This bug isn't about saved messages.

Compose just a new mail and send it. See STR.

=?UTF-8?B?VGVzdCDvv73vv73vv70=?= appears to be wrong to start with.

That is what TB made of it.

But again: The encoding problem is not that urgent.
It would be nice, if Comment 13 could be fixed in this bug (Ping Chen?).
And then I'll open a new bug for Comment 14.

(In reply to Alfred Peters from comment #19)

"create a message"? This bug isn't about saved messages.
Compose just a new mail and send it. See STR.

OK, got it, sorry. If per the STR in comment #0 you create non-ASCII-content in the custom header, then it ends up badly in the message. Understood. It's just a bit confusing that comment #0 says Null: "foo bar" created, when in comment #14 you show X-Header-1. Anyway, once the custom headers are restored, we should test them with non-ASCII content.

(In reply to Jorg K (CEST = GMT+2) from comment #20)

Understood. It's just a bit confusing that comment #0 says Null: "foo bar" created, when in comment #14 you show X-Header-1. Anyway, once the custom headers are restored, we should test them with non-ASCII content.

That's with the patch of Comment 11. It works fine - just with the nits of Comment 13 and Comment 14.

Well, I don't consider mis-encoding a "nit". Mojibake is just not tolerable anywhere. So if we restore the function, it needs to restored to its previous glory.

Attached patch 1658890-no-pill.patch (obsolete) — Splinter Review

Sorry for being late, I misread comment 13 and thought comment 14 was a separate issue. This patch

I did some testing, message-id is unchanged. Test äöü becomes =?UTF-8?B?VGVzdCDDpMO2w7w=?= and Test 😃 becomes =?UTF-8?B?VGVzdCDwn5iD?=. Please see if it works for you now.

Attachment #9170554 - Attachment is obsolete: true
Attachment #9170580 - Attachment is obsolete: true
Attachment #9170554 - Flags: review?(mkmelin+mozilla)
Attachment #9170580 - Flags: review?(mkmelin+mozilla)
Attachment #9171319 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9171319 [details] [diff] [review]
1658890-no-pill.patch

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

I don't seem to get any new headers showing up

I've added this to user.js, and id5 is correct:

user_pref("mail.identity.id5.headers", "archive, supercedes");
user_pref("mail.identity.id5.headers", "archive");
user_pref("mail.identity.id5.header.archive", "X-No-Archive: yes");
user_pref("mail.identity.id5.header.supercedes", "Supercedes: <1234@example.com>");

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +983,5 @@
>  
>    let container = document.getElementById(rowID);
>    let input = container.querySelector(`input[is="autocomplete-input"]`);
> +  if (!input) {
> +    input = container.querySelector(`input`);

nit: Just normal quotes "input" when there's no need for backticks
Attachment #9171319 - Flags: review?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #24)

I don't seem to get any new headers showing up

I've added this to user.js, and id5 is correct:

user_pref("mail.identity.id5.headers", "archive, supercedes");
user_pref("mail.identity.id5.headers", "archive");
user_pref("mail.identity.id5.header.archive", "X-No-Archive: yes");
user_pref("mail.identity.id5.header.supercedes", "Supercedes:
<1234@example.com>");

I will take a look next week. But even if no headers show up, it's a separate issue. This bug is about mail.compose.other.header and typing the header content manually in the compose window. As I understand, mail.identity.id5.headers are default custom headers appended to every sent message.

(In reply to Ping Chen (:rnons) from comment #23)

Created attachment 9171319 [details] [diff] [review]
1658890-no-pill.patch

I did some testing, message-id is unchanged. Test äöü becomes =?UTF-8?B?VGVzdCDDpMO2w7w=?= and Test 😃 becomes =?UTF-8?B?VGVzdCDwn5iD?=. Please see if it works for you now.

Yes, works here also.

(In reply to Ping Chen (:rnons) from comment #25)

(In reply to Magnus Melin [:mkmelin] from comment #24)

I don't seem to get any new headers showing up

I've added this to user.js, and id5 is correct:

user_pref("mail.identity.id5.headers", "archive, supercedes");

I will take a look next week. But even if no headers show up, it's a separate issue. This bug is about mail.compose.other.header and typing the header content manually in the compose window. As I understand, mail.identity.id5.headers are default custom headers appended to every sent message.

I agree.

@Magnus Melin:
There must be only one pref "mail.identity.id5.headers".
And there is no space allowed in the value.

user_pref("mail.identity.id5.headers", "archive,supercedes");
user_pref("mail.identity.id5.header.archive", "X-No-Archive: yes");
user_pref("mail.identity.id5.header.supercedes", "Supercedes: <1234@example.com>");

This works for me. But this wasn't broken at all. ;-)

Comment on attachment 9171319 [details] [diff] [review]
1658890-no-pill.patch

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

Ah, I'll check that. We have so many obscure features...
Attachment #9171319 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9171319 [details] [diff] [review]
1658890-no-pill.patch

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

Ok, so I added

user_pref("mail.compose.other.header", "Approved,Supercedes");

... and those headers show up in the overflow. 

Clicking around there (not sure exactly which sequence) I get
JavaScript error: chrome://messenger/content/messengercompose/MsgComposeCommands.js, line 4738: TypeError: can't access property "focus", container.querySelector(...) is null 

Then I write "yes" in the Approved header and try to do Send later. This results in 

JavaScript error: resource:///modules/jsmime/jsmime.js, line 3409: TypeError: cannot use 'in' operator to search for "email" in "yes"

... and a pretty broken message in the Outbox
Attachment #9171319 - Flags: review?(mkmelin+mozilla) → review-

(In reply to Magnus Melin [:mkmelin] from comment #29)

user_pref("mail.compose.other.header", "Approved,Supercedes");

Then I write "yes" in the Approved header and try to do Send later. This
results in

JavaScript error: resource:///modules/jsmime/jsmime.js, line 3409:
TypeError: cannot use 'in' operator to search for "email" in "yes"

I can confirm this with the Approved header. It has a special meaning.
I guess it must be an email address. But that doesn't work either.

This was already broken before, see: Bug 1204740

Then I write "yes" in the Approved header and try to do Send later. This

I guess it must be an email address. But that doesn't work either.

https://dxr.mozilla.org/comm-central/rev/2a29ee0adb310b54a6a2df72034953fed8f2b043/comm/mailnews/mime/jsmime/jsmime.js#167

// From RFC 5536:
addHeader("Approved", parseAddress, writeAddress);

JFTR

(In reply to Alfred Peters from comment #30)

(In reply to Magnus Melin [:mkmelin] from comment #29)

This was already broken before, see: Bug 1204740
=> Bug 1180209 Comment 8

Now I see more upcoming problems.

  • Can we handle an email address for newsgroups and plain strings for email?
  • What happens to passwords/strings with 8-bit chars? Do the mailing list servers decode a MIME encoded string?
See Also: → 1180209

(In reply to Magnus Melin [:mkmelin] from comment #29)

Comment on attachment 9171319 [details] [diff] [review]
1658890-no-pill.patch

Review of attachment 9171319 [details] [diff] [review]:

JavaScript error: resource:///modules/jsmime/jsmime.js, line 3409:
TypeError: cannot use 'in' operator to search for "email" in "yes"

Ok, finally all these /special/ headers will throw the above error, because they need an address (like To etc.) output:
https://dxr.mozilla.org/comm-central/search?q=addHeader+parseAddress++file%3Ajsmime.js&redirect=false

@Ping Chen: Could you please change the field type accordingly.

Because of the Approved header:
It would be nice to set the field type based on the account type:

  • Newsgroup account -> address data
  • Email account -> plain text

That should be enough to select the right parser in jsmime.js.
If you wish, I'll do the changes in jsmime.js in Bug 1180209, , but feel free to do it yourself.

Attached patch 1658890-no-pill.patch (obsolete) — Splinter Review
  • Fix can't access property "focus", container.querySelector(...) is null of comment 29
  • Revert to use setRawHeader to fix Approved header

From https://searchfox.org/comm-central/source/mailnews/mime/public/msgIStructuredHeaders.idl#175-176, setRawHeader expects the arguments to be ACString. When using setRawHeader in JS, the header value is by default a UTF-16 string. This caused the encoding error of comment 14. This patch adds a utf16StringToBinaryString to convert header value before passing it to setRawHeader. This should fix all known problems and bug 1180209, please confirm.

Attachment #9171319 - Attachment is obsolete: true
Attachment #9171618 - Flags: review?(mkmelin+mozilla)
Attached patch 1658890-no-pill.patch (obsolete) — Splinter Review

Adds browser_customHeaders.js to test encoding of non-ASCII custom header value and Approved header. Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=031bab14e4d04197998ec37b610911&selectedTaskRun=aILavSBFRQa0yCrEAcDsaA.0

Attachment #9171618 - Attachment is obsolete: true
Attachment #9171618 - Flags: review?(mkmelin+mozilla)
Attachment #9171624 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9171624 [details] [diff] [review]
1658890-no-pill.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +4145,5 @@
>    let pills = row.querySelectorAll("mail-address-pill");
>    let input = row.querySelector(
>      `input[is="autocomplete-input"][recipienttype]`
>    );
> +  if (input) {

Please add a comment that this case is about custom headers which may not use pills.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +82,5 @@
> +    }
> +  }
> +
> +  for (let otherHeaderRow of document.querySelectorAll(
> +    "[data-labeltype=addr_other]"

This is a very wide net, please make it more specific to improve performance. 
I guess ``.address-row[data-labeltype="addr_other"]`` should do it

@@ +984,5 @@
>  
>    let container = document.getElementById(rowID);
>    let input = container.querySelector(`input[is="autocomplete-input"]`);
> +  if (!input) {
> +    input = container.querySelector('input');

please prefer double quotes

I'd just write 
let input = container.querySelector(`input[is="autocomplete-input"]`) || container.querySelector("input")

::: mail/test/browser/composition/browser_customHeaders.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Test mail.compose.other.header is rendered and handled correctly.

Yay for tests!

@@ +37,5 @@
> +
> +  // Set values to custom headers.
> +  let cwc = open_compose_new_mail();
> +  let inputs = cwc.window.document.querySelectorAll(
> +    "[data-labeltype=addr_other] input"

Please update this accordingly per above.
For the attr values (like addr_other), I think it's preferable to always quote even if it's not strictly needed.

@@ +57,5 @@
> +    !draftMsgLines.some(
> +      line => line.trim() == "X-Header1: =?UTF-8?B?VGVzdCDDpMO2w7w=?="
> +    )
> +  ) {
> +    Assert.ok(false, "X-Header1 not found or incorrect");

Could be better to not use an if-clause but instead Assert the thing you have in if

::: mailnews/mime/src/MimeJSComponents.jsm
@@ +214,5 @@
> +   * See msgIWritableStructuredHeaders for more details.
> +   * NOTE: aValue is expected to be ACString, when calling this function from
> +   * JS, need to convert JS string to 8-bit string first.
> +   * jsmime.mimeutils.utf16StringToBinaryString is provided for this purpose.
> +   */

This all seems hacky. I think the problem is that ACString is ASCII only. 
Please change setRawHeader to take AUTF8String instead and then I think things should just work I think.
Attachment #9171624 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch 1658890-no-pill.patch (obsolete) — Splinter Review

Updates to comment 36.

Please change setRawHeader to take AUTF8String instead and then I think things should just work I think.

It works on my local. Let's see if Try run passes https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c5b5c8dc81af7a601cceb47ec80e66d2fdd67801

Attachment #9171624 - Attachment is obsolete: true
Attachment #9171643 - Flags: review?(mkmelin+mozilla)

(In reply to Ping Chen (:rnons) from comment #37)

Created attachment 9171643 [details] [diff] [review]
1658890-no-pill.patch

I'm happy with it.

Comment on attachment 9171643 [details] [diff] [review]
1658890-no-pill.patch

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

Looks good, just a couple of minor things

::: mail/base/content/mailWidgets.js
@@ +2149,5 @@
>      /**
>       * Create a new recipient row container with the input autocomplete.
>       *
> +     * @param {Object} recipient - An object for various element attributes.
> +     * @param {boolean} disablePill - A flag to disable pill and autocompletion.

would "rawInput" be a better name?

::: mail/test/browser/composition/browser_customHeaders.js
@@ +19,5 @@
> +);
> +
> +var draftsFolder;
> +
> +add_task(function setupModule(module) {

setupModule was a mozmill thing. Now without mozmill there's no need to have this. 
You can just set that up inside the test.

@@ +56,5 @@
> +  Assert.ok(
> +    draftMsgLines.some(
> +      line => line.trim() == "X-Header1: =?UTF-8?B?VGVzdCDDpMO2w7w=?="
> +    ),
> +    "X-Header1 not found or incorrect"

These messages are now backwards. 
Under a normal run one should get something like

Ok  - Correct X-Header1 found

Now one would see

Ok - X-Header1 not found or incorrect
Attachment #9171643 - Flags: review?(mkmelin+mozilla) → review+

Update to comment 39.

Attachment #9171643 - Attachment is obsolete: true
Attachment #9171835 - Flags: review+

Comment on attachment 9171835 [details] [diff] [review]
1658890-no-pill.patch

[Approval Request Comment]
Regression caused by (bug #): bug 440377
User impact if declined: Custom header name becomes Null in sent mail
Testing completed (on c-c, etc.): Completed
Risk to taking this patch (and alternatives if risky): Type of header value in setRawHeader is changed from ACString to AUTF8String, but all tests passed, so it should be OK.

Attachment #9171835 - Flags: approval-comm-esr78?
Attachment #9171835 - Flags: approval-comm-beta?

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/ca5683d5593d
Fix null custom header name when sending message. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

Comment on attachment 9171835 [details] [diff] [review]
1658890-no-pill.patch

[Triage Comment]
approved for beta

Attachment #9171835 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 9171835 [details] [diff] [review]
1658890-no-pill.patch

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

Our p≡p add-on for Thunderbird also uses custom headers, that's why we're interested in this bug.
Looks like a few nits have slipped through the review. You're basically re-dedicating `setRawHeader()` not to take a "raw" UTF-8 byte sequence any more, but instead a (UTF-16-encoded) JS string. Therefore, it would have been preferable to remove the "Raw" from the function name.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +6,5 @@
>  /* import-globals-from MsgComposeCommands.js */
>  
>  var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
>  var { MimeParser } = ChromeUtils.import("resource:///modules/mimeParser.jsm");
> +var { jsmime } = ChromeUtils.import("resource:///modules/jsmime.jsm");

Unneeded import.

::: mailnews/mime/public/msgIStructuredHeaders.idl
@@ +176,1 @@
>                      in string aCharset);

The documentation no longer reflects what the function is doing. The charset parameter is now obsolete.

::: mailnews/mime/src/MimeJSComponents.jsm
@@ +209,5 @@
>    setAddressingHeader(aHeaderName, aAddresses) {
>      this.setHeader(aHeaderName, fixXpconnectAddresses(aAddresses));
>    },
>  
>    setRawHeader(aHeaderName, aValue, aCharset) {

charset parameter not used.

On second thought, the changes to setRawHeader() should be reversed and instead a separate interface for use in JS should be created. From the JS call site https://hg.mozilla.org/comm-central/rev/ca5683d5593d#l3.42, no UTF-8 raw byte string is passed but rather a UTF-16-encoded JS string which should be mapped onto AString. This is similar to the difference between parseEncodedHeader() and parseEncodedHeaderW(). Please read
https://searchfox.org/comm-central/rev/7156143da17c283114591163b3074e622484ee6e/mailnews/mime/public/nsIMsgHeaderParser.idl#96

Not sure that's accurate. UTF8String and AString are just the UTF-8 and UTF-16 versions for idl smart string parameters. We're just passing around strings here so why would it make a difference?

Yes, the charset param should be removed.

If you remove the charset parameter, you should also rename the function since you're visiting all the call sites anyway.

As was said, the re-dedicated function now takes a AUTF8String parameter instead of a ACString parameter. For JS callers that means that XPCOM will have to do an UTF-16 to UTF-8 conversion, and then back to UTF-16 since the function itself is provided in JS. AUTF8Sting is a single-byte interface whereas AString is a two-byte interface. What is implemented works, but comes at the cost of two runtime conversions. You might want to check with an IDL expert here.

Remove the charset argument from setRawHeader function.

About the function name, I think the raw refers to the header value being raw/unstructured, that's why parseStructuredHeader is used.

About the performance, I think the conversion happens when the value is used (going out of xpconnect), not when the function is called (going into xpconnect).

From https://searchfox.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#821,831,842,847, when an xpcom function is called from JS, XPCVariant::newVariant is used, but no conversion happens. Then if this xpcom function is implemented in JS, XPCVariant::VariantDataToJS is used.

From https://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCVariant.cpp#379,385,388, if the variant->GetAsJSVal is already a JS primitive value, it's returned directly without conversion.

In the case of setRawHeader, when called from C++, nsACString ---> XPConnect ---> JS UTF16, the conversion was previously done by jsmime.headerparser.convert8BitHeader, and now by xpconnect, no performance degradation. When called from JS, JS UTF16 ---> XPConnect ---> JS UTF16, no conversion happens, performance is improved.

Of course this is just my understanding, I'm not an expert.

Attachment #9172949 - Flags: review?(mkmelin+mozilla)
Attachment #9172949 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9172949 [details] [diff] [review]
1658890-charset.patch

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

Ping, thanks for addressing this quickly and the XPCOM research :-)

Sadly the use of AUTF8String is not well documented in https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPIDL, it only says: "Most of the time you don't want to use this type but AUTF8String or ACString" in one spot, but there is no direct comparison of when to use AUTF8String vs. AString. Looking at M-C coding practise, AUTF8String is used, mostly in the network component, even if the access is done (mostly) from JS. I have the impression that interfaces just got changed from ACString to AUTF8String when things became non-ASCII, for example in URLs, to avoid C++ changes that would have come from a change to AString.

However, I think this will *BREAK* newsgroups with UTF-8-encoded names :-( - See below. I suggest not to land this without further tests.

::: mailnews/compose/src/nsMsgCompUtils.cpp
@@ +313,5 @@
>      rv = nntpService->GenerateNewsHeaderValsForPosting(
>          newsgroups, getter_Copies(newsgroupsHeaderVal),
>          getter_Copies(newshostHeaderVal));
>      NS_ENSURE_SUCCESS(rv, rv);
> +    finalHeaders->SetRawHeader("Newsgroups", newsgroupsHeaderVal);

While apparently the other call sites pass in ACSII data, this one passes in a header which was obtained from GetRawHeader() and may contain UTF-8 as byte string. In this case, the line that was removed, aValue = jsmime.headerparser.convert8BitHeader(aValue, aCharset); would be needed. To be on the safe side, leaving the semantics of the original function untouched and adding a new function would be the way to go.

To be 100% clear: It's likely that newsgroups with UTF-8 names are already broken after the first patch. If you need examples, turn to bug 1389762 for examples (news.newsfan.net). Apparently there are many newsgroups with non-ASCII names encoded in UTF-8 and also other charsets (but the latter don't work).

(note that nntp is pretty much busted on trunk for other reasons - bug 1661955)

Right, non-UTF8 names are not supported, but UTF-8 names are. As for getRawHeader():
https://searchfox.org/comm-central/rev/7156143da17c283114591163b3074e622484ee6e/mailnews/mime/jsmime/jsmime.js#1873
If in doubt ask :jcranmer. It appears that setRawHeader() and getRawHeader() are a matched pair, and now the semantic of one of them was changed. Maybe NNTP still works on beta, or you can do a try build of ESR.

Thanks for your interest in this bug.

To be 100% clear: It's likely that newsgroups with UTF-8 names are already broken after the first patch. If you need examples, turn to bug 1389762 for examples (news.newsfan.net).

There is no way my last week's patch can cause a 3-years old bug. I appreciate if you want to contribute some test cases to https://searchfox.org/comm-central/source/mailnews/compose/test/unit/test_messageHeaders.js#309

Right, non-UTF8 names are not supported, but UTF-8 names are. As for getRawHeader():
https://searchfox.org/comm-central/rev/7156143da17c283114591163b3074e622484ee6e/mailnews/mime/jsmime/jsmime.js#1873
If in doubt ask :jcranmer. It appears that setRawHeader() and getRawHeader() are a matched pair, and now the semantic of one of them was changed.

Seems to me the getRawHeader function in jsmime is not related to msgIWritableStructuredHeaders.getRawHeader at all.

I would be happy to fix if you can prove something is broken by 1658890-no-pill.patch, for other problems, please consider filing new bugs.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/25dd31b9c909
followup - Remove charset from setRawHeader. r=mkmelin

Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED

Reference to: https://bugzilla.mozilla.org/show_bug.cgi?id=1661458

With TB 78.2.1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:78.0) Gecko/20100101 Thunderbird/78.2.1

In header I see the same as before:
Null: 5b30489b-bc40.............

Supersedes don't work

In my user.js
user_pref("mail.compose.other.header", "Supersedes");

What should I do now?

(In reply to Ping Chen (:rnons) from comment #56)

There is no way my last week's patch can cause a 3-years old bug. I appreciate if you want to contribute some test cases to https://searchfox.org/comm-central/source/mailnews/compose/test/unit/test_messageHeaders.js#309

You misunderstood. The three-year-old bug is about raw non-UTF-8 newsgroups. We believe that now UTF-8 newsgroups are broken. Have you tested that?

Seems to me the getRawHeader function in jsmime is not related to msgIWritableStructuredHeaders.getRawHeader at all.

Isn't msgIStructuredHeaders.getRawHeader() implemented here:
https://searchfox.org/comm-central/rev/e75f551c5deb7740ac08a13cb9e488b1c4ed3f82/mailnews/mime/jsmime/jsmime.js#1892
What am I missing and what is msgIWritableStructuredHeaders.getRawHeader?

:FritzS, it's not uplifted to esr78 yet. Can you please try again with beta or nightly?

Seems to me the getRawHeader function in jsmime is not related to msgIWritableStructuredHeaders.getRawHeader at all.

Isn't msgIStructuredHeaders.getRawHeader() implemented here:
https://searchfox.org/comm-central/rev/e75f551c5deb7740ac08a13cb9e488b1c4ed3f82/mailnews/mime/jsmime/jsmime.js#1892
What am I missing and what is msgIWritableStructuredHeaders.getRawHeader?

You missed https://searchfox.org/comm-central/source/mailnews/mime/src/MimeJSComponents.jsm#185 and https://searchfox.org/comm-central/source/mailnews/mime/src/MimeJSComponents.jsm#108

That was missed indeed, apologies! Please take a look at bug 1662350. We couldn't get to the stage to post to Test.UTF8.测试1 since the subscription dialogue was already broken (which may be unrelated). Posting there in TB 78 still appears to work (but the message never appears in the group, perhaps one needs an account there).

This is 1658890-charset.patch rebased to the esr78 repo. I'm wondering is it really necessary to uplift this patch, since it doesn't fix any bug.

Flags: needinfo?(mkmelin+mozilla)
Attachment #9173297 - Flags: review+

That one probably isn't strictly needed for 78 since it's just code clean-up. Does the main patch (the other one) apply cleanly to 78?

Flags: needinfo?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #64)

Does the main patch (the other one) apply cleanly to 78?

Yes, a hunk is resolved automatically.

applying 1658890-no-pill.patch
patching file mail/base/content/mailWidgets.js
Hunk #1 succeeded at 2155 with fuzz 1 (offset 10 lines).
now at: 1658890-no-pill.patch

(In reply to Ping Chen (:rnons) from comment #63)

This is 1658890-charset.patch rebased to the esr78 repo. I'm wondering is it really necessary to uplift this patch, since it doesn't fix any bug.

Avoids merge conflicts if further patches in the affected areas need to be uplifted.

(In reply to p≡p Project from comment #52)

To be 100% clear: It's likely that newsgroups with UTF-8 names are already broken after the first patch.

That statement is wrong, I tested it on a Daily build and it still works.

(In reply to p≡p Project from comment #62)

Please take a look at bug 1662350.

That bug is invalid.

Apologies!

Comment on attachment 9171835 [details] [diff] [review]
1658890-no-pill.patch

[Triage Comment]
Approved for esr78

Attachment #9171835 - Flags: approval-comm-esr78? → approval-comm-esr78+

Backout Thunderbird 78.2.2 for causing crash:
https://hg.mozilla.org/releases/comm-esr78/rev/b84e7103a735

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Caused a crash in bct3 tests on all platforms when uplifted to c-esr78.
https://treeherder.mozilla.org/#/jobs?repo=comm-esr78&selectedTaskRun=Xf7CHVzfSkSB2IqGs-KWtQ.0&revision=aaaa4c8a07e0f41c6f4c571cd560552cbe84e978

Verified backout with try jobs:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c4efa77787dc6a456320c2299db5749f546395b7
and
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8a461c2cbd7e43537dd7505d475e51c96d27b6ce

The second try job included only the no-pill patch to see if the crash was caused by the charset patch somehow. It failed as well. bct3 was green without either patch.

Ping, can you take a look? We didn't see this error on beta.

Flags: needinfo?(remotenonsense)

(In reply to Rob Lemley [:rjl] from comment #71)

Caused a crash in bct3 tests on all platforms when uplifted to c-esr78.

Saw JavaScript Error: "uncaught exception: [fluent] Missing translations in en-US: remove-address-row-type." in the log. That's strange, it worked on my local when preparing 1658890-charset-esr78.patch. And the remove-address-row-type entry can be found in https://hg.mozilla.org/releases/comm-esr78/file/aaaa4c8a07e0f41c6f4c571cd560552cbe84e978/mail/locales/en-US/messenger/messengercompose/messengercompose.ftl#l8.

Anyway, I found the code throwing the error is redundant, the tooltip will be overwritten by https://hg.mozilla.org/releases/comm-esr78/file/aaaa4c8a07e0f41c6f4c571cd560552cbe84e978/mail/components/compose/content/MsgComposeCommands.js#l4176. After removing the code, browser_customHeaders pass on my local again.

Flags: needinfo?(remotenonsense)
Attachment #9173926 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9173926 [details] [diff] [review]
1658890-fix-test-esr78.patch

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

Not sure why this causes a problem, but let's remove it if it's causing problems.
Attachment #9173926 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9171835 [details] [diff] [review]
1658890-no-pill.patch

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

Hmmm, why are we still attaching all sorts of autocomplete attributes and an autocomplete-only function to an other header input (aka raw input) which doesn't autocomplete?
This looks unfinished and needs cleanup. I have a patch.

::: mail/base/content/mailWidgets.js
@@ +2149,5 @@
>      /**
>       * Create a new recipient row container with the input autocomplete.
>       *
> +     * @param {Object} recipient - An object for various element attributes.
> +     * @param {boolean} rawInput - A flag to disable pill and autocompletion.

pills

@@ +2240,5 @@
>        input.setAttribute("completedefaultindex", true);
>        input.setAttribute("forcecomplete", true);
>        input.setAttribute("completeselectedindex", true);
>        input.setAttribute("minresultsforpopup", 2);
>        input.setAttribute("ignoreblurwhilesearching", true);

All of these attributes only apply for autocomplete inputs, why are we setting them for rawHeader inputs?

@@ +2255,2 @@
>        });
>        input.onBeforeHandleKeyDown = event => {

onBeforeHandleKeyDown() is run from the autocomplete toolkit code, so it won't run for raw header. Why are we still attaching this function to raw header inputs although it will never run there?

@@ +2264,5 @@
>  
>        input.setAttribute("recipienttype", recipient.type);
>        input.setAttribute("size", 1);
>  
> +      if (!rawInput) {

Having these small rawInput conditionals all over the place makes this code very hard to understand. So combined with all of the above, this needs a thorough cleanup where code applicable to raw input vs. autocomplete input gets bundled each in their own condition.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3915,5 @@
>            row: `addressRow${header}`,
>            label: `${header}AddrLabel`,
>            labelId: header,
>            container: `${header}AddrContainer`,
> +          class: "",

Good change. Let's hope this won't have side effects.

@@ +4738,5 @@
> +    let input =
> +      container.querySelector(
> +        `input[is="autocomplete-input"][recipienttype]`
> +      ) || container.querySelector("input");
> +    input.focus();

querySelector can be simplified into ".address-input[recipienttype]", and that's short enough then to remove the input variable which is only used once.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +984,5 @@
>  
>    let container = document.getElementById(rowID);
> +  let input =
> +    container.querySelector(`input[is="autocomplete-input"]`) ||
> +    container.querySelector("input");

Again, querySelector can be simplified to match both autocomplete and raw input as shown above.
Blocks: 1663526
Regressions: 1663583

Is this going to ship in the next release? It would be handy to be able to add custom headers again.

Flags: needinfo?(rob)
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
See Also: → 195716
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: