1390337 - Remove some unused functions in nsIMsgDBView and nsIMsgHeaderParser

Status: RESOLVED FIXED

(MailNews Core :: MIME, enhancement)

Description

[:aceman]

Found via bug 1390051, these functions seem mostly unused:
- getMsgHdrsForSelection (unused)
- extractHeaderAddressNames (few occurences can be unrolled)
- extractHeaderAddressName (few occurences can be replaced by extractFirstName)

I also replace some uses of extractHeaderAddressMailboxes, but not all yet.

I checked these functions are NOT used by addons.

Comment 1

[:aceman]

Attachment: patch

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d2c5ffd206c7f62d48d54f93c28195687b53263d

Comment 2

[:aceman]

The failure in mozmill/newmailaccount/test-newmailaccount.js is also seen on trunk without my patch.

Comment 3

[Jorg K (CEST = GMT+2)]

No one is blaming you, there's fresh bustage every day :-( - Bug 1390431.

Comment 4

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8897180 [details] [diff] [review]
patch

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

r+, assuming you fix the various encoding errors.

This sort of "when do we apply RFC 2047 decoding" madness is why I disliked these helper methods in the first place.

::: mail/base/content/mailWindowOverlay.js
@@ +2002,5 @@
>  function MsgCreateFilter()
>  {
>    // retrieve Sender direct from selected message's headers
>    var msgHdr = gFolderDisplay.selectedMessage;
> +  let emailAddresses = MailServices.headerParser.parseDecodedHeader(msgHdr.author);

You'll want to use msgHdr.mime2DecodedAuthor here.

@@ +3123,5 @@
>                   aMimeHdr.extractHeader("Return-Receipt-To", false); // not
>      let fromHdr = aMimeHdr.extractHeader("From", false);
>  
> +    let mdnAddr = MailServices.headerParser.parseDecodedHeader(mdnHdr).map(addr => addr.email).join(", ");
> +    let fromAddr = MailServices.headerParser.parseDecodedHeader(fromHdr).map(addr => addr.email).join(", ");

These two functions should be parseEncodedHeader (note that we're getting the raw MIME headers from extractHeader, not the RFC 2047-decoded header).

::: suite/mailnews/mailWindowOverlay.js
@@ +2628,5 @@
>      var headerParser = MailServices.headerParser;
>      // update the allow remote content for sender string
>      var mailbox = headerParser.extractHeaderAddressMailboxes(aMsgHdr.author);
>      var emailAddress = mailbox || aMsgHdr.author;
> +    var displayName = headerParser.extractFirstName(aMsgHdr.author);

Ditto, this should be mime2DecodedAuthor.

Comment 5

[:aceman]

Attachment: patch v1.1

Sure, thanks for improving the correctness of the code.

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8899290 [details] [diff] [review]
patch v1.1

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

Sorry about the drive-by comments:

I don't understand the logic of this:
extractHeaderAddressName() is replaced by extractFirstName(). OK.
extractHeaderAddressNames() only has two callers in test, so they get dealt with.
extractHeaderAddressMailboxes() is declared deprecated, yet it is a handy abbreviation for:
return this.parseDecodedHeader(aLine).map(addr => addr.email).join(", ");

There are a few callers and you replace some of them, not all by copying the code there in mailWindowOverlay.js:
+    let mdnAddr = MailServices.headerParser.parseEncodedHeader(mdnHdr).map(addr => addr.email).join(", ");
+    let fromAddr = MailServices.headerParser.parseEncodedHeader(fromHdr).map(addr => addr.email).join(", ");
What's the point of that?

Why is that function deprecated in the first place? And you copy its code around to some callers but not all?

::: mailnews/mime/public/nsIMsgHeaderParser.idl
@@ +200,5 @@
>     * @param aLine          The header line to parse.
>     * @return               A comma-separated list of just the mailbox parts
>     *                       of the email-addresses.
>     */
>    /* @deprecated */ ACString extractHeaderAddressMailboxes(in ACString aLine);

So this one isn't removed although you replace some calls?

Comment 7

[:aceman]

Yes, I do not remove extractHeaderAddressMailboxes yet.

(In reply to Jorg K (GMT+2) from comment #6)
> There are a few callers and you replace some of them, not all by copying the
> code there in mailWindowOverlay.js:
> +    let mdnAddr =
> MailServices.headerParser.parseEncodedHeader(mdnHdr).map(addr =>
> addr.email).join(", ");
> +    let fromAddr =
> MailServices.headerParser.parseEncodedHeader(fromHdr).map(addr =>
> addr.email).join(", ");
> What's the point of that?
> 
> Why is that function deprecated in the first place? And you copy its code
> around to some callers but not all?

Apparently the code is wrong at some callers and needs to be fixed, that is why the function is deprecated.
Notice extractHeaderAddressMailboxes() contained parseDecodedHeader, now the expanded 2 callers use parseEncodedHeader.

I would agree that if the callers can't be replaced by some simpler code than just the expansion of extractHeaderAddressMailboxes() we could keep the helper and make 2 of them for each variant of parse{En,De}codedHeader. But I am not sure if jcranmer want's such API.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #7)
> Apparently the code is wrong at some callers and needs to be fixed, that is
> why the function is deprecated.
> Notice extractHeaderAddressMailboxes() contained parseDecodedHeader, now the
> expanded 2 callers use parseEncodedHeader.
At 1:24 AM, that slipped my attention :-(

> I would agree that if the callers can't be replaced by some simpler code
> than just the expansion of extractHeaderAddressMailboxes() we could keep the
> helper and make 2 of them for each variant of parse{En,De}codedHeader. But I
> am not sure if jcranmer want's such API.
Well, I suggest we either do no clean-up or full clean-up.

Joshua, how about creating an function for 
.parseEncodedHeader(xx).map(addr => addr.email).join(", "); (note *En*code)
instead of copying that code everywhere. And maintain extractHeaderAddressMailboxes() or rename it, if
.parseDecodedHeader(xx).map(addr => addr.email).join(", "); (note *De*coded)
header is useful in one of the callers.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8899290 [details] [diff] [review]
patch v1.1

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

Since I get to fix JSMime regressions, I'm very wary of changes in this area. So I've given this a long hard look and wanted to check whether the changes work.

In the remote content notifications you supposedly get a different message depending on whether From: and Disposition-Notification-To: match. I haven't seen this, but this is another issue. The patch sadly causes a JS error. Have you tried it?

::: mail/base/content/mailWindowOverlay.js
@@ +3140,5 @@
>      let mdnHdr = aMimeHdr.extractHeader("Disposition-Notification-To", false) ||
>                   aMimeHdr.extractHeader("Return-Receipt-To", false); // not
>      let fromHdr = aMimeHdr.extractHeader("From", false);
>  
> +    let mdnAddr = MailServices.headerParser.parseEncodedHeader(mdnHdr).map(addr => addr.email).join(", ");

JavaScript error: chrome://messenger/content/mailWindowOverlay.js, line 3144: NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsIMsgHeaderParser.parseEncodedHeader]

Comment 10

[Jorg K (CEST = GMT+2)]

Sorry, I misread, nothing to do with remote content, this is in setMDNMsg(). However, the JS error is real :-(

Comment 11

[:aceman]

Yes, I didn't test it after converting from DecodedHeader to EncodedHeader and didn't notice parseEncodedHeader takes more arguments.
Should we pass in null or msgHdr.effectiveCharset which are the variants the existing callers do?

Comment 12

[Jorg K (CEST = GMT+2)]

Caller:

  parseEncodedHeader: function (aHeader, aCharset, aPreserveGroups, count) {
    aHeader = aHeader || "";
    let value = MimeParser.parseHeaderField(aHeader,
      MimeParser.HEADER_ADDRESS | MimeParser.HEADER_OPTION_ALL_I18N, aCharset);
    return fixArray(value, aPreserveGroups, count);
  },

and callee:

  parseHeaderField: function MimeParser_parseHeaderField(text, flags, charset) {
    // If we have a raw string, convert it to Unicode first
    if (flags & MimeParser.HEADER_OPTION_ALLOW_RAW)
      text = jsmime.headerparser.convert8BitHeader(text, charset);

So the charset is looked at since HEADER_OPTION_ALL_I18N includes HEADER_OPTION_ALLOW_RAW:
https://dxr.mozilla.org/comm-central/rev/7b75c2e0b9deb590c18aaddb2a6168146cde8ca4/mailnews/mime/src/mimeParser.jsm#216

I think the switch from DecodeHeader to EncodedHeader is unnecessary since you only want the e-mail address, not the name part. And you don't know which charset to use. That said, usually headers can be UTF-8, so I don't think that passing that would go too far astray. I'm sure Joshua will have a better answer ;-)

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #12)
> And you don't know which charset to use.
Just saw in MsgComposeCommands.js:
let from = MailServices.headerParser.parseEncodedHeader(params.composeFields.from, null).join(", ");
So they are using null.

Comment 14

[Joshua Cranmer [:jcranmer]]

(In reply to Jorg K (GMT+2) from comment #12)
> I think the switch from DecodeHeader to EncodedHeader is unnecessary since
> you only want the e-mail address, not the name part. And you don't know
> which charset to use. That said, usually headers can be UTF-8, so I don't
> think that passing that would go too far astray. I'm sure Joshua will have a
> better answer ;-)

There's a method on nsIMsgHdr that gets the effective charset as I recall. I don't recall what it is exactly, but mime2DecodedAuthor is properly getting the charset.

(Sorry for forgetting that parseEncodedHeader takes a mandatory charset argument. I've been unable to effectively test patches for a bit, so I'm a really bad reviewer at this juncture).

(In reply to Jorg K (GMT+2) from comment #8)
> Joshua, how about creating an function for 
> .parseEncodedHeader(xx).map(addr => addr.email).join(", "); (note *En*code)
> instead of copying that code everywhere. And maintain
> extractHeaderAddressMailboxes() or rename it, if
> .parseDecodedHeader(xx).map(addr => addr.email).join(", "); (note *De*coded)
> header is useful in one of the callers.

Personally, I think the entire interface is completely rotten, but the rot really starts from the fact that we're storing the wrong bloody thing in the database in the first place. Keeping a method for "get all the mailboxes from this header" is okay, especially since mailboxes aren't affected by RFC 2047 decoding. They are affected by charsets, but if we're getting non-ASCII email addresses in non-UTF-8 headers, we have *serious* problems. We do need to be careful if we're handling binary strings or Unicode strings in JS, though.

Comment 15

[:aceman]

Attachment: patch v2.2

So I have reverted the opencoding of *Mailboxes and added a version of the function using parseEncodedHeader. So now we can keep the helper and also can choose whether Encoded or Decoded is appropriate.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903865 [details] [diff] [review]
patch v2.2

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

::: mail/base/content/mailWindowOverlay.js
@@ -2020,5 @@
>  function MsgCreateFilter()
>  {
>    // retrieve Sender direct from selected message's headers
>    var msgHdr = gFolderDisplay.selectedMessage;
> -  let emailAddress = MailServices.headerParser.extractHeaderAddressMailboxes(msgHdr.author);

Why not leave that function? It's not removed elsewhere. If it works on the decoded header, use msgHdr.mime2DecodedAuthor as a parameter, no?

::: mailnews/mime/public/nsIMsgHeaderParser.idl
@@ +201,5 @@
>     * @param aLine          The header line to parse.
>     * @return               A comma-separated list of just the mailbox parts
>     *                       of the email-addresses.
>     */
>    /* @deprecated */ ACString extractHeaderAddressMailboxes(in ACString aLine);

Leave that "deprecated"?

@@ +216,2 @@
>     */
> +  /* @deprecated */ ACString extractEncodedHeaderAddressMailboxes(in ACString aLine, in ACString aCharset);

Add a "deprecated" function???

Comment 17

[:aceman]

(In reply to Jorg K (GMT+2) from comment #16)
> ::: mail/base/content/mailWindowOverlay.js
> @@ -2020,5 @@
> >  function MsgCreateFilter()
> >  {
> >    // retrieve Sender direct from selected message's headers
> >    var msgHdr = gFolderDisplay.selectedMessage;
> > -  let emailAddress = MailServices.headerParser.extractHeaderAddressMailboxes(msgHdr.author);
> 
> Why not leave that function? It's not removed elsewhere. If it works on the
> decoded header, use msgHdr.mime2DecodedAuthor as a parameter, no?

Yes, that would work too. I just thought running extractHeaderAddressMailboxes() is semantically incorrect as it theoretically concatenates multiple values into one string, but then we handle it as a single email address in MsgFilters, etc. Yes, author should rarely have multiple values, it would make no sense. On the other hand, if really there are multiple values, sending them into the filter editor may be useful as we do not know which one the user wants. He can then delete the others. It just may be dangerous to send multiple addresses if all the variables are named emailAddress.
So you can decide:)
 
> ::: mailnews/mime/public/nsIMsgHeaderParser.idl
> >    /* @deprecated */ ACString extractHeaderAddressMailboxes(in ACString aLine);
> Leave that "deprecated"?
> 
> > +  /* @deprecated */ ACString extractEncodedHeaderAddressMailboxes(in ACString aLine, in ACString aCharset);
> Add a "deprecated" function???

Yes. I think this wasn't decided yet how to replace the function. But at least we can keep the helper and fix some bugs (the decoded author changes) in this bug.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8903865 [details] [diff] [review]
patch v2.2

I'm clearing my review queue today :-)

(In reply to :aceman from comment #17)
> So you can decide:)
I'll let Joshua take a look first since it's his stuff. I can see arguments for both extractHeaderAddressMailboxes() and parseDecodedHeader(msgHdr.mime2DecodedAuthor), or even parseEncodedHeader(msgHdr.author), no?

I still think the "deprecated" comment should go and a second one shouldn't be added.

Comment 19

[Joshua Cranmer [:jcranmer]]

(In reply to Jorg K (GMT+2) from comment #18)
> I still think the "deprecated" comment should go and a second one shouldn't
> be added.

I agree with Jorg's sentiments here. extractHeaderAddressMailboxes should do the right thing on a legal header independent of whether an RFC-2047 encoded header or its decoded variant is used (RFC 2047 encoding is not legal in the mailbox portion of the header, and software that tries to apply it there is likely to get caught up by MTAs before delivery). EAI and IDN do make charsets a mild concern, but there's a host of other issues we have to tackle in that regard, and it requires much more diligent tracking all the way back to the database. So there's no need for a second function.

Comment 20

[Jorg K (CEST = GMT+2)]

Oops, misunderstanding here. I tried to say:
I still think the "deprecated" comment should go and a second *such comment* shouldn't be added [to the new function].

But as you said, since we only want to extract the e-mail address ("mailbox") we don't care whether the input is decoded or encoded. That's what I tried to say at the end of comment #12:
I think the switch from DecodeHeader to EncodedHeader is unnecessary since you only want the e-mail address, not the name part.

Comment 21

[:aceman]

Attachment: patch v3

So let's see if I understood it correctly.

Comment 22

[:aceman]

Attachment: patch v3.1

Is it OK now?

Comment 23

[Jorg K (CEST = GMT+2)]

Oh boy, now I need to get back into this after more than a year :-(

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 9027277 [details] [diff] [review]
patch v3.1

I get an overdue review reminder for this every day, hence cycling the request. I'll look at it over the Christmas break.

Comment 25

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9b04b7f9693febcda2d3f2428e332a646cacd583

I'm sure this has had a few try runs, but better safe than sorry. Surprisingly no bit rot :-) - I've promised a review, so I'm at it now.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 9027277 [details] [diff] [review]
patch v3.1

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

Thanks for the clean-up, it looks OK to me. I've commented on the various bits to wrap my head around what is happening. I'll land it at the next convenience assuming a clear try run (which in hindsight looks like a bit of an overkill, but anyway).

::: mail/base/content/mailWindowOverlay.js
@@ +2941,5 @@
>  
>      let mdnAddr = MailServices.headerParser.extractHeaderAddressMailboxes(mdnHdr);
>      let fromAddr = MailServices.headerParser.extractHeaderAddressMailboxes(fromHdr);
>  
> +    let authorName = MailServices.headerParser.extractFirstName(

OK, so we're replacing extractHeaderAddressName with extractFirstName, and we're also changing the implementation of the latter to receive the code of the former.

::: mailnews/mime/src/mimeJSComponents.js
@@ +292,5 @@
>    },
>  
>    extractFirstName: function (aHeader) {
> +    let addresses = this.parseDecodedHeader(aHeader, false);
> +    return (addresses.length > 0) ? (addresses[0].name || addresses[0].email) : "";

This is more or less the code from extractHeaderAddressName().

::: mailnews/mime/test/unit/test_nsIMsgHeaderParser2.js
@@ +68,5 @@
>    for (let i = 0; i < checks.length; ++i) {
>      Assert.equal(MailServices.headerParser.extractHeaderAddressMailboxes(checks[i][0]), checks[i][1]);
> +    let names = MailServices.headerParser.parseDecodedHeader(checks[i][0])
> +                                         .map(addr => addr.name || addr.email)
> +                                         .join(", ");

So this is open-coding the extractHeaderAddressNames() function.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9fe194930e8d
Remove some unused functions in nsIMsgDBView and nsIMsgHeaderParser. r=jcranmer,jorgk