1414518 - Allow Addons to Access Arbitrary Headers in currentHeaderData

Status: RESOLVED FIXED

(MailNews Core :: MIME, enhancement)

Description

[Patrick Brunschwig]

Attachment: emit-all-headers.patch v1

nsMimeHtmlEmitter.cpp contains an optimization that avoids processing of "unneccessary" headers in the UI. This optimization has a number of drawbacks and is most likely no longer necessary:

1. The optimization does 21 string comparisons for each string (which is an unnecessary additional task)
2. The optimization makes assumptions about the UI that are not necessarily correct.
3. The fact that headers are not available in the UI leads to extensions needing to parse the message again (like in the case of Enigmail).

Removing the optimization leads to all headers being accessible in the UI. I could not find any further impact on the UI, except for using a little bit more memory. I therefore suggest to remove this code entirely as in the attached patch.

Comment 1

[Jorg K (CEST = GMT+2)]

Sorry about the delay. Still recovering from the nine weekend bustages. I'll get to it.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8925247 [details] [diff] [review]
emit-all-headers.patch v1

Sorry, please find another reviewer.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8925247 [details] [diff] [review]
emit-all-headers.patch v1

I'm back ;-)

Comment 4

[Jorg K (CEST = GMT+2)]

First of all, I apologise for the bumpy ride this bug as had so far.

Can you please give me some more background:
What does "in the UI" mean in these phrases:
- ... processing of "unneccessary" headers in the UI.
- The fact that headers are not available in the UI ...
- ... leads to all headers being accessible in the UI.

Which headers does Enigmail need "in the UI"? Can you point me to the code in Enigmail where the reparsing happens?
Note: I've looked through a lot of Enigmail code for bug 1389789, so I'd appreciate if you could give me a DRX-like reference.

Comment 5

[Patrick Brunschwig]

The parsing happens here:
https://sourceforge.net/p/enigmail/source/ci/master/tree/ui/content/enigmailMessengerOverlay.js#l584

If non-standard headers like "autocrypt", "openpgp", but also other headers were always available in currentHeaderData, then parsing the complete message could be avoided to access the top-level message headers (though admittedly since bug 1412633 having the message structure available in JS is required anyhow). 

I'm currently hacking around this in an ugly way by setting mailnews.headers.extraExpandedHeaders="openpgp, autocrypt" in msgRead.jsm and then deleting it before the message is displayed:
https://sourceforge.net/p/enigmail/source/ci/master/tree/ui/content/enigmailMessengerOverlay.js#l202

Comment 6

[Jorg K (CEST = GMT+2)]

Can you please grant me access to bug 1412633.

Comment 7

[Jorg K (CEST = GMT+2)]

Looks like this is one of those annoying reviews where the submitter needs to educate the reviewer first :-(

You didn't answer most of the questions from comment #4: What does "in the UI" mean and which headers does Enigmail need, apparently openpgp and autocrypt. And what's the deal with currentHeaderData? How is the code you're modifying related to that data? That header data seems to be manipulated in msgHdrViewOverlay.js a lot.

Does "in the UI" mean that the UI only depends the headers that are let through, like they are either displayed directly or drive buttons? I wonder what would be driven by "x-mailer". How may more headers would be let through it we removed the code as you suggested? And finally: You're not in VIEW_ALL_HEADERS mode?

Comment 8

[Jorg K (CEST = GMT+2)]

I've done some more reading in msgHdrViewOverlay.js. processHeaders() gets called from that C++ code and prepares currentHeaderData. Everything passed in, unless it receives special treatment, gets put into the array with |currentHeaderData[lowerCaseHeaderName] = header;|.

That huge C++ if statement sure is ugly, but I'm also not a great friend of pumping all headers into JS. What's the background of VIEW_ALL_HEADERS vs. !VIEW_ALL_HEADERS?

Comment 9

[Patrick Brunschwig]

"in the UI" is for me the JS code is accessible in the composer window, i.e. any code called from overlays to messenger.xul.

VIEW_ALL_HEADERS vs. !VIEW_ALL_HEADERS relates to the pref "mail.show_headers", where:

2 = display _all_ headers of the message to the user in the message reader window
1 = only display the message headers that we are displaying by default (like From, Subject, To, Cc).

I think there is actually less overhead if we just fill currentHeaderData with all headers than to do 20 string-comparisons per header that we process...

Comment 10

[alta88]

I've run into this as well with my extension and worked around it by setting the pref to 2 (and overriding the UI to display whatever headers the user configures). But this isn't viable for any other use since almost no one wants to see all headers, and the UI keys off the pref. Some MTAs may have large numbers of headers, but since this only applies to a single message load (I think multimessage has its own display mechanism), there really isn't any concern about it being a memory issue. And the hacks to get an unfavored header are ugly (like above, or using MsgHdrToMimeMessage() to read/parse again).

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8925247 [details] [diff] [review]
emit-all-headers.patch v1

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

I had to do the changes manually since the patch didn't apply. Let's see whether it breaks anything:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=964b620ef8f9fda24e1c792b326ae6e631906b75

::: mailnews/mime/emitters/nsMimeHtmlEmitter.cpp
@@ -252,5 @@
>  {
>    // if we aren't broadcasting headers OR printing...just do whatever
>    // our base class does...
>    if (mFormat == nsMimeOutput::nsMimeMessagePrintOutput)
> -

There is something wrong with your patch here, it doesn't apply.

Comment 12

[Jorg K (CEST = GMT+2)]

There were no test failures, yet I'm not a great friend of pumping all headers which aren't needed.

How about adding a pref mailnews.headers.extraAddonHeaders to which your add-on can append once at initialisation. We'd then push those headers.

Opinions?

Comment 13

[Patrick Brunschwig]

(In reply to Jorg K (GMT+1) from comment #12)
> There were no test failures, yet I'm not a great friend of pumping all
> headers which aren't needed.
> 
> How about adding a pref mailnews.headers.extraAddonHeaders to which your
> add-on can append once at initialisation. We'd then push those headers.
> 
> Opinions?

That would be a valid, workable alternative.

Comment 14

[alta88]

I don't think it's a big deal, but on the other hand the principle of 'only order what you will eat' is a good one. As long as there is a wildcard option, ie a * value for the pref means stream all headers (without affecting the UI display choice), then ok.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8925247 [details] [diff] [review]
emit-all-headers.patch v1

Patrick, could you prepare such a patch. Mostly we duplicate the code for extraExpandedHeaders. I don't know why an add-on would ever wish to see all headers, but we could special-case "*" to mean that.

Comment 16

[Patrick Brunschwig]

Yes, I'll prepare a patch.

Comment 17

[Patrick Brunschwig]

Attachment: Patch v2, added new pref mailnews.headers.extraAddonHeaders

As promised, attached a new patch that adds a new pref "mailnews.headers.extraAddonHeaders", and pushes the contained headers into currentHeaderData. Specifying "*" will push all headers.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8936872 [details] [diff] [review]
Patch v2, added new pref mailnews.headers.extraAddonHeaders

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

Thanks. I can fix the nits and the Contains(), unless you want to do it yourself. Sorry, digging in old ugly code isn't much fun.

r+ with the nits addressed, or I'll do it and f? you.

::: mailnews/mailnews.js
@@ +50,5 @@
>  // space-delimited list of extra headers to show in msg header display area.
>  pref("mailnews.headers.extraExpandedHeaders", "");
>  
> +// space-delimited list of extra headers that will be pushed to currentHeaderData for processing
> +// in addons (without being displayed). Use "*" to get all headers.

Maybe add: ("*" is the only wildcard supported), so people don't think a* b* will match ax and by.

Also add:
Entries in the list need to be lowercase (right?).

::: mailnews/mime/emitters/nsMimeHtmlEmitter.cpp
@@ +176,5 @@
>    nsCOMPtr<nsIPrefBranch> pPrefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
>    if (pPrefBranch)
>    {
>      pPrefBranch->GetCharPref("mailnews.headers.extraExpandedHeaders", extraExpandedHeaders);
>      // todo - should make this upper case

That comment should go. Or really make it lowercase since below we compare lower case.

@@ +185,5 @@
>      }
> +
> +
> +    pPrefBranch->GetCharPref("mailnews.headers.extraAddonHeaders", extraAddonHeaders);
> +    // todo - should make this upper case

Does this copied comment make sense? It's funny since below we compare against lower case.

@@ +233,3 @@
>          skip = false;
>        } else if (extraExpandedHeadersArray.Length() > 0) {
>          // Make headerStr lower case because IndexOf is case-sensitive.

Hmm, no IndexOf in the code.

@@ +240,5 @@
>            skip = false;
> +      } else if (extraAddonHeadersArray.Length() > 0) {
> +
> +        // push all headers if extraAddonHeaders contains "*"
> +        if (extraAddonHeaders.Contains('*')) {

This should read: .EqualsLiteral("*"), no?
.Contains will find a * everywhere, but the agreement was to do a very basic wildcard of * to mean "all headers".

If you agree, I can change that at checkin. Let me know.

@@ +244,5 @@
> +        if (extraAddonHeaders.Contains('*')) {
> +          skip = false;
> +        }
> +        else {
> +          // Make headerStr lower case because IndexOf is case-sensitive.

Again, IndexOf is not in the code, so change this to Contains or just say "string comparison". Or lose the comment. Also, I'm confused about the case here. The pref needs to be lowercase or this won't work, right?

Comment 19

[Patrick Brunschwig]

I'd appreciate if you could take care of the nits, I don't have time today to work on this.

The pref is case-insensitive. That's why we have: 
  ToLowerCase(extraAddonHeaders);

Comment 20

[Jorg K (CEST = GMT+2)]

Oops, I missed |ToLowerCase(extraExpandedHeaders);| and |ToLowerCase(extraAddonHeaders);|. All clear now. I'll take care of it tonight.

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1414518-emit-addon-headers.patch (v3)

I reshuffled this a bit ;-)

Comment 22

[Jorg K (CEST = GMT+2)]

I've just spotted the trailing spaces, I'll remove them before landing.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8937039 [details] [diff] [review]
1414518-emit-addon-headers.patch (v3)

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

::: mailnews/mime/emitters/nsMimeHtmlEmitter.cpp
@@ +248,5 @@
>          ToLowerCase(headerStr);
>          // Accept if it's an "extra" header.
> +        if (checkExtraHeaders && extraExpandedHeadersArray.Contains(headerStr))
> +          skip = false;
> +        else if (checkAddonHeaders && extraAddonHeadersArray.Contains(headerStr))

This should just be 'if'.

Comment 24

[Patrick Brunschwig]

Comment on attachment 8937039 [details] [diff] [review]
1414518-emit-addon-headers.patch (v3)

Looks good to me. 

We should mention in the comment in mailnews.js that "*" only works, if it's the only 'header' in the pref (that was different in my patch, but I'm perfectly fine with this solution).

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1414518-emit-addon-headers.patch (v3b)

Fixed white-space, removed 'else' and improved comment. This is ready to go.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/68bfb968e75e
introduce pref mailnews.headers.extraAddonHeaders to emit extra headers to currentHeaderData. r=jorgk