336615 - shouldn't use magic numbers for junk status

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Eyal Rozenberg]

in nsMsgDBView.cpp we have:

1276   if (!junkScoreStr.IsEmpty())
1277   {
1278     // I set the cut off at 50. this may change
1279     // it works for our bayesian plugin, as "0" is good, and "100" is junk
1280     // but it might need tweaking for other plugins
1281     properties->AppendElement(atoi(junkScoreStr.get()) > 50 ? kJunkMsgAtom : kNotJunkMsgAtom);
1282   }

and

2802     rv = SetStringPropertyByIndex(
2803         aIndex, "junkscore", 
2804         aNewClassification == nsIJunkMailPlugin::JUNK ? "100" : "0");

which is just wrong. Junk-related constants should be properly and centrally defined, not be magic numbers within .cpp files; and such constants should also be used in mailCommands.js (see bug 324953), and perhaps elsewhere.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Something like this? There may be more places the numbers are used esp. in javascript, but these are the most obvious ones.

Comment 2

[[e-mail]]

Comment on attachment 303090 [details] [diff] [review]
proposed fix

I asked around and found out about bug 366491 which also defines some constants but I don't think it covers that aspect as comprehensively as this patch does.

>-        *result = junkScoreStr.IsEmpty() ? (0) : atoi(junkScoreStr.get()) + 1;
>+        *result = junkScoreStr.IsEmpty() ? nsIJunkMailPlugin::JUNK_LOWSCORE : atoi(junkScoreStr.get()) + 1;
This one really is supposed to be 0. sr=me on the other changes.

Comment 3

[Eyal Rozenberg]

Well, it's better than the current state of affairs AFAICT... although a comment or two explaining that that the minimum is 0, etc., wouldn't hurt either.

Comment 4

[Kent James (:rkent)]

The use of the cutoff of 50 is meaningless, as the de facto definition of junkscore is just two values, 0 and 100. I hate to see you add that constant, implying that it has some real meaning.

Comment 5

[Magnus Melin [:mkmelin]]

Kent: what would you suggest instead? If say another spam plugin use a scale from 0 to 5, or something, we can't blindly use the magic value of 50. I suppose we could always use (max-min)/2, but even then it would seem easier to just define it as a constant. 

Comment 6

[Kent James (:rkent)]

Throughout the code, it is assumed that junkscore == 0 means not junk, and ==100 is junk. No other values are will work reliably. This is not the same as the scale used by a plugin. You really need to think of junkscore as a binary variable with ==0 for ham, and ==100 for spam, and all other values not used. Whatever scale used by a plugin is remapped to these two values whenever OnMessageClassified is called.

In my first incarnation of bug 366491, I tried to rename that variable to eliminate this confusion. But that caused too many changes, and David was nervous. So instead, I left the confusing old usage, and created a new variable (currently called junkpercent in my latest version) that represents an integer with values between 0 and 100, and can have intermediate values. This is designed to show the uncertainty from a plugin which is needed if you want to filter on junk uncertainty. This is the value that could be meaningfully compared to some value such as 50.

So my objection to using 50 as a cutoff is that it reinforces the misunderstanding about the true nature of junkscore. If you are going to touch this part of the code, I would suggest that you take code like:

atoi(aJunkScore) > 50 

and replace it with

atoi(aJunkScore) > JUNK_LOWSCORE

or, probably even clearer,

atoi(aJunkScore) != JUNK_LOWSCORE

I guess you could argue that this is a riskier change then simply replacing magic constants. But why create a new constant that would just be removed by someone who tried to cleanup the code, particularly in an interface? It's also confusing because the real default cutoff in the current plugin is 90, not 50. So the value of 50 is totally meaningless, and should not be dignified with a constant in an interface.

But I could accept other arguments, as we're really talking here about damage control from some sloppy programming ages ago.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 303090 [details] [diff] [review]
proposed fix

I'll make some changes to this over the weekend.

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Don't use a cutoff value, since the score can be only 0 or 100. Also found a few more places the magic numbers were used.

Comment 9

[Kent James (:rkent)]

Do we have any estimate of when this might get reviewed and incorporated? These changes are in the same sections of code that my bug 366491 touches, so I really want to apply this patch before I submit another version of my patch.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 303774 [details] [diff] [review]
proposed fix, v2

+    properties->AppendElement(atoi(junkScoreStr.get()) == nsIJunkMailPlugin::IS_SPAM_SCORE
+                              ? kJunkMsgAtom : kNotJunkMsgAtom);

? should be at the end of the line.

+        aValue.Assign(GetString((!junkScoreStr.IsEmpty() &&
+                                (atoi(junkScoreStr.get()) == nsIJunkMailPlugin::IS_SPAM_SCORE)) ?
+                      NS_LITERAL_STRING("messageJunk").get() : EmptyString().get()));

Not your fault, but please drop the .get() from the end of NS_LITERAL_STRING and EmptyString.

-  // I set the cut off at 50. this may change
-  // it works for our bayesian plugin, as "0" is good, and "100" is junk
-  // but it might need tweaking for other plugins
-  if (atoi(junkScoreStr.get()) > 50) {
+  if (atoi(junkScoreStr.get()) == nsIJunkMailPlugin::IS_SPAM_SCORE) {
     PR_LOG(MsgPurgeLogModule, PR_LOG_ALWAYS, ("added message to delete"));
     return mHdrsToDelete->AppendElement(aMsgHdr);
   }
   else
     return NS_OK;

Again, not your faul, but could you drop the else before the return please.

Comment 11

[Mark Banner (:standard8)]

So with (and maybe without this change) I couldn't implement a plugin that stored a number other than 0 or 100 alongside the message? Well, I could, as long as the threshold was > 99.

So how could I debug an extension over a period of time where I did want to store the score alongside the message (i.e. so I could try it for a time and check what messages were set to later)? Also in this case, I wouldn't be able to vary the threshold via my extension. I can see why from the current code base that its not really necessary, but I'm thinking about future options as well.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 303774 [details] [diff] [review]
proposed fix, v2

-  {
-    // I set the cut off at 50. this may change
-    // it works for our bayesian plugin, as "0" is good, and "100" is junk
-    // but it might need tweaking for other plugins
-    properties->AppendElement(atoi(junkScoreStr.get()) > 50 ? kJunkMsgAtom : kNotJunkMsgAtom);
-  }
+    properties->AppendElement(atoi(junkScoreStr.get()) == nsIJunkMailPlugin::IS_SPAM_SCORE
+                              ? kJunkMsgAtom : kNotJunkMsgAtom);

Although it was this way before, I wonder if it is better to use nsCString::ToInteger here rather than atoi?

Comment 13

[Kent James (:rkent)]

Re comment 11: "I couldn't implement a plugin that stored a number other than 0 or 100 alongside the message?" Well that is pretty much the intention of bug 366491  There I propose that the main interface to the plugin be changed so that it returns a number between 0 and 100 that is proportional to junk likelihood. Then there is the followup bug 414179 that allows you to filter on that number. I've been running my local TB with an implementation of both of those bugs for awhile.

The issue in the current bug is, what is the definition of the variable junkScore?  I've already discussed this in comment 6 as well as in bug 366491 

Comment 14

[Magnus Melin [:mkmelin]]

(In reply to comment #10)
> Not your fault, but please drop the .get() from the end of NS_LITERAL_STRING
> and EmptyString.

Actually, those are needed, doesn't compile otherwise.

Comment 15

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Addressing review comments, using ToInteger instead.
I think comment 13 covers why a threshold isn't needed - the score numbers I'm fixing now will be mostly for backward compat.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 306714 [details] [diff] [review]
proposed fix, v3

+    properties->AppendElement(junkScoreStr.ToInteger((PRInt32*)&rv) == nsIJunkMailPlugin::IS_SPAM_SCORE ?
+                              kJunkMsgAtom : kNotJunkMsgAtom);
+  NS_ENSURE_SUCCESS(rv, rv);

Nit: indentation needs fixing.

r=me with that fixed.

Comment 17

[[e-mail]]

Comment on attachment 306714 [details] [diff] [review]
proposed fix, v3

Sorry for overlooking this. In fact, I've since bitrotted you by removing nsMsgKeyArray.

>-[scriptable, uuid(caf3d467-d57c-11d6-96a9-00039310a47a)]
>+[scriptable, uuid(7fc75005-444d-4042-80ba-86afdb576089)]
Addition of constants doesn't require a uuid change, does it? I think in certain cases you're event allowed to do it on a frozen interface.

>+      junkStatus = (atoi(aJunkScore) == nsIJunkMailPlugin::IS_SPAM_SCORE) ?
>+          junkStatus = nsIJunkMailPlugin::JUNK :
>           junkStatus = nsIJunkMailPlugin::GOOD;
This doesn't look quite right. Either convert it to an if/else or remove the spurious assignments on the RHS.

>-    properties->AppendElement(atoi(junkScoreStr.get()) > 50 ? kJunkMsgAtom : kNotJunkMsgAtom);
>-  }
>+    properties->AppendElement(junkScoreStr.ToInteger((PRInt32*)&rv) == nsIJunkMailPlugin::IS_SPAM_SCORE ?
>+                              kJunkMsgAtom : kNotJunkMsgAtom);
>+  NS_ENSURE_SUCCESS(rv, rv);
There are many cases like this, so I'll pick on this one: the old code didn't actually error-check, so how sure are you that it won't suddenly start generating errors?

>+        aValue.Assign(GetString((!junkScoreStr.IsEmpty() &&
>+                                (junkScoreStr.ToInteger((PRInt32*)&rv) == nsIJunkMailPlugin::IS_SPAM_SCORE)) ?
>+                      NS_LITERAL_STRING("messageJunk").get() : EmptyString().get()));
>+        NS_ENSURE_SUCCESS(rv, rv);
Wow, this is an ugly line. Consider cleaning it up in a couple of ways:
1. aValue is already the empty string, so you only need to assign it in the junk case (and you can then use AssignLiteral).
2. ToInteger return 0 in the empty string case (although then you would have to ignore the error return).

Comment 18

[Magnus Melin [:mkmelin]]

(In reply to comment #17)
> (From update of attachment 306714 [details] [diff] [review])
> Addition of constants doesn't require a uuid change, does it? I think in

I'll skip changing it.

> >+      junkStatus = (atoi(aJunkScore) == nsIJunkMailPlugin::IS_SPAM_SCORE) ?
> >+          junkStatus = nsIJunkMailPlugin::JUNK :
> >           junkStatus = nsIJunkMailPlugin::GOOD;
> This doesn't look quite right. Either convert it to an if/else or remove the
> spurious assignments on the RHS.

Really nice catch!

> >-    properties->AppendElement(atoi(junkScoreStr.get()) > 50 ? kJunkMsgAtom : kNotJunkMsgAtom);
> >-  }
> >+    properties->AppendElement(junkScoreStr.ToInteger((PRInt32*)&rv) == nsIJunkMailPlugin::IS_SPAM_SCORE ?
> >+                              kJunkMsgAtom : kNotJunkMsgAtom);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> There are many cases like this, so I'll pick on this one: the old code didn't
> actually error-check, so how sure are you that it won't suddenly start
> generating errors?

I wouldn't expect problems, but we could perhaps use an assert instead. Opinions?

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: proposed fix (for checkin)

Unbitrotted patch for checkin. Decided to use an assert instead if the conversion to integer fails.

Comment 20

[Magnus Melin [:mkmelin]]

Checking in mail/base/content/mailCommands.js;
/cvsroot/mozilla/mail/base/content/mailCommands.js,v  <--  mailCommands.js
new revision: 1.41; previous revision: 1.40
done
Checking in mailnews/base/resources/content/mailCommands.js;
/cvsroot/mozilla/mailnews/base/resources/content/mailCommands.js,v  <--  mailCommands.js
new revision: 1.108; previous revision: 1.107
done
Checking in mailnews/base/search/public/nsIMsgFilterPlugin.idl;
/cvsroot/mozilla/mailnews/base/search/public/nsIMsgFilterPlugin.idl,v  <--  nsIMsgFilterPlugin.idl
new revision: 1.15; previous revision: 1.14
done
Checking in mailnews/base/search/src/nsMsgSearchTerm.cpp;
/cvsroot/mozilla/mailnews/base/search/src/nsMsgSearchTerm.cpp,v  <--  nsMsgSearchTerm.cpp
new revision: 1.154; previous revision: 1.153
done
Checking in mailnews/base/src/nsMsgDBView.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgDBView.cpp,v  <--  nsMsgDBView.cpp
new revision: 1.305; previous revision: 1.304
done
Checking in mailnews/base/src/nsMsgPurgeService.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgPurgeService.cpp,v  <--  nsMsgPurgeService.cpp
new revision: 1.30; previous revision: 1.29
done
Checking in mailnews/base/src/nsMsgQuickSearchDBView.cpp;
/cvsroot/mozilla/mailnews/base/src/nsMsgQuickSearchDBView.cpp,v  <--  nsMsgQuickSearchDBView.cpp
new revision: 1.34; previous revision: 1.33
done
Checking in mailnews/base/util/nsMsgDBFolder.cpp;
/cvsroot/mozilla/mailnews/base/util/nsMsgDBFolder.cpp,v  <--  nsMsgDBFolder.cpp
new revision: 1.331; previous revision: 1.330
done
Checking in mailnews/imap/src/nsImapMailFolder.cpp;
/cvsroot/mozilla/mailnews/imap/src/nsImapMailFolder.cpp,v  <--  nsImapMailFolder.cpp
new revision: 1.800; previous revision: 1.799
done
Checking in mailnews/local/src/nsLocalMailFolder.cpp;
/cvsroot/mozilla/mailnews/local/src/nsLocalMailFolder.cpp,v  <--  nsLocalMailFolder.cpp
new revision: 1.572; previous revision: 1.571
done
Checking in mailnews/local/src/nsParseMailbox.cpp;
/cvsroot/mozilla/mailnews/local/src/nsParseMailbox.cpp,v  <--  nsParseMailbox.cpp
new revision: 1.298; previous revision: 1.297
done
Checking in mailnews/public/MailNewsTypes2.idl;
/cvsroot/mozilla/mailnews/public/MailNewsTypes2.idl,v  <--  MailNewsTypes2.idl
new revision: 1.13; previous revision: 1.12
done

->FIXED