431806 - Unnecessary warnings when opening a local .eml file

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Kent James (:rkent)]

In debug mode, when opening a local .eml file for display, you receive two warnings in nsMailBoxUrl.cpp. The first is NS_ENSURE_TRUE(msg) failed at line 447, the second is NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005 at line 455

These warnings can be traced to attempts to get the folder character set, which is meaningless for a file email. So these warnings are for operations that occur during the normal course of events, and should not generate console warnings. Reading and processing of the email proceeds normally even with the warnings.

While these error messages are rather trivial during normal operation, during testing of the bayesian filter they generate huge amounts of console noise. The easiest way to pipe an email to the bayesian filter, either for unit test or for effectiveness testing, is to read a file email in the same manner.

Change the testing in these routines so no warning is shown, but the error is passed up the chain where it is then properly handled.

Comment 1

[Kent James (:rkent)]

There's another warning I also want to fix in this bug. This is at http://mxr.mozilla.org/seamonkey/source/mailnews/db/msgdb/src/nsMsgDatabase.cpp#153 where I get "NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80550006" when the routine fails to find folder flags. As with the other warning, just detect this and pass it to the caller without a warning.

Comment 2

[Kent James (:rkent)]

Attachment: Replace warnings with simple error returns

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 319503 [details] [diff] [review]
Replace warnings with simple error returns

I'm going to jump in here.

@@ -145,17 +145,18 @@ NS_IMETHODIMP nsMsgDBService::OpenFolder
     {
       PRInt32 numMessages;
       msgDatabase->m_mdbAllMsgHeadersTable->GetCount(msgDatabase->GetEnv(), &numHdrsInTable);
       msgDatabase->m_dbFolderInfo->GetNumMessages(&numMessages);
       if (numMessages != (PRInt32) numHdrsInTable)
         msgDatabase->SyncCounts();
     }
   }
-  NS_ENSURE_SUCCESS(rv, rv);
+  if (NS_FAILED(rv))
+    return rv;

Please see my comment on bug 421050 comment 3 and bug 421050 comment 9. I'd prefer slightly more detail here, because there are only two cases we expect this to fail. Anything else and at least in debug mode we should warn the developer.

Comment 4

[Kent James (:rkent)]

I see that bug 421050 is trying to cure the same warning that I mention in comment 1. Since that bug is ahead of this one, I'll defer to it and eliminate the change.

That still leaves the changes associated with the warnings in comment 0. I don't see how to approach those in the manner as your comments on the other section, since there is no upstream error code to check. What do you suggest?

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> That still leaves the changes associated with the warnings in comment 0. I
> don't see how to approach those in the manner as your comments on the other
> section, since there is no upstream error code to check. What do you suggest?

That's a good point. I'm actually not too worried on these ones, because they aren't quite as prominent. It would be good though to add some comments to the code about what's going on, especially in GetFolderCharset, and probably to the idl as well, saying that it may return failure if the url doesn't have a folder associated with it.

We probably should also set *aCharacterSet to nsnull at the start of that function, and NS_ENSURE_ARG_POINTER on it as well.

Comment 6

[David :Bienvenu]

Comment on attachment 319503 [details] [diff] [review]
Replace warnings with simple error returns

I'm OK with this, modulo Standard8's comments.

Comment 7

[Kent James (:rkent)]

Attachment: More comments, no nsMsgDatabase.cpp change

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 319672 [details] [diff] [review]
More comments, no nsMsgDatabase.cpp change

>+
>+  // fails with NS_ERROR_FAILURE for standalone .eml messages
>   attribute nsIMsgFolder folder;

Could you make this:

/**
 * The folder associated with this url.
 * 
 * @exception NS_ERROR_FAILURE    May be thrown if the url does not
 *                                relate to a folder, e.g. standalone
 *                                .eml messages.
 */

>+  // fails with NS_ERROR_FAILURE for standalone .eml messages
>   readonly attribute string folderCharset;

I think this could be commented in the same way.

> NS_IMETHODIMP nsMailboxUrl::GetFolderCharset(char ** aCharacterSet)
> {
>   nsCOMPtr<nsIMsgFolder> folder;
>+  NS_ENSURE_ARG_POINTER(aCharacterSet);

Please put the NS_ENSURE... before the nsCOMPtr declaration.

r=me with those fixed.

Comment 9

[Kent James (:rkent)]

Attachment: Fixed nits and comments

Ready to checkin, carrying over r and sr.

Comment 10

[Phil Ringnalda (:philor)]

Adding checkin-needed while mail/mailnews is frozen is just going to lead someone who doesn't know we're frozen to check it in at the wrong time.

Comment 11

[Mark Banner (:standard8)]

Patch checked in:
Checking in mailnews/base/public/nsIMsgMailNewsUrl.idl;
/cvsroot/mozilla/mailnews/base/public/nsIMsgMailNewsUrl.idl,v  <--  nsIMsgMailNewsUrl.idl
new revision: 1.35; previous revision: 1.34
done
Checking in mailnews/local/src/nsMailboxUrl.cpp;
/cvsroot/mozilla/mailnews/local/src/nsMailboxUrl.cpp,v  <--  nsMailboxUrl.cpp
new revision: 1.116; previous revision: 1.115
done

Comment 12

[Magnus Melin [:mkmelin]]

This is fixed, no?

Comment 13

[Kent James (:rkent)]

Yep, it's fixed. Fixing.