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

I'll look into it.

Added support for multiple address books for whitelist.

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 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); });

(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.

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

Checked in changeset id: 154:a5e182244d42