Improve (null) checks in nsImapService.cpp

RESOLVED FIXED in Thunderbird 12.0

Status

MailNews Core
Networking: IMAP
--
critical
RESOLVED FIXED
8 years ago
3 years ago

People

(Reporter: sgautherie, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 12.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

8 years ago
Blocks: 23302
(Reporter)

Updated

8 years ago
Blocks: 83709
(Reporter)

Updated

8 years ago
Blocks: 15865
(Reporter)

Updated

8 years ago
Blocks: 137006
(Reporter)

Updated

8 years ago
Blocks: 33451
(Reporter)

Comment 1

8 years ago
Created attachment 491392 [details] [diff] [review]
(Av1) Improve some NS_ENSURE_ARG_POINTER(), plus some nits

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)
(Reporter)

Comment 2

8 years ago
Created attachment 491458 [details] [diff] [review]
(Bv1) Fix 5 crashes reported by bug 412109 (WIP) test, plus some nits

(Applies on top of patch Av1.)
Attachment #491458 - Flags: review?(bienvenu)
(Reporter)

Updated

8 years ago
Target Milestone: Thunderbird 3.3a1 → Thunderbird 3.3a2
(Reporter)

Comment 3

8 years ago
David, ping for review.

Comment 4

8 years ago
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 5

8 years ago
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-
(Reporter)

Comment 6

8 years ago
(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?

Comment 7

8 years ago
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.

Comment 8

8 years ago
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

Updated

7 years ago
Depends on: 684481

Comment 9

7 years ago
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 → ---

Comment 10

7 years ago
(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.
(Reporter)

Comment 11

7 years ago
Created attachment 574515 [details] [diff] [review]
(Av2) nsImapService.cpp: Improve some NS_ENSURE_ARG_POINTER(), plus some nits
[Checked in: Comment 13]

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)
(Reporter)

Comment 12

7 years ago
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 13

7 years ago
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+

Updated

7 years ago
Severity: critical → trivial
Target Milestone: --- → Thunderbird 11.0

Comment 14

7 years ago
no real world crashes, so removing keyword
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: crash
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
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]
(Reporter)

Comment 15

7 years ago
Created 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

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)
(Reporter)

Comment 16

7 years ago
(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 → ---

Comment 17

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

Comment 18

7 years ago
(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!

Comment 19

7 years ago
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**)
(Reporter)

Comment 20

7 years ago
(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

Comment 21

7 years ago
OK, thx, I'd prefer the crash keyword to refer to crashes our users might see...
Keywords: crash
(Reporter)

Comment 22

7 years ago
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 24

7 years ago
Created 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]

I de-bitrotted this patch, and it looks OK, thx.
Attachment #577421 - Attachment is obsolete: true
Attachment #587412 - Flags: review+
Attachment #577421 - Flags: review?(dbienvenu)
(Reporter)

Comment 25

7 years ago
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?
(Reporter)

Updated

7 years ago
Attachment #587412 - Attachment filename: 611233-Bv2_bug-412109-crashes.diff → 611233-Bv2a_bug-412109-crashes.diff

Comment 26

7 years ago
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-
(Reporter)

Updated

7 years ago
Whiteboard: [fixed in TB 11: Av2]
Target Milestone: Thunderbird 11.0 → Thunderbird 12.0

Comment 28

6 years ago
Is this still open?

Comment 29

6 years ago
Is this intentionally left as unresolved?

Comment 30

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

Comment 31

6 years ago
(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)

Updated

6 years ago
Status: ASSIGNED → NEW

Comment 33

3 years ago
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
Last Resolved: 7 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.