Closed Bug 611233 Opened 9 years ago Closed 4 years ago

Improve (null) checks in nsImapService.cpp

Categories

(MailNews Core :: Networking: IMAP, defect, critical)

defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 12.0

People

(Reporter: sgautherie, Unassigned)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [fixed in TB 11: Av2])

Attachments

(2 files, 3 obsolete files)

No description provided.
Blocks: 23302
Blocks: 83709
Blocks: 15865
Blocks: 137006
Blocks: 33451
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.
Attachment #491392 - Flags: review?(bienvenu)
(Applies on top of patch Av1.)
Attachment #491458 - Flags: review?(bienvenu)
Target Milestone: Thunderbird 3.3a1 → Thunderbird 3.3a2
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.
Attachment #491458 - Flags: review?(bienvenu) → review-
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.
Attachment #491392 - Flags: review?(bienvenu) → review-
(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
Depends on: 684481
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.
Target Milestone: Thunderbird 3.3a2 → ---
(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...
Attachment #491392 - Attachment is obsolete: true
Attachment #574515 - Flags: review?(dbienvenu)
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
Attachment #574515 - Flags: review?(dbienvenu) → review+
Severity: critical → trivial
Target Milestone: --- → Thunderbird 11.0
no real world crashes, so removing keyword
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Keywords: crash
Resolution: --- → FIXED
Attachment #574515 - Attachment description: (Av2) nsImapService.cpp: Improve some NS_ENSURE_ARG_POINTER(), plus some nits. → (Av2) nsImapService.cpp: Improve some NS_ENSURE_ARG_POINTER(), plus some nits [Checked in: Comment 13]
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.
Attachment #491458 - Attachment is obsolete: true
Attachment #577421 - Flags: review?(dbienvenu)
(In reply to David :Bienvenu from comment #14)
> no real world crashes, so removing keyword

Err, no.
Severity: trivial → critical
Status: RESOLVED → REOPENED
Flags: in-testsuite-
Keywords: crash
Resolution: FIXED → ---
(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...)
Status: REOPENED → ASSIGNED
OK, thx, I'd prefer the crash keyword to refer to crashes our users might see...
Keywords: crash
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...
Attachment #577421 - Flags: review?(mbanner)
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.
Attachment #577421 - Flags: review?(mbanner)
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.
Attachment #587412 - Attachment description: de-bitrotted patch → (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]
Attachment #587412 - Flags: approval-comm-aurora?
Attachment #587412 - Attachment filename: 611233-Bv2_bug-412109-crashes.diff → 611233-Bv2a_bug-412109-crashes.diff
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.
Attachment #587412 - Flags: approval-comm-aurora? → approval-comm-aurora-
Whiteboard: [fixed in TB 11: Av2]
Target Milestone: Thunderbird 11.0 → Thunderbird 12.0
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?
Flags: needinfo?(sgautherie.bz)
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.
Assignee: sgautherie.bz → nobody
Flags: needinfo?(sgautherie.bz)
Status: ASSIGNED → NEW
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
Status: NEW → RESOLVED
Closed: 8 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.