Closed Bug 1414518 Opened 3 years ago Closed 3 years ago

Allow Addons to Access Arbitrary Headers in currentHeaderData

Categories

(MailNews Core :: MIME, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 59.0

People

(Reporter: patrick, Assigned: patrick)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch emit-all-headers.patch v1 (obsolete) — Splinter Review
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.
Attachment #8925247 - Flags: review?(jorgk)
Sorry about the delay. Still recovering from the nine weekend bustages. I'll get to it.
Comment on attachment 8925247 [details] [diff] [review]
emit-all-headers.patch v1

Sorry, please find another reviewer.
Attachment #8925247 - Flags: review?(jorgk)
Attachment #8925247 - Flags: review?(rkent)
Comment on attachment 8925247 [details] [diff] [review]
emit-all-headers.patch v1

I'm back ;-)
Attachment #8925247 - Flags: review?(rkent) → review?(jorgk)
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.
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
Can you please grant me access to bug 1412633.
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?
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?
"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...
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 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.
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?
Flags: needinfo?(patrick)
Flags: needinfo?(alta88)
(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.
Flags: needinfo?(patrick)
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.
Flags: needinfo?(alta88)
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.
Attachment #8925247 - Flags: review?(jorgk)
Yes, I'll prepare a patch.
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.
Attachment #8925247 - Attachment is obsolete: true
Attachment #8936872 - Flags: review?(jorgk)
Summary: Don't optimize emitting of headers to UI → Allow Addons to Access Arbitrary Headers in currentHeaderData
Attachment #8936872 - Attachment mime type: text/x-patch → text/patch
Attachment #8936872 - Attachment is patch: true
Attachment #8936872 - Attachment mime type: text/patch → text/plain
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?
Attachment #8936872 - Flags: review?(jorgk) → review+
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);
Oops, I missed |ToLowerCase(extraExpandedHeaders);| and |ToLowerCase(extraAddonHeaders);|. All clear now. I'll take care of it tonight.
Flags: needinfo?(jorgk)
I reshuffled this a bit ;-)
Attachment #8936872 - Attachment is obsolete: true
Flags: needinfo?(jorgk)
Attachment #8937039 - Flags: feedback?(patrick)
I've just spotted the trailing spaces, I'll remove them before landing.
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 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).
Attachment #8937039 - Flags: feedback?(patrick) → feedback+
Fixed white-space, removed 'else' and improved comment. This is ready to go.
Attachment #8937039 - Attachment is obsolete: true
Attachment #8937116 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/68bfb968e75e
introduce pref mailnews.headers.extraAddonHeaders to emit extra headers to currentHeaderData. r=jorgk
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 59.0
You need to log in before you can comment on or make changes to this bug.