1277606 - (coverity) mailnews/base/src/nsMsgCopyService.cpp: Checking for |copysource| nullness is missing in one place

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[ISHIKAWA, Chiaki]

Coverity found this.
In mailnews/base/src/nsMsgCopyService.cpp, checking for
|copysource| being null or not is missing (?).
From the comparisons with nearby source lines, that it could be
null somehow seems to be a real-world concern.

So it should be checked.

A patch follows.

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Check whether |copysource| is NULL or not.

Here is a patch.
Come to think of it, it may be that |copysource|check may not be at the top-level
condition in the if(), but rather checked inside the if () { }
as in the next code block?

The patch has been tested, though, in  
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=db719c35fbb3b1981178716d68a72ae34d31f8ac

(the patch is 
 82531f4cb9a6 IC whether |copysource| is nullptr or not should be checked. The attached one has a slightly changed comment line.)
  
and it does not seem to have a problem(?)

Whom should I ask for a review?

Comment 2

[:aceman]

Comment on attachment 8759221 [details] [diff] [review]
Check whether |copysource| is NULL or not.

But when is copySource null? Doesn't it mean some bigger problem that we should not just hide by null-checking it?

Comment 3

[Kent James (:rkent)]

Comment on attachment 8759221 [details] [diff] [review]
Check whether |copysource| is NULL or not.

Review of attachment 8759221 [details] [diff] [review]:
-----------------------------------------------------------------

The copy service is one of the poster children for a method where the responsibility to do notifications is completely unclear, the notifications themselves are not clearly defined, and there is no error handling that anyone understands. If we return an error rv from DoNextCopy, will that properly get propagated to the caller and user in all reasonable cases? I don't really know.

Given that this request is coming from a theoretical issue rather than something that is known to be an issue, I think that we should do the right thing here and hope for the best. That is, set rv = NS_ERROR_UNEXPECTED and add a warning when there is null copySource. So I suppose that I am agreeing with aceman.

What to do about ClearRequests? Another classic example of where the responsibility for this notification is diffuse, so we can't really predict what we are supposed to be doing here. We "should" be calling it, but that has its own unchecked copySource here:

notifier->NotifyFolderMoveCopyCompleted(aRequest->m_isMoveOrDraftOrTemplate, copySource->m_msgFolder, aRequest->m_dstFolder);

so we would just move the crash. So let's skip that.

So in the end, just adding NS_ENSURE_STATE(copySource) before the call would do the trick. That's what I would recommend.

This method would be a prime candidate to clean up the callback plan, and convert to JS. It has been the source of endless regressions over the years as we have tried to modify other code.

Comment 4

[ISHIKAWA, Chiaki]

(In reply to Kent James (:rkent) from comment #3)
> Comment on attachment 8759221 [details] [diff] [review]
> Check whether |copysource| is NULL or not.
> 
> Review of attachment 8759221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The copy service is one of the poster children for a method where the
> responsibility to do notifications is completely unclear, the notifications
> themselves are not clearly defined, and there is no error handling that
> anyone understands. If we return an error rv from DoNextCopy, will that
> properly get propagated to the caller and user in all reasonable cases? I
> don't really know.
> 

Is there a document about the notification flow somewhere?
I see listeners everywhere, but there is no coherent diagram or anything.
(That was a surprise when I dived into the source code to fix some bugs.)

The situation is basically the same as unrestrained use of "goto"s with many different labels IMHO :-(

> Given that this request is coming from a theoretical issue rather than
> something that is known to be an issue, I think that we should do the right
> thing here and hope for the best. That is, set rv = NS_ERROR_UNEXPECTED and
> add a warning when there is null copySource. So I suppose that I am agreeing
> with aceman.
> 
> What to do about ClearRequests? Another classic example of where the
> responsibility for this notification is diffuse, so we can't really predict
> what we are supposed to be doing here. We "should" be calling it, but that
> has its own unchecked copySource here:
> 
> notifier->NotifyFolderMoveCopyCompleted(aRequest->m_isMoveOrDraftOrTemplate,
> copySource->m_msgFolder, aRequest->m_dstFolder);
> 
> so we would just move the crash. So let's skip that.
> 
> So in the end, just adding NS_ENSURE_STATE(copySource) before the call would
> do the trick. That's what I would recommend.
> 

I think I agree in principle. But where is exactly this NS_ENSURE_STATE(copySource) supposed to be placed?
I am a little hazy.
 ...
          else if (copyRequest->m_requestType == nsCopyFoldersType )
          {
 Here  --->   NS_ENSURE_STATE(copySource);
              copySource->m_processed = true;
               ...


> This method would be a prime candidate to clean up the callback plan, and
> convert to JS. It has been the source of endless regressions over the years
> as we have tried to modify other code.

I think it is a good idea to check for the sanity of the code by using NS_ENSURE_STATE() and friends to make sure some strange conditions are at least caught in the debug build, etc.

TIA

Comment 5

[Kent James (:rkent)]

(In reply to ISHIKAWA, Chiaki from comment #4)
> (In reply to Kent James (:rkent) from comment #3)
> 
> Is there a document about the notification flow somewhere?
> I see listeners everywhere, but there is no coherent diagram or anything.
> (That was a surprise when I dived into the source code to fix some bugs.)

I wish that there was, but I am not aware of any. Some documentation of this would be really helpful, as it is really difficult to get all of the notifications correct when you change anything.

> 
> I think I agree in principle. But where is exactly this
> NS_ENSURE_STATE(copySource) supposed to be placed?
> I am a little hazy.
>  ...
>           else if (copyRequest->m_requestType == nsCopyFoldersType )
>           {
>  Here  --->   NS_ENSURE_STATE(copySource);
>               copySource->m_processed = true;
>                ...

Yes, in the end that simple change is all that I think we need.

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Check |copysource| nullness with NS_ENSURE_STATE(copysource)

NS_ENSURE_STATE(copysource) check.
Compiles locally. TIA

Comment 7

[ISHIKAWA, Chiaki]

Thank you. I will put the checkin-needed keyword.

Comment 8

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/03959f38f7bb