655578 - Some emails aren't filtered on list-id anymore

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect)

Description

[Ludovic Hirlimann [:Usul]]

Attachment: Email failing to be filtered.

I have a simple filter that does the following :
name="GnuPG"
enabled="yes"
type="17"
action="Move to folder"
actionValue="imap://[e-mail]/Gnu-PG ml"
condition="AND (\"list-id\",contains,gnupg-users.gnupg.org)"

As seen in msgFilterRules.dat , this used to work perfectly - and seem to still be working with some messages but not others. I think It's been broken since a few days.

The filter log is of course empty - I didn't check the error console

Comment 1

[Ludovic Hirlimann [:Usul]]

Attachment: Email being properly filtered

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

I'm seeing this too.  Certain messages to mailing lists using "list-id" filters are not getting filtered on delivery.

The more annoying part is that you can't just re-run the filters on the custom header to get them where you're going without some legwork (bug 184490).

However, once you add a:
  user_pref("mailnews.customDBHeaders", "List-Id");
then the mailbox parser will stick the header in the .msf file and then run filters works for newly received messages or if you "repair folder".

In bug 184490 comment 55 bienvenu suggested he was going to look into syncing those two properties, but it then seems like maybe offline magic was supposed to moot that need?

Note that the "mailnews.customDBHeaders" preference is space delimited, whereas "mailnews.customHeaders" is technically colon-delimited (but strips all whitespace during meaningful processing) although the JS logic uses a join with ": ".

I've enabled the filter log locally in the hopes that it might shed some light and will keep an eye on it the next time this happens.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

This seems to have stopped happening now that I've set "mailnews.customDBHeaders" to include List-Id.

I therefore put forth the theory that there is somehow a (less probable) case where the message filters are run in a mode where they do not have access to the List-Id header unless it is in the msf.

One additional factor seems to be that the mailing lists this happened for were all in my address book.  Specifically, "[e-mail]" and the others are in my "collected addresses" address book because I have written them messages and I guess I have the auto-add setting turned on.  (The address generally shows up in the "To" and "X-BeenThere" headers, but I am not sure if this varies at all in the messages that did not work for me.)

Comment 4

[David :Bienvenu]

as you say, the mailnews.customDBHeaders stuff means that we set a property on the msg hdr when parsing the headers, and the filter application stuff checks that property. But there is also code to grovel through the headers looking for the arbitrary header that executes if the property isn't set:

http://mxr.mozilla.org/comm-central/source/mailnews/base/search/src/nsMsgSearchTerm.cpp#784

If setting customDBHeaders fixes the issue, there are two possibilities:

1. The imap protocol code that determines which headers to ask for is sometimes not asking for List-ID, because nsMsgFilterList::ComputeArbitraryHeaders() is failing for some reason. Since that method caches its result, the failures should be consistent for any given run of Thunderbird. I haven't heard that's the case, so I doubt that's the issue.

2. The code that grovels through the header soup is broken, perhaps because of an uninitialized variable or something. rkent did change this code recently to deal with multi-line header values so there does seem to be a bit of smoke :-)...there are extensive xpcshell tests for it, though. If it is an uninitialized variable, debug builds may not ever see the issue because memory state is more predictable in debug builds.

Comment 5

[David :Bienvenu]

rkents change was checked in 5/03, http://hg.mozilla.org/comm-central/rev/09e5bddbead8 so the regression range is reasonably close. I'll poke through that code.

Comment 6

[Kent James (:rkent)]

Hard to see how bug 540449 could cause this, as that affects junk status searches. Now bug 363238 is related, but that has been reviewed but not checked in. I'm a little reluctant to check that in while the issue of this current bug is outstanding, as that could muddy the issue.

Comment 7

[David :Bienvenu]

ah, right, http://hg.mozilla.org/comm-central/rev/7b75008cb771, bug 124641 is much more likely to be involved. That was landed 4/29

Comment 8

[David :Bienvenu]

GetNextBodyLine goes completely off the rails on the problem message; I suspect it's one of these headers:

X-Rogue: :MFPA:
X-Face: 9V>2Thjx~2}N]Q=UoKRvzP1LKjJro#0OB, re&LYjt#F+]&mm<4:uFd,
	dYCu$YT\XG]VlX`->+,
	zq{Nm1@NXc+^HjuhT{8)<-XCuA!8O.{CLf`?BbR3QzpXpi<eoGNmkhAOB'u

Comment 9

[David :Bienvenu]

backing out http://hg.mozilla.org/comm-central/rev/7b75008cb771 does indeed fix the regression, but I suspect the actual problem might be deeper - it seems like some headers aren't making it from the imap parser over to the backend, after we see that x-rogue header.

Comment 10

[David :Bienvenu]

It can't be the x-rogue or x-face headers since we don't request those headers. It has to be something to do with the multiple references lines before the list-id header.

Comment 11

[David :Bienvenu]

In particular, I believe it was the nsParseMailbox.cpp changes in http://hg.mozilla.org/comm-central/rev/7b75008cb771 that broke this. I was fixated on the nsMsgSearchTerm changes since it's the search term evaluation that's failing, but the nsParseMailbox changes seem to be the one that's slightly corrupting the headers.

Comment 12

[David :Bienvenu]

I have a fix for this which I believe will pass the unit tests. I'm not super happy about the fix, but the nsParseMailbox code is depressing. I need to come up with an xpcshell test that shows this bug as well.

Essentially, the header parser code works by taking one big buffer which contains multiple null terminated strings, and parses the individual headers. Along the way, it modifies the buffer in place to do things like whitespace folding. When it modifies the buffer, e.g., removing whitespace, it doesn't byte shift the rest of the buffer to reclaim the folded space. This was the case before the regressing patch, but now we do more buffer modifications, and we're not null terminating the left-over blocks. This basically affects filtering on custom headers, because all other filtering uses headers in the db, which are not affected. The big buffer is used after the header parsing to apply the filter.

I tried doing the shifting, but I didn't get it right. I'd like to keep figuring that out, but in the meantime, I've added an extra null terminating byte.

Comment 13

[David :Bienvenu]

Attachment: proposed fix

this fixes the test case I have, and passes the unit tests. Basically, it restores the null termination of the slop left over after folding whitespace, which is sufficient for fixing the bug.

I'd really prefer that we left-shift the whole buffer (or rewrite this whole mess), but I haven't been able to get that working, whereas this seems to work fine.

I'm working on a test, but it's going to take a while, since I need a new test (there don't seem to be any relevant existing tests).

Comment 14

[David :Bienvenu]

Attachment: wip on unit test

this test fails w/o the patch. Now to check that it passes with the patch.

Comment 15

[David :Bienvenu]

Attachment: cumulative patch w/ unit test

this fixes the unit test (the filter terms weren't quite right before) so that now it fails w/o the core changes but succeeds with them.

Comment 16

[Kent James (:rkent)]

Comment on attachment 538602 [details] [diff] [review]
cumulative patch w/ unit test

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

Patch makes sense. I also confirmed that the test fails without the patch, and succeeds with.

Only issue is the \ No newline at end of file in the patch

Comment 17

[David :Bienvenu]

fixed on trunk and miramar - pulling an aurora tree now.

http://hg.mozilla.org/comm-central/rev/e7765eb76068

http://hg.mozilla.org/releases/comm-miramar/rev/53c7db54f13c

Comment 18

[Ludovic Hirlimann [:Usul]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:5.0) Gecko/20110619 Thunderbird/5.0b2pre

Comment 19

[Mark Banner (:standard8)]

Checked into aurora: http://hg.mozilla.org/releases/comm-aurora/rev/b26c7969544f