651002 - single comma only in to or cc field displays cached content, exception in mailWidgets.xml

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[ned]

User-Agent:       Mozilla/5.0 (Windows NT 6.0; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.15) Gecko/20110303 Thunderbird/3.1.9

This might be a little difficult to reproduce, but bear with me.  We have a piece of software that creates automatic email messages.  One piece of bad behavior from this software is that it apparently inserts a plain comma, all by itself, in the CC field.  (This is a bit of sloppiness in the handling of multiple CC recipients that is being addressed in that software).

Nonetheless, it exposed a nasty bug in Thunderbird that does not manifest in other email clients.  When you are reading a mail message (particularly in the three-pane window) that has a CC field with just a single comma in it, you get the following behavior:

1) In the bottom view pane, Thunderbird always "fills in" the recipients that were CC'ed on the last message you read before this one.  They're not really there (as one can confirm from Viewing Message Source), but it's jarring nonetheless.

2) There's often the same effect with the Subject line - in the bottom view pane, it replaces the legitimate Subject line of the email you're viewing with the Subject line of your last viewed message.

Looking at the View Message Source, I see that the Subject line is right after the CC line.  So you might have something like this in the source:

CC: ,
SUBJECT: Real subject line here

... but it displays in the bottom pane as:

from (correct sender)
subject (wrong subject from previous message)
to (you)
cc (people from previous message)


Reproducible: Always

Steps to Reproduce:
Not sure how to create what is clearly a malformed email (with CC: ,) ... but if you can, you'll see what I'm talking about.  If you need me to forward you such a message, give me an email to shoot it to.

Because of course, when I try to make the well-behaved Thunderbird message creation window do this, it won't.  Good Thunderbird.

Comment 1

[rsx11m]

Yes, that's easy to reproduce and indeed causes unexpected behavior. I'm seeing tags from previously viewed messages appearing as well in a test message only containing just a single "," in the "To:" header without any recipient. Though it may or may not be permissible by the standard this case should be caught.

Error console reads for Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0a2) Gecko/20110420 Thunderbird/3.3a4pre, Error: uncaught exception: [Exception... "Index or size is negative or greater than the allowed amount"  code: "1" nsresult: "0x80530001 (NS_ERROR_DOM_INDEX_SIZE_ERR)"  location: "chrome://messenger/content/mailWidgets.xml Line: 412"].

Comment 2

[rsx11m]

The code where it throws the exception was introduced in bug 563612:

> // Hide any extra nodes but keep them around for later.
> for (let j = i*2 - 1; j < aAddressesNode.childNodes.length; j++) {
>   aAddressesNode.childNodes[j].hidden = true;
> }

While adding "if (i > 0)" in front of it resolves the exception, the leaking of prior information still occurs, so that alone won't fix the issue.

In general it appears that we expect pairs of "<address><comma>" thus any empty "<address>" may cause an issue. In this case, with just a single "<comma>" but no address, i=0 and some cached content is displayed.

Comment 3

[rsx11m]

Attachment: Proposed fix

This fixes it for me, I forgot about the fact that previously filled address nodes still have to be hidden even for the i=0 case. Everything else seems to work fine when switching messages within and across folders now.

I've also tried a case like ", first, , second" which is correctly displayed as "first(x), second()" in the list, thus empty <address> fields are caught, just not the case where there is no address at all.

Comment 4

[rsx11m]

If I read http://tools.ietf.org/html/rfc5322#section-3.4 correctly, addr-spec can't be an empty string, so that header is flawed and this comes down to a stability fix to avoid the exception (i.e., no updates of any tests).

Comment 5

[rsx11m]

Attachment: Minimal test case

Comment 6

[Jim Porter (:squib)]

(In reply to comment #3)
> Created attachment 527314 [details] [diff] [review]
> Proposed fix

I'd go with this instead, since I think it's a little clearer:

  for (let j = Math.max(i*2 - 1, 0); j < aAddressesNode.childNodes.length; j++) {

Comment 7

[rsx11m]

> Math.max(i*2 - 1, 0)

Wouldn't this imply floating-point rather than integer arithmetic, or am I thinking too much of C-style data types here?

Comment 8

[rsx11m]

It may be a bit easier to read, but it appears to me that the inline arithmetic should be more efficient than going through the global Math object and ending up in js_math_max() making this a double-precision calculation...

I'll leave the current patch up for review and let Blake decide which version he prefers, it would be a quick change.

Comment 9

[Jim Porter (:squib)]

I doubt performance would be measurably affected by that, since I expect the majority of CPU time spent in that loop is in the DOM.

Comment 10

[rsx11m]

Attachment: Alternative patch

To make this easier for Blake, here is the variant based on Jim's comment #6. Functionally it's completely equivalent to attachment 527314 [details] [diff] [review].

It still feels a bit odd to me to waste processor cycles and stack frames for
a simple boundary check, but I agree with comment #9 that this shouldn't come with any measurable performance penalty (called once per header displayed).

Please plus one and minus the other, depending on which version you prefer.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 528324 [details] [diff] [review]
Alternative patch

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

Yeah, I think having the "?:" inside the "for(;;)" is just a little too hard to read.  ("for(?:;;)" almost looks like a Cthulhu-smiley…)

If we're really worried about performance regressions, I suggest we cache the value of aAddressesNode.childNodes.length, cause that's way more likely to be a problem than the Math.max call.  ;)

r=me (with or without caching the length).

Thanks,
Blake.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 527314 [details] [diff] [review]
Proposed fix

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

As mentioned in the previous review, I prefer the other form of this patch.

Comment 13

[rsx11m]

Thanks Blake. Yes, I see that "for(?:;;)" looks a bit funny...

I'll leave it simple, caching aAddressesNode.childNodes.length and other frequently used items may be more of a global problem beyond this specific
issue and could be addressed in a separate perf bug for the header UI.

Push for comm-central please.

Comment 14

[rsx11m]

Attachment: Final patch

Actually, since a variable "cached" has been defined exactly for the purpose of keeping track of the number of cached address nodes further up we can equally use it here as well.

Please push this version instead.

Comment 15

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/1b77f9ef4186

Comment 16

[rsx11m]

Comment on attachment 528628 [details] [diff] [review]
Final patch

Thanks for the check-in. This looks good in the tinderbox builds and tests,
thus requesting branch approval (assuming that throwing an uncaught exception qualifies as a stability fix). The patch applies fine to comm-1.9.2.

Comment 17

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/releases/comm-1.9.2/rev/9d16bc1a378e