These are just some clean-ups I noticed yet. *Remove all |NS_ENSURE_ARG_POINTER(aClientEventTarget);| and similar, as nsImapService::GetImapConnectionAndLoadUrl() calls nsImapIncomingServer::GetImapConnectionAndLoadUrl(), which calls nsImapIncomingServer::GetImapConnection(), which check it first argument before using it, since it exists, in http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/imap/src/nsImapIncomingServer.cpp&rev=1.7. *Remove 2 unused |nsCAutoString msgKey;|. *1st: Leftover in bug 23302. *2nd: Uselessly added in bug 83709. *1 s/NS_ENSURE_ARG/NS_ENSURE_ARG_POINTER/. *Added in bug 15865. *Remove 1 NS_ENSURE_ARG. *Added in bug 137006, then leftover (as NS_SetPersistentFile() checks its args) in bug 33451.

(Applies on top of patch Av1.)

David, ping for review.

Comment on attachment 491458 [details] [diff] [review] (Bv1) Fix 5 crashes reported by bug 412109 (WIP) test, plus some nits in general, we prefer to declare vars where they are first used, so you should change the code to look like: nsresult rv = DecomposeImapURI(messageURI, getter_AddRefs(folder), msgKey); instead of doing nsresult rv; Or you could just attach a patch that fixes the bug w/o the other unrelated changes... Also, I tend to prefer NS_ENSURE_SUCCESS(rv, rv), over if (NS_FAILED(rv) return rv, if the failure is unexpected.

Comment on attachment 491392 [details] [diff] [review] (Av1) Improve some NS_ENSURE_ARG_POINTER(), plus some nits those are interface methods, so they need to do the arg checking.

(In reply to comment #5) > those are interface methods, so they need to do the arg checking. I am new to this. Could you tell me more about that policy, so I learn how to fix these and the future ones the right way: when/which args should be checked?

if it's an NS_IMETHODIMP (i.e. something defined in a .idl interface) then it should handle null, if it is only defined in a .h or .cpp, then it's generally considered private.

If you're not familiar with xpcom (and you really should be to make changes to anything non-trivial in the backend), you should read https://developer.mozilla.org/en/Creating_XPCOM_Components/An_Overview_of_XPCOM#Interfaces

Serge, will you continue on this? Or can I try addressing David's comments? David, are you objecting to the NS_ENSURE_ARG_POINTER removals by Serge? As I understand your comments, all those interface functions should check for passed nulls (even if currently no callers are passing them as Serge says). The goal is even to add these checks where they are missing (per bug 412109). Is that correct? I understand what you mean with the "nsresult rv" initializations.

(In reply to :aceman from comment #9) > Serge, will you continue on this? Or can I try addressing David's comments? > > David, are you objecting to the NS_ENSURE_ARG_POINTER removals by Serge? Yes, those changes were wrong. > As I understand your comments, all those interface functions should check > for passed nulls (even if currently no callers are passing them as Serge > says). The goal is even to add these checks where they are missing (per bug > 412109). > Is that correct? yes, those checks are correct.

Av1, with comment 5 suggestion(s), and more. In SetDefaultLocalPath(), |NS_ENSURE_ARG(aPath)| removal assumes the one in NS_SetPersistentFile() is enough...

Comment on attachment 574515 [details] [diff] [review] (Av2) nsImapService.cpp: Improve some NS_ENSURE_ARG_POINTER(), plus some nits [Checked in: Comment 13] And the added checks before FolderCommand(), DiddleFlags() and ChangeFolderSubscription() calls are redundant as is. You may not want them...

Comment on attachment 574515 [details] [diff] [review] (Av2) nsImapService.cpp: Improve some NS_ENSURE_ARG_POINTER(), plus some nits [Checked in: Comment 13] landed http://hg.mozilla.org/comm-central/rev/fac3f825a13b

no real world crashes, so removing keyword

Bv1, with comment 4 suggestion(s). The 5th crash, in nsImapService::DeleteFolder(), was fixed by Av2 patch in the meantime. I'm re-adding NS_ENSURE_ARG_POINTER(aPath) in nsImapService::SetDefaultLocalPath(), for consistency (even if redundant), as I didn't expect that you would accept (and land) patch Av2 as is.

(In reply to David :Bienvenu from comment #14) > no real world crashes, so removing keyword Err, no.

(In reply to Serge Gautherie (:sgautherie) from comment #16) > (In reply to David :Bienvenu from comment #14) > > no real world crashes, so removing keyword > > Err, no. What's the real world crash? Running js code that exercises all the methods is not real world crash, since it doesn't happen in the product.

(In reply to Serge Gautherie (:sgautherie) from comment #16) > (In reply to David :Bienvenu from comment #14) > > no real world crashes, so removing keyword > > Err, no. If you have a real world crash, please paste the stack trace in, steps to reproduce, if known, and any crash-stat incidents so we can track them correctly, thx!

I'm not sure what Serge had in mind when submitting with crash keyword, but steps are of course difficult to get for many of our crashes. FWIW, the following is a list of crash signatures for the past 6 weeks involving nsImapService, and none have bug reports nsImapService::GetUrlForUri(char const*, nsIURI**, nsIMsgWindow*) nsImapService::DisplayMessage(char const*, nsISupports*, nsIMsgWindow*, nsIUrlListener*, char const*, nsIURI**) nsImapService::GetMessageFromUrl(nsIImapUrl*, int, nsIMsgFolder*, nsIImapMessageSink*, nsIMsgWindow*, nsISupports*, int, nsIURI**) nsRefPtr<nsTypedSelection>::~nsRefPtr<nsTypedSelection>() | nsImapService::NewChannel(nsIURI*, nsIChannel**) nsImapService::StreamMessage(char const*, nsISupports*, nsIMsgWindow*, nsIUrlListener*, int, nsACString_internal const&, int, nsIURI**) nsCOMPtr<nsIAbCard>::StartAssignment() | nsImapService::NewChannel(nsIURI*, nsIChannel**) nsImapService::GetServerFromUrl(nsIImapUrl*, nsIMsgIncomingServer**) nsImapService::`scalar deleting destructor''(unsigned int) nsImapService::NewURI(nsACString_internal const&, char const*, nsIURI*, nsIURI**) nsImapService::StoreCustomKeywords(nsIEventTarget*, nsIMsgFolder*, nsIMsgWindow*, nsACString_internal const&, nsACString_internal const&, nsACString_internal const&, nsIURI**) nsImapService::nsImapService() nsRefPtr<nsDOMAttribute>::assign_assuming_AddRef(nsDOMAttribute*) | nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) | nsCOMPtr<nsIURI>::nsCOMPtr<nsIURI>(nsQueryInterface) | nsImapService::GetImapConnectionAndLoadUrl(nsIEventTarget*, nsIIma... nsImapService::AddRef() nsImapService::GetDefaultLocalPath(nsILocalFile**) nsImapService::NewChannel nsImapService::GetCacheSession(nsICacheSession**) nsImapService::FetchMessage(nsIImapUrl*, int, nsIMsgFolder*, nsIImapMessageSink*, nsIMsgWindow*, nsISupports*, nsACString_internal const&, int, nsACString_internal const&, nsIURI**) nsImapService::IsMsgInMemCache(nsIURI*, nsIMsgFolder*, nsICacheEntryDescriptor**, int*) nsImapService::FolderCommand(nsIEventTarget*, nsIMsgFolder*, nsIUrlListener*, char const*, int, nsIMsgWindow*, nsIURI**) nsImapService::nsImapService nsImapService::Biff(nsIEventTarget*, nsIMsgFolder*, nsIUrlListener*, nsIURI**, unsigned int) nsCOMPtr<nsIImapMailFolderSink>::nsCOMPtr<nsIImapMailFolderSink>(nsQueryInterface) | nsImapService::SetImapUrlSink(nsIMsgFolder*, nsIImapUrl*) nsRefPtr<nsCanvasPatternAzure>::~nsRefPtr<nsCanvasPatternAzure>() | nsImapService::StreamMessage(char const*, nsISupports*, nsIMsgWindow*, nsIUrlListener*, int, nsACString_internal const&, int, nsIURI**) nsImapService::OnlineMessageCopy nsCOMPtr<nsIImapMessageSink>::nsCOMPtr<nsIImapMessageSink>(nsQueryInterface) | nsImapService::NewURI(nsACString_internal const&, char const*, nsIURI*, nsIURI**) nsRefPtr<nsTypedSelection>::~nsRefPtr<nsTypedSelection>() | nsImapService::DecomposeImapURI(nsACString_internal const&, nsIMsgFolder**, unsigned int*) nsImapService::NewChannel(nsIURI*, nsIChannel**)

(In reply to David :Bienvenu from comment #17) > What's the real world crash? Running js code that exercises all the methods > is not real world crash, since it doesn't happen in the product. Oh, agreed. Nontheless, note that Joey Minta's bugs which block bug 412109 have 'crash' keyword too: I just carried that over... (In reply to David :Bienvenu from comment #18) > If you have a real world crash, please paste the stack trace in, steps to > reproduce, if known, and any crash-stat incidents so we can track them > correctly, thx! I don't have any specific case, other than the js tool. As I understood it, this work is hoped to (at least) help w.r.t. what Wayne wrote in comment 19. (In reply to Wayne Mery (:wsmwk) from comment #19) > FWIW, the following is a list of crash signatures for the past 6 weeks > involving nsImapService, and none have bug reports I tried to query https://crash-stats.mozilla.com/query?advanced=1 but all I got when I tried to view the actual reports (= stacks !?) is "We couldn't find the OOID you're after" :-< (Last time I used this site was a long time ago...)

OK, thx, I'd prefer the crash keyword to refer to crashes our users might see...

Comment on attachment 577421 [details] [diff] [review] (Bv2) nsImapService.cpp: Fix 4 crashes reported by bug 412109 (WIP) test, Add 3 additional NS_ENSURE_SUCCESS(rv, rv), plus some nits David, ping for review. Or Mark, as I saw you reviewed similar patches...

Comment on attachment 577421 [details] [diff] [review] (Bv2) nsImapService.cpp: Fix 4 crashes reported by bug 412109 (WIP) test, Add 3 additional NS_ENSURE_SUCCESS(rv, rv), plus some nits We've been busy the last couple of weeks, I suspect David will get to this soon after the holidays.

I de-bitrotted this patch, and it looks OK, thx.

Comment on attachment 587412 [details] [diff] [review] (Bv2a) nsImapService.cpp: Fix 4 crashes reported by bug 412109 (WIP) test, Add 3 additional NS_ENSURE_SUCCESS(rv, rv), plus some nits [Checked in: Comment 25] http://hg.mozilla.org/comm-central/rev/775c1ec9d576 "approval-comm-aurora=?": Low risk. May prevent crashes. To go along patch A which went in TB 11.

I highly doubt this will fix any crashes, fwiw.

Comment on attachment 587412 [details] [diff] [review] (Bv2a) nsImapService.cpp: Fix 4 crashes reported by bug 412109 (WIP) test, Add 3 additional NS_ENSURE_SUCCESS(rv, rv), plus some nits [Checked in: Comment 25] AFAIK there's no known crashes here - these are just theoretical, or maybe via a bad extension. I don't think we need to accelerate this.

Is this still open?

Is this intentionally left as unresolved?

Serge, Mark ? Both patches are checked in. Can somebody please state what is left to do here?

(In reply to :aceman from comment #30) > Serge, Mark ? Both patches are checked in. Can somebody please state what is > left to do here?

The work would be to take the test suite in bug 412109 and run it for the imap service, and fix any more crashes. I doubt it'd prevent any real world crashes, but you never know. I'm guessing Serge isn't actually working on this at the moment, so re-assigning to nobody.

FWIW, Serge's last comment in (In reply to Mark Banner (:standard8) from comment #32) > The work would be to take the test suite in bug 412109 and run it for the > imap service, and fix any more crashes. I doubt it'd prevent any real world > crashes, but you never know. It sounds like we should close this given that it's landed a set of patches long ago, and file new bugs for anything that's still needed to complete bug 412109