368415 - if you only download message headers, mails which are recognised as junk (spam) lose their "junk" status if you download their content

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Snap]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070118 Minefield/3.0a2pre
Build Identifier: Mozilla Thunderbird 2.0 beta 2 (20070116)

If you have downloaded message headers identified as junk, the junk status is set to "not junk" if you download the entire message.

Reproducible: Always

Steps to Reproduce:
1. Account settings: fetch headers only (and leave messages on server)
2. Download headers

=> Thunderbird identify junk mail

3. Click on a junk mail. You can not see its content because it is not downloaded. Click on the link to download it (pop://yyy:110/?uidl=xxx).

=> The status of the mail is no longer "junk". Remote images can be loaded.


Expected Results:  
The junk status should not change.

Comment 1

[Robin Bankhead]

I have a similar issue with TBird version 1.5.0.10 (20070221) / Linux -- has been happening for all the 1.5.* versions.

I don't have any of my accounts set to "fetch headers only", though.  Spam is always filtered to the specified folder (I have it set to filter into 'Deleted' on each account's respective folderspace).  However, most of the time (at least 70% of mail retrievals, as an estimate, and I think it's getting more frequent) after filtering they lose their 'Junk' status.

It's interesting that this doesn't happen every time.  Also I should note that spam that gets past the filter, when manually marked (and automatically moved), keeps its mark.

Comment 2

[Kent James (:rkent)]

This could be the same problem as bug 424876.

Comment 3

[Wayne Mery (:wsmwk)]

I can reproduce with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090619 Shredder/3.0b3pre

Comment 4

[Kent James (:rkent)]

I don't understand the step "=> Thunderbird identify junk mail" in the STR. Is that the user marking the message as junk? Or the bayes filter running and determining the message to be junk?

Comment 5

[Wayne Mery (:wsmwk)]

in my testing, it was manual "J"

Comment 6

[Kent James (:rkent)]

The filter plugin code does not filter for junk if junk status is already set. So this sounds like one of the class of bugs where the database information is being blown away.

Does the same thing happen to tags?

Anyway, I'll look at it.

Comment 7

[Wayne Mery (:wsmwk)]

(In reply to comment #6)
> The filter plugin code does not filter for junk if junk status is already set.
> So this sounds like one of the class of bugs where the database information is
> being blown away.
> 
> Does the same thing happen to tags?

yea, tag is lost also. (like happened in bug 440366)

Comment 8

[Adrian Zaugg]

The junk status gets also lost, when you mark a bunch of messages manually as junk and you have "move them to the account's junk folder" set, recollect those messages and move them back to the original folder (or any other). It doesn't matter if you reset the training data before.

I think in some cases do get the messages reevaluated and the manually set (or sometimes even unset!) junk state changes gets overriden. A manually set or unset junk states should never be changed automatically.

The story gets probably worse to sove when mentionning the situation where you use serveral Thunderbirds on different computers to the same IMAP account. 

This behavior has been in every Thunderbird so far. Thank you for correcting!

Comment 9

[Snap]

(In reply to comment #4)
> I don't understand the step "=> Thunderbird identify junk mail" in the STR. Is
> that the user marking the message as junk? Or the bayes filter running and
> determining the message to be junk?

Hello Kent, as you may have noticed, I meant "the bayesian filter mark some mails as junk". I did not try to mark mails manually.

Comment 10

[Adrian Zaugg]

I think this is relatet to bug #289011.

Comment 12

[Kent James (:rkent)]

Attachment: The fix, for both the click-on-url case, and the download-from-selection case.

Comment 13

[Kent James (:rkent)]

Comment on attachment 401755 [details] [diff] [review]
The fix, for both the click-on-url case, and the download-from-selection case.

I checked again, and it seems to be fine. This is part of my series of bugs to protect database properties.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 401755 [details] [diff] [review]
The fix, for both the click-on-url case, and the download-from-selection case.

Cancelling approval request until this has review (so I can keep track of the queue more easily).

Either bienvenu can set when he reviews the patch or please re-request.

Please also include a statement of risk when requesting approvals.

Comment 15

[David :Bienvenu]

Comment on attachment 401755 [details] [diff] [review]
The fix, for both the click-on-url case, and the download-from-selection case.

thx for the patch, makes sense.

I think instead of oldMessageUri, I'd prefer origMessageUri

This is also for a partially downloaded message as well, right?

+  /// message uri for header-only message version

Comment 16

[Kent James (:rkent)]

Attachment: use "orig" instead of "old" in naming.

Comment 17

[Kent James (:rkent)]

I tried to understand the risks of this bug. I cannot say it is risk free. It relies on assumptions about pop3 urls, that the "uidl=" string will only be included in urls that refer to a single message. Bienvenu would be a better candidate to discuss risk of that.

I think the risk of new crashes is very low. The main risk, if any, is that I misunderstood the nature of pop3 URLs, and will try to update message headers with the message header from another message. But I really think Bienvenu needs to evaluate this. I mainly did this patch as part of my series to remove all occurrances where the message headers were being blown away as part of normal message processing.

Comment 18

[David :Bienvenu]

afaik, the uidl is only set when we're trying to do a partial to a full download, so I think we're safe there.

+NS_IMETHODIMP nsMsgLocalMailFolder::UpdateNewMsgHdr(nsIMsgDBHdr* aOldHdr, nsIMsgDBHdr* aNewHdr)

could use NS_ENSURE_ARG_POINTERs on aOldHdr and aNewHdr, since it's an idl method.

I believe this is fairly safe.

Comment 19

[David :Bienvenu]

Comment on attachment 406559 [details] [diff] [review]
use "orig" instead of "old" in naming.

a=me

Comment 20

[Kent James (:rkent)]

As I am traveling and away from my development environment at the moment, I won't be able to land this until Wednesday, October 20. AFAIK that is OK, but if there are issues with that then please contact me.

Comment 21

[Kent James (:rkent)]

Attachment: Rev d: add NS_ENSURE_ARG_POINTER tests

This adds the tests requested in comment 18.

Comment 22

[Kent James (:rkent)]

Comment on attachment 407562 [details] [diff] [review]
Rev d: add NS_ENSURE_ARG_POINTER tests

Pushed as http://hg.mozilla.org/comm-central/rev/e747fcd38477