506397 - Support multiple spam/junk corpus files

Status: RESOLVED FIXED

(MailNews Core :: Filters, enhancement)

Description

[Kent James (:rkent)]

As I thought about how to improve spam processing in TB, it occurred to me that the sharing of the spam database, such as requested in Bug 181471, could best be accomplished if the backend supported multiple spam corpus files. That is, the spam corpus used by the bayesian filter would be a combination of the local user's training file, and some sort of static file that is distributed either on a local LAN, or using an internet service.

This is part of what is needed for bug 181471, hence marking the blocking. The rest of that bug could then be implemented in an extension.

Comment 1

[Kent James (:rkent)]

Attachment: WIP but no known issues

Here's a basically working patch.

Comment 2

[Kent James (:rkent)]

Attachment: minor tweaks

Let's get this reviewed.

Comment 3

[David :Bienvenu]

Comment on attachment 392562 [details] [diff] [review]
minor tweaks

probably worth changing the member name to something like mJunkPlugin:
-    nsCOMPtr<nsISupports> mSupports;
+    nsCOMPtr<nsIJunkMailPlugin> mSupports;

should you do more to handle this error, e.g., continue to go back to the start of the loop?

+        rv = traitService->GetAliases(antiTrait, &antiAliasesLength, &antiAliases);
+        if (NS_FAILED(rv))
+          NS_ERROR("trait service failed to get aliases");
+      }

Comment 4

[Kent James (:rkent)]

(In reply to comment #3)
> (From update of attachment 392562 [details] [diff] [review])
> probably worth changing the member name to something like mJunkPlugin:
> -    nsCOMPtr<nsISupports> mSupports;
> +    nsCOMPtr<nsIJunkMailPlugin> mSupports;
>
I left this name because, as I understood the code, the only use of this variable was to generate a reference, which just uses the nsISupports functionality. I had to specify though one of the interfaces because the compiler complained about the ambiguity. I can change it if you want though.

> should you do more to handle this error, e.g., continue to go back to the start
> of the loop?
> 
> +        rv = traitService->GetAliases(antiTrait, &antiAliasesLength,
> &antiAliases);
> +        if (NS_FAILED(rv))
> +          NS_ERROR("trait service failed to get aliases");
> +      }

My rational in not handling it is this. Aliases are likely to be optional additions to the accuracy of classification (think of an external junk corpus that is used to enhance the accuracy of the local training) that are added by extensions (which could be unreliable), so it was best to continue mail processing even without the alias. So I thought about the error handling, and this is what I thought was best. I'm not real happy with the error reporting using NS_ERROR, but that is a larger problem in Mozilla mailnews that has no simple answers here. So I'm not convinced that anything else makes sense, but you can try to convince me.

Comment 5

[Kent James (:rkent)]

Is there any hope of getting this reviewed soon?

Comment 6

[David :Bienvenu]

I meant to say that NS_ERROR does nothing in release builds, so there's no handling that case; wouldn't adding a "continue;" at least skip the garbage data?

Comment 7

[David :Bienvenu]

I think calling it mSupports would make people think it's an nsISupports, and it's not, so it would just make the code clearer, I think.

Comment 8

[Kent James (:rkent)]

Attachment: Fixed nits, carrying over r/sr

Patch to checkin.

Comment 9

[Kent James (:rkent)]

Comment on attachment 394343 [details] [diff] [review]
Fixed nits, carrying over r/sr

Oops, wrong bug.

Comment 10

[Kent James (:rkent)]

Comment on attachment 392562 [details] [diff] [review]
minor tweaks

Sorry about the bugspam.

Comment 11

[Kent James (:rkent)]

(In reply to comment #6)
> I meant to say that NS_ERROR does nothing in release builds, so there's no
> handling that case; wouldn't adding a "continue;" at least skip the garbage
> data?

I wish there was a simple way to report this error. I think what I should do is to add an entry to the log at least, since we have one.

As to the continue, that would skip the current trait. But that trait is likely to be JUNK, and skipping it would completely skip JUNK processing. Instead, what I did was to make sure that the alias variables were initialized to values that skip the bad data:

  PRUint32 proAliasesLength = 0;
  PRUint32* proAliases = nsnull;

Isn't that what you were trying to accomplish with your continue?

Comment 12

[David :Bienvenu]

(In reply to comment #11)

> 
> As to the continue, that would skip the current trait. But that trait is likely
> to be JUNK, and skipping it would completely skip JUNK processing. Instead,
> what I did was to make sure that the alias variables were initialized to values
> that skip the bad data.

ah, ok, as long as it's ok to have null elements and 0 lengths in pro/Anti Aliases and pro/AntiAliasesLengths, then that looks OK.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 392562 [details] [diff] [review]
minor tweaks


>+   * @param aTraitIndex:  the internal identifier for the aliased trait
>+   * @param aTraitAlias:  the internal identifier for the alias to add

nit: we don't normally use : after parameter names (several places in the one file).

>-    nsCOMPtr<nsISupports> mSupports;
>+    nsCOMPtr<nsIJunkMailPlugin> mSupports;

I think as David suggested it would be better to change the name of this variable.

Comment 14

[Kent James (:rkent)]

Attachment: Fixed Standard8's and Bienvenu's nits

Carrying over review.

Comment 15

[David :Bienvenu]

Comment on attachment 395327 [details] [diff] [review]
Fixed Standard8's and Bienvenu's nits

This hasn't changed from the original code, from what I can tell, but why is it OK not to close the Stream/file here?

+  FILE* stream;
+  nsresult rv = aFile->OpenANSIFileDesc("rb", &stream);

What led me to ask is that instead of doing if (error = ...)
  break;

you could return NS_ERROR_FAILURE directly. But if it turns out you want to close the file before returning, then you can't...

Comment 16

[Kent James (:rkent)]

(In reply to comment #15)
>
>  why is it OK not to close the Stream/file here?
>

You're right, the closing of traits.dat needs to be fixed. I'll be happy to do that. Thanks for catching that!

Comment 17

[Kent James (:rkent)]

Attachment: closing the stream now

Carrying over r=Standard8

Comment 18

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/5de0a596f65c