870236 - Consolidate return value behaviour of nsMsgSearchTerm::Match* functions

Status: RESOLVED FIXED

(MailNews Core :: Search, defect)

Description

[:aceman]

There are some inconsistencies in the nsMsgSearchTerm::Match* functions in what they return in case of failure:
- only some of the functions return NS_ERROR_FAILURE in case an invalid nsMsgSearchOp was used.
- some of them call NS_ASSERTION in that case, some do NS_ERROR, some do nothing. I propose to make all call NS_ERROR.
- some even return true in *pResult (a match) if invalid comparison operation was used (see e.g. http://hg.mozilla.org/comm-central/file/4c366a1789a7/mailnews/base/search/src/nsMsgSearchTerm.cpp#l1473)

There is also some more cleanup possible:
-'break' in the 'default:' branch of 'switch'.
-'Else' after return.
-Change 'err' to 'rv' for coding style.
-empty line after NS_ENSURE_ARG_POINTER() everywhere.
-merge: "nsresult rv = MatchString(dbHdrValue, nullptr, aResult); return rv;"
-comment that MatchStatus() not used only for nsMsgMessageFlags but also for nsMsgFolderFlags (as they are both 'unsigned long' for now). And that in this function nsMsgSearchOp::Is and nsMsgSearchOp::Isnt is intentionally used as Contains/DoesntContain for legacy reasons.

But first I'd like confirmation from rkent that all these changes are welcome.

Comment 1

[Kent James (:rkent)]

The cleanups that you mention all make sense (I assume "-'Else' after return." means to remove Else after return). I also like seeing them in a cleanup bug rather than complicating the review of a fault bug.

Comment 2

[:aceman]

(In reply to Kent James (:rkent) from comment #1)
> The cleanups that you mention all make sense (I assume "-'Else' after
> return." means to remove Else after return).
Yes. The same as removing 'break' in the 'default:' branch of 'switch'.

> I also like seeing them in a
> cleanup bug rather than complicating the review of a fault bug.
Thanks.

Comment 3

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

Comment 4

[Suyash Agarwal (:sshagarwal)]

Sir,

I have tried to make the changes. Please let me know if I am moving in the right direction.

Comment 5

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 749919 [details] [diff] [review]
Patch

I missed a few things in this. Sorry.

Comment 6

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch

Sir,
Sorry for the last patch. I have made the necessary changes, please let me know what needs to be changed in this.

Comment 7

[:aceman]

Comment on attachment 749936 [details] [diff] [review]
Patch

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

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +851,3 @@
>    nsCString dbHdrValue;
>    aHdr->GetStringProperty(m_hdrProperty.get(), getter_Copies(dbHdrValue));
> +  return MatchString(dbHdrValue, nullptr, aResult);

Good.

@@ -870,5 @@
>  NS_IMETHODIMP nsMsgSearchTerm::MatchUint32HdrProperty(nsIMsgDBHdr *aHdr, bool *aResult)
>  {
>    NS_ENSURE_ARG_POINTER(aResult);
>    NS_ENSURE_ARG_POINTER(aHdr);
> -

Sorry, the bug description was meant to keep and add blank line after NS_ENSURE_ARG_POINTER block where missing. Not remove it.

@@ -894,5 @@
>      if (dbHdrValue != m_value.u.msgStatus)
>        result = true;
>      break;
> -  default:
> -    break;

Do not remove this, add rv=NS_ERROR_FAILURE and NS_ERROR("invalid compare op for uint")

@@ -1065,1 @@
>  }

This was meant to be 

if ()
  return x

return y

@@ -1079,5 @@
>    // Save some performance for opIsEmpty / opIsntEmpty
>    if(nsMsgSearchOp::IsEmpty != m_operator && nsMsgSearchOp::IsntEmpty != m_operator)
>    {
> -    NS_ASSERTION(MsgIsUTF8(nsDependentCString(m_value.string)),
> -                 "m_value.string is not UTF-8");

You removed the condition. Revert this back.

The change of NS_ASSERTION to NS_ERROR was only meant in the default branch of switch on nsMsgSearchOp .

@@ -1087,5 @@
>      {
>        ConvertToUnicode(charset, nsCString(stringToMatch), utf16StrToMatch);
>      }
>      else {
> -      NS_ASSERTION(MsgIsUTF8(stringToMatch), "stringToMatch is not UTF-8");

Revert.

@@ +1122,5 @@
>                         nsCaseInsensitiveStringComparator()))
>        result = true;
>      break;
>    default:
> +    NS_ERROR("invalid operator matching search results");

This is good, bud add rv=NS_ERROR_FAILURE and make sure result = false here.

@@ +1141,5 @@
>     NS_ENSURE_ARG_POINTER(pResult);
>     *pResult = false;
>     bool result;
> +   nsresult rv = InitHeaderAddressParser();
> +   NS_ENSURE_SUCCESS(rv, rv);

Good.

@@ -1173,5 @@
>  
>     if (NS_SUCCEEDED(parseErr) && count > 0)
>     {
> -     NS_ASSERTION(names, "couldn't get names");
> -     NS_ASSERTION(addresses, "couldn't get addresses");

Revert.

@@ +1244,5 @@
>          result = true;
>      break;
>    default:
>      rv = NS_ERROR_FAILURE;
> +    NS_ERROR("invalid compare op for dates");

Good.

@@ +1290,5 @@
>            result = true;
>        }
>        break;
>      default:
> +      NS_ERROR("invalid compare op for msg age");

rv = failure and ensure result = false.

@@ -1339,5 @@
>      if (sizeToMatchKB == m_value.u.size)
>        result = true;
>      break;
> -  default:
> -    break;

Add back and add rv=failure and NS_ERROR

@@ -1435,5 @@
>      if (aJunkPercent == m_value.u.junkPercent)
>        result = true;
>      break;
> -  default:
> -    break;

Add back and add rv=failure and NS_ERROR

@@ -1455,5 @@
>      break;
>    default:
>      if (m_value.u.label != aLabelValue)
>        result = true;
> -    break;

Good. But I think this should be converted to case nsMsgSearchOp::Isnt and add default: branch the standard rv= failure and NS_ERROR clauses.

@@ +1987,2 @@
>    }
> +  return rv;

Changing functions other than Match* may be out of scope of this bug, so please do not change any other logic in them than 'rv' variable name.

Comment 8

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Sir,
Thanks for your feedback. I have made the changes you suggested.

Comment 9

[:aceman]

Comment on attachment 750349 [details] [diff] [review]
Patch v2

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

Much better now.

Can you add the last wish from the bug description?
-comment that MatchStatus() not used only for nsMsgMessageFlags but also for nsMsgFolderFlags (as they are both 'unsigned long' for now). And that in this function nsMsgSearchOp::Is and nsMsgSearchOp::Isnt is intentionally used as Contains/DoesntContain for legacy reasons.

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +872,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(aResult);
>    NS_ENSURE_ARG_POINTER(aHdr);
> +  
> +  nsresult rv = NS_OK;

Good.

@@ +1403,3 @@
>    }
>  
>    *pResult = matches;

So what is the value of 'matches' in case we got the operator failure? Ensure we set *pResult to false here. (In case the caller does not check the function return value (rv) for failure.) So probably just set 'matches' to false.

I mentioned this in the bug description:
- some even return true in *pResult (a match) if invalid comparison operation was used (see e.g. http://hg.mozilla.org/comm-central/file/4c366a1789a7/mailnews/base/search/src/nsMsgSearchTerm.cpp#l1473)

@@ +1427,4 @@
>    }
>  
>    *pResult = matches;
>    return rv;

Again, is *pResult false in case of failure?

And I think you need to also fix this problem in nsMsgSearchTerm::MatchStatus

@@ +1476,3 @@
>      if (m_value.u.label != aLabelValue)
>        result = true;
> +  default:

This is what I wanted, but we need to make sure what the "Label" actually is and if what are the allowed operators in the UI. We will need rkent's advice here.

Comment 10

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Sir,

Thanks for the review, I have made the changes.
For line #1476, default value of matches was false (at the time of its definition), so I think, if the control enters the default clause, it doesn't get changed anywhere and matches will be false. So, I haven't reassigned false to it in the default block. If this needs to be changed, please let me know.

Comment 11

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

Sir,

I have made the changes you suggested. Please review this patch and suggest me if this is okay or what needs to be changed.

Comment 12

[:aceman]

Comment on attachment 750670 [details] [diff] [review]
Patch v4

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

Great, this is almost done now. Just small nits.

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +1067,5 @@
>  
>    nsCOMPtr<nsIMimeConverter> mimeConverter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);
>    nsAutoCString stringToMatch;
>    nsresult rv = mimeConverter->DecodeMimeHeaderToUTF8(
>      rfc2047string, charset, charsetOverride, false, stringToMatch);

I now noticed we check this rv nowhere. Please set *pResult = false before this and add NS_ENSURE_SUCCESS(rv, rv) after this.

@@ +1166,5 @@
> +nsresult nsMsgSearchTerm::MatchRfc822String (const char *string,
> +                                             const char *charset,
> +                                             bool charsetOverride,
> +                                             bool *pResult)
> +{

This is good.

@@ +1167,5 @@
> +                                             const char *charset,
> +                                             bool charsetOverride,
> +                                             bool *pResult)
> +{
> +  NS_ENSURE_ARG_POINTER(pResult);

Add blank line after this.

@@ +1174,5 @@
> +  nsresult rv = InitHeaderAddressParser();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  // Isolate the RFC 822 parsing weirdnesses here. MSG_ParseRFC822Addresses
> +  // returns a catenated string of null-terminated strings, which we walk
> +  // across, tring to match the target string to either the name OR the address

Have you reindented the whole function here? I don't think rkent will like that.

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4 (revised)

Sir,

I have made the changes you suggested.
There was a leading extra space in every line of this function, so I removed that extra space from every line. If this isn't accepted, please let me know, I will revert it back.

Comment 14

[:aceman]

Comment on attachment 750704 [details] [diff] [review]
Patch v4 (revised)

Great!

Comment 15

[Kent James (:rkent)]

Comment on attachment 750704 [details] [diff] [review]
Patch v4 (revised)

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

Sorry for the slow review, I've been buried in my own user's issues for awhile.

There are enough changes in this that even though the changes are all small, I should probably see it again before we finish it.

I should also note that there is some small risk of regression here, as we are changing behavior in several locations so that invalid ops are now returning errors. I don;t think that will be an issue though.

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +740,5 @@
>                                                  bool ForFiltering,
>                                                  bool *pResult)
>  {
>    NS_ENSURE_ARG_POINTER(pResult);
> +  

Nit: no space at the beginning of this line.

@@ +847,5 @@
>  NS_IMETHODIMP nsMsgSearchTerm::MatchHdrProperty(nsIMsgDBHdr *aHdr, bool *aResult)
>  {
>    NS_ENSURE_ARG_POINTER(aResult);
>    NS_ENSURE_ARG_POINTER(aHdr);
> +  

Nit: no space

@@ +857,5 @@
>  NS_IMETHODIMP nsMsgSearchTerm::MatchFolderFlag(nsIMsgDBHdr *aMsgToMatch, bool *aResult)
>  {
>    NS_ENSURE_ARG_POINTER(aMsgToMatch);
>    NS_ENSURE_ARG_POINTER(aResult);
> +  

Nit: no space

@@ +870,5 @@
>  NS_IMETHODIMP nsMsgSearchTerm::MatchUint32HdrProperty(nsIMsgDBHdr *aHdr, bool *aResult)
>  {
>    NS_ENSURE_ARG_POINTER(aResult);
>    NS_ENSURE_ARG_POINTER(aHdr);
> +  

Nit: no space

@@ +896,5 @@
>        result = true;
>      break;
>    default:
> +    rv = NS_ERROR_FAILURE;
> +    result = false;

result defaults to false, no need to set it a second time.

@@ +1039,5 @@
> +    switch(m_operator)
> +    {
> +    case nsMsgSearchOp::IsInAB:
> +      if (cardForAddress)
> +       *pResult = true;

Nit: indentation off

@@ +1048,5 @@
> +      break;
> +    default:
> +      rv = NS_ERROR_FAILURE;
> +      *pResult = false;
> +      NS_ERROR ("invalid compare op for address book");

Don't set *pResult = false, it  defaults to false.
Nit: space after ERROR.

@@ +1054,1 @@
>      NS_IF_RELEASE(cardForAddress);

Since you are doing cleanup, could you redefine cardForAddress as nsCOMPtr<nsIAbCard> and eliminate this NS_IF_RELEASE?

@@ +1066,5 @@
>    NS_ENSURE_ARG_POINTER(pResult);
>  
>    nsCOMPtr<nsIMimeConverter> mimeConverter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);
>    nsAutoCString stringToMatch;
> +  *pResult = false;

This is not needed, the called routines should handle.

@@ +1085,5 @@
>                                        const char *charset,
>                                        bool *pResult)
>  {
>    NS_ENSURE_ARG_POINTER(pResult);
> +  

Nit: extra space

@@ +1149,5 @@
>        result = true;
>      break;
>    default:
> +    rv = NS_ERROR_FAILURE;
> +    result = false;

Do not set result, it defaults to false.

@@ +1323,5 @@
>        }
>        break;
>      default:
> +      rv = NS_ERROR_FAILURE;
> +      result = false;

do not set result, it defaults to false.

@@ +1363,5 @@
>        result = true;
>      break;
>    default:
> +    rv = NS_ERROR_FAILURE;
> +    result = false;

Do not set, this is the default

@@ +1490,5 @@
>      if (m_value.u.label != aLabelValue)
>        result = true;
> +  default:
> +    rv = NS_ERROR_FAILURE;
> +    result = false;

Do not set, default value

@@ +1693,5 @@
>      if (p1 != p2)
>        result = true;
>      break;
>    default:
>      result = false;

For consistency, remove this setting to default value

Comment 16

[:aceman]

(In reply to Kent James (:rkent) from comment #15)
>I should also note that there is some small risk of regression here, as we >are changing behavior in several locations so that invalid ops are now >returning errors. I don;t think that will be an issue though.

That is the point here, we want to catch invalid ops instead of producing random results.

> @@ +1066,5 @@
> >    NS_ENSURE_ARG_POINTER(pResult);
> >  
> >    nsCOMPtr<nsIMimeConverter> mimeConverter = do_GetService(NS_MIME_CONVERTER_CONTRACTID);
> >    nsAutoCString stringToMatch;
> > +  *pResult = false;
> 
> This is not needed, the called routines should handle.

This is for the case where we return on NS_ENSURE_SUCCESS(rv, rv);
In that case no other routine is called that would set the result.

All other nits look fine, thanks for catching them.

Comment 17

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5

I have made the changes proposed.

Comment 18

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5

Comment 19

[:aceman]

Comment on attachment 755822 [details] [diff] [review]
Patch v5

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

::: mailnews/base/search/src/nsMsgSearchTerm.cpp
@@ +1030,5 @@
>      return rv;
>  
>    if (mDirectory)
>    {
> +    nsCOMPtr<nsIAbCard> cardForAddress = nullptr;

Not sure we need the nullptr assignment here. But rkent will know better.

@@ +1270,5 @@
>          result = true;
>      break;
>    default:
>      rv = NS_ERROR_FAILURE;
> +    result = false;

Not needed, result is false at the start of the function.

@@ +1459,5 @@
>        result = true;
>      break;
>    default:
> +    rv = NS_ERROR_FAILURE;
> +    result = false;

Not needed, result is false at the start of the function.

@@ +1511,5 @@
>      matches = !matches;
>      break;
>    default:
>      rv = NS_ERROR_FAILURE;
> +    matches = false;

Good, this is needed.

Comment 20

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5 (revised)

Sir,

I have made the changes proposed. But I am still not sure about removing 
*pResult = false near line#1066.

Comment 21

[:aceman]

Comment on attachment 756021 [details] [diff] [review]
Patch v5 (revised)

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

I would leave "*pResult = false near line#1066" in, but rkent must decide.

Comment 22

[Kent James (:rkent)]

Comment on attachment 756021 [details] [diff] [review]
Patch v5 (revised)

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

I suppose I could add a few more nits, but I'm not going to. So let's just call this done.

Comment 23

[Suyash Agarwal (:sshagarwal)]

If there are a few changes to suggest, I don't know why you are not suggesting them.

Comment 24

[Kent James (:rkent)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #23)
> If there are a few changes to suggest, I don't know why you are not
> suggesting them.

Because I think that we expend too much effort on nits as it is.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/25027c73dd53