549931 - Irregular message does not update header pane on message view (aka 'undisclosed-recipients' not showing)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Brett Leber]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/532.5 (KHTML, like Gecko) Chrome/4.0.249.89 Safari/532.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100303 Lanikai/3.1b2pre

The attached message does not update the header pane--Thunderbird continues to show the header of the previously viewed message--when clicking the message to view. If you open the folder (Inbox, in my case) with the message preselected, you get NO header pane. This message is not a problem with TB 3.0, but it is a problem with 3.1b2pre. 

Clearing .MSF file, re-indexing, and compacting have no effect.

Reproducible: Always

Steps to Reproduce:
(a little tricky because the message won't show the problem if it's forwarded, only if it's received in its current form)

1. In 3-pane view, click message from message list.

Actual Results:  
Header pane does not update or appear.

Expected Results:  
Header pane should update.

Comment 1

[Brett Leber]

Attachment: The problematic message.

Comment 2

[Ludovic Hirlimann [:Usul]]

Have you tried safe mode? (see https://support.mozillamessaging.com/en-US/kb/Safe+Mode for more information)

Comment 3

[Ludovic Hirlimann [:Usul]]

Anything in Tools -> Error console ?

Comment 4

[Brett Leber]

Good idea. Here is the Error Console message when clicking the problematic mail message:

Error: uncaught exception: [Exception... "Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIMsgHeaderParser.extractHeaderAddressMailboxes]"  nsresult: "0x8007000e (NS_ERROR_OUT_OF_MEMORY)"  location: "JS frame :: chrome://messenger/content/msgHdrViewOverlay.js :: anonymous :: line 529"  data: no]

Will try Safe Mode next.

Comment 5

[Brett Leber]

Same error in Safe Mode. For reference, I'm running Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.2pre) Gecko/20100316 Lanikai/3.1b2pre

(In reply to comment #4)
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x8007000e (NS_ERROR_OUT_OF_MEMORY)
> [nsIMsgHeaderParser.extractHeaderAddressMailboxesextractHeaderAddressMailboxes]"  nsresult: "0x8007000e
> (NS_ERROR_OUT_OF_MEMORY)"  location: "JS frame ::
> chrome://messenger/content/msgHdrViewOverlay.js :: anonymous :: line 529" 
> data: no]

Comment 6

[Mark Banner (:standard8)]

Confirming the regression on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.2.2pre) Gecko/20100221 Lightning/1.0b2pre Lanikai/3.1b1pre.

I get the same exception. As the email looks quite simple, I think this needs investigation.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

I don't think this really qualifies for the either of the 3.1 blocking criteria:

a) make the upgrade experience from TB2 very painful for a large number of users

or 

b) be a new, reproducible, severe quality issue (eg dataloss, frequent crashes)

If we get more information that makes us believe a very significant number of users is experiencing this, please renominate.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Standard8 pointed out to me that we'd hit this for any mail with an empty group-syntax, which would be hit in significant numbers.  If someone was interested in doing a binary search on the nightly builds to figure out when this regression crept in, that would be helpful!

Comment 9

[Mark Banner (:standard8)]

The regression bug is bug 242693, I pretty much know the details of how to fix it and test it, so taking bug.

Comment 10

[Wayne Mery (:wsmwk)]

perhaps the patch you are contemplating will also fix this - xref v3.0 regression Bug 548914 -  malformed To: prevents proper message header display

Comment 13

[Mitra Ardron]

In Bug 559620 I reported a case where this bug was used as part of a phishing attack. 

- Mitra

Comment 14

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

Attachment: The first of my failing test cases

Comment 15

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

Attachment: The second of my failing test cases.

Comment 16

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

Comment on attachment 439581 [details]
The first of my failing test cases

(Uh, I'll just attach the To: lines instead…)

To: <"Undisclosed-Recipient:;"@latte.ca>
To: undisclosed-recipients: ;
To: <undisclosed-recipients:>
To: "Me at [:] Home" <[e-mail]>


And something seems off with the To line of:
To: A Group:Chris Jones <[e-mail]>,[e-mail],John <[e-mail]>;
but I think that might be a bug in dmose's code.

Thanks,
Blake.

Comment 19

[David :Bienvenu]

I'm starting to think this should block b2 instead of rc1 - is the fix hard?

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

bienvenu: why?

Comment 21

[David :Bienvenu]

We've had at least three dupes in the last two days.

Comment 22

[Mark Banner (:standard8)]

Here's the possibilities:

- Back out bug 242693, don't have it in 3.1
- Fix this bug!
- Band-aid for beta 2 and then fix or backout.

At the moment, I'd probably be realistically looking at a day for a full fix to this bug - I've looked at it before and I know something bad is going on, I just can't see what changed to cause it.

Backing out bug 242693 is a possibility, but I'd have liked to look a bit more in-depth at this one.

The band-aid would be to put try/catches around the extractHeaderAddressMailboxes calls in msgHdrViewOverlay.js that prevent the error, and would mean that we don't display *anything* for that header line. As I say, this is a band-aid and would be just for beta 2.

I'm open to thoughts on this.

Comment 23

[Mitra Ardron]

Importance I think is that its being actively exploited for some effective Phishing & Spamming -= which I'm guessing is why we are getting the dupes.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Band-aid for b2 sounds ok to me, assuming you have time.

Comment 25

[Mark Banner (:standard8)]

Note to self:
Before bug 242693 the behaviour of passing 'Undisclosed recipients:;' was as follows:

- nsIMsgHeaderParser.extractHeaderAddressMailboxes returns '\"Undisclosed recipients:;\"'
- nsIMsgHeaderParser.extractHeaderAddressNames returns '\"Undisclosed recipients:;\"'
- nsIMsgHeaderParser.extractHeaderAddressName returns 'Undisclosed recipients:;'

The latter two seem wrong. From the descriptions, I'd expect just 'Undisclosed recipients'.

extractHeaderAddressMailboxes also seems wrong - I think it should just be returning '' - the empty string because there are no mailboxes.

Comment 26

[Mark Banner (:standard8)]

For 3.1 beta 2, we've decided to work around this by putting in a try/catch that will keep things going. The side effect will be that the address will just be displayed as an empty string, but at least we'll be displaying a semi-relevant header.

Hence, we'll still need to fix this before rc1, but for beta 2 the error is not quite so harsh.

Comment 28

[Mark Banner (:standard8)]

Attachment: Test case showing the 3.0 behaviour of splitRecipients

Comment 29

[Mark Banner (:standard8)]

Attachment: Proposed fix

This fix seems to do the right job for 3.1 at least. Our handling of group syntax in nsMsgHeaderParser isn't very good. What I'm doing in this patch is to try and match us reasonably closely to 3.0 behaviour without regressing bug 242693, and end up displaying the right things.

When I commit this, I'm also proposing that we back out the work around that we had for beta 2, i.e. backing out bug 562786 from comm-1.9.2.

A summary of the patch follows:

- Took out the code that skipped empty groups. Although we should probably be skipping empty groups (at least for extractHeaderAddressMailboxes functionality), we'll actually end up not displaying anything if we do.

We should probably be displaying the group name, but I think that's more complex and going to change APIs and things, and hence a subject for a different bug.

- Determine if we're in a group or not and use this to determine if ";" is an address separator or not. This gives rise to some funny syntax, but if you look at the 3.0 testcases (attachment 445956 [details] [diff] [review]) you'll find that 3.0 gave basically the same results.

- Adjusts the testcases that we've got in the tree, and add one in test_nsIMsgHeaderParser2.js that checks for the cause of this bug - "Undisclosed recipients:;".

I think this is the best we can do for 3.1, if we go for it, then I'll file at least one follow-up to look at improving our syntax handling.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

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

r+sr=dmose.  It'll be nice to do better in the future.  :-)

Comment 31

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/7823da3c6bf2
http://hg.mozilla.org/releases/comm-1.9.2/pushloghtml

1.9.2 push also included backing out the patch from bug 562786.