449747 - Running Junk Mail Controls manually on Folder or message fails if multiple address books selected for whitelist

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect)

Description

[Mark Banner (:standard8)]

I spotted this whilst looking at some code, I couldn't find a duplicate.

If a user has multiple address books selected for junk whitelist, and then tries to run junk mail controls on a folder or a specific message, it will fail:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /Users/moztest/comm/main/src/mailnews/addrbook/src/nsAbDirectoryRDFResource.cpp, line 69
##!!! ASSERTION: unable to initialize resource: 'Error', file /Users/moztest/comm/main/src/mozilla/rdf/base/src/nsRDFService.cpp, line 1030
An error occurred executing the cmd_runJunkControls command
[Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIRDFService.GetResource]" nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)" location: "JS frame :: chrome://messenger/content/junkCommands.js :: processFolderForJunk :: line 362" data: no]

See

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/resources/content/junkCommands.js&rev=1.6&mark=362#356

and compare with:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/util/nsMsgDBFolder.cpp&rev=1.353&mark=1863-1881#1861

Comment 1

[Kent James (:rkent)]

I'll look into it.  

Comment 2

[Kent James (:rkent)]

Attachment: Support multiple address books

Added support for multiple address books for whitelist.

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 334640 [details] [diff] [review]
Support multiple address books

>+        if (abCard)
>         {
>-          var db = aMsgHdr.folder.getMsgDatabase(msgWindow);
>-          db.setStringProperty(aMsgHdr.messageKey, "junkscore", Components.interfaces.nsIJunkMailPlugin.IS_HAM_SCORE);
>-          db.setStringProperty(aMsgHdr.messageKey, "junkscoreorigin", "whitelist");
>-          this.mGoodMsgHdrs.appendElement(aMsgHdr, false);
>+          // message is ham from whitelist
>+          {
>+            var db = aMsgHdr.folder.getMsgDatabase(msgWindow);
>+            db.setStringProperty(aMsgHdr.messageKey, "junkscore",
>+                                 Components.interfaces.nsIJunkMailPlugin.IS_HAM_SCORE);
>+            db.setStringProperty(aMsgHdr.messageKey, "junkscoreorigin", "whitelist");
>+            this.mGoodMsgHdrs.appendElement(aMsgHdr, false);
>+          }
>+          return;

nit: you don't need the extra bracket set/indentation here.

>+    var whiteListAbURIs = spamSettings.whiteListAbURI.split(" ");
>+    for (var abCount = 0; abCount < whiteListAbURIs.length; abCount++)
>+    {
>+      whiteListDirectories.push(Components.classes["@mozilla.org/abmanager;1"]
>+                                          .getService(Components.interfaces.nsIAbManager)
>+                                          .getDirectory(whiteListAbURIs[abCount]));
>+    }

Please cache the nsIAbManager service in a local variable.

r=me with those fixed.

Comment 4

[[e-mail]]

Comment on attachment 334640 [details] [diff] [review]
Support multiple address books

>-    if (aWhiteListDirectory)
>+    if (aWhiteListDirectories.length)
I'm undecided as to whether it makes sense to null-check aWhiteListDirectories.

>       var abCard = false;
>-      try {
>-        abCard = aWhiteListDirectory.cardForEmailAddress(authorEmailAddress);
>-      } catch (e) {}
>-      if (abCard)
>       {
>-        // message is ham from whitelist
>         {
Looks like the old code had the same bug.

The other approach is something like this:
try {
  if (aWhiteListDirectories[abCount].cardForEmailAddress(authorEmailAddress) {
    // message is ham from whitelist
    var db = aMsgHdr.folder.getMsgDatabase(msgWindow);
    db.setStringProperty(aMsgHdr.messageKey, "junkscore",
                         Components.interfaces.nsIJunkMailPlugin.IS_HAM_SCORE);
    db.setStringProperty(aMsgHdr.messageKey, "junkscoreorigin", "whitelist");
    this.mGoodMsgHdrs.appendElement(aMsgHdr, false);
    return;
  }
}

>+  // if enabled in the spam settings, retrieve whitelist addressbooks
>+  var whiteListDirectories = [];
>   if (spamSettings.useWhiteList && spamSettings.whiteListAbURI) {
>+    var whiteListAbURIs = spamSettings.whiteListAbURI.split(" ");
>+    for (var abCount = 0; abCount < whiteListAbURIs.length; abCount++)
>+    {
>+      whiteListDirectories.push(Components.classes["@mozilla.org/abmanager;1"]
>+                                          .getService(Components.interfaces.nsIAbManager)
>+                                          .getDirectory(whiteListAbURIs[abCount]));
>+    }
If you wanted to be really slick you could probably do something like
whiteListDirectories = spamSettings.whiteListAbURI.split(" ").map(
  function (aURI) {
    return abManager.getDirectory(aURI);
  });

Comment 5

[Kent James (:rkent)]

(In reply to comment #4)
> (From update of attachment 334640 [details] [diff] [review])
> >-    if (aWhiteListDirectory)
> >+    if (aWhiteListDirectories.length)
> I'm undecided as to whether it makes sense to null-check aWhiteListDirectories.

The original null value was specifically used to communicate that there is no whitelisting. I'm simply using the length of the array for the same purpose. Otherwise, I would have had to redefine aWhiteListDirectories as an array on the first successful path through a for loop earlier - which is ugly.

If by null-checking you mean checking as a safety feature, I don't see the value. This isn't C++ where such an error crashes everything. Plus to be really error-free, we would need to check the type of aWhiteListDirectories to make sure it is an array, not just null check - which is excessive, particularly since this function is really local to this file as used.

Comment 6

[Kent James (:rkent)]

Attachment: Implemented Standard8 and Neil's comments

Implemented comments, except for Neil's "really slick" possibility. Maybe I'll try slick another day, another bug. Patch to checkin.

Comment 7

[Mark Banner (:standard8)]

Checked in changeset id: 154:a5e182244d42