Last Comment Bug 870236 - Consolidate return value behaviour of nsMsgSearchTerm::Match* functions
: Consolidate return value behaviour of nsMsgSearchTerm::Match* functions
Status: RESOLVED FIXED
[good first bug][mentor=aceman][lang=...
:
Product: MailNews Core
Classification: Components
Component: Search (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 24.0
Assigned To: Suyash Agarwal (:sshagarwal)
:
:
Mentors:
Depends on:
Blocks: 218853
  Show dependency treegraph
 
Reported: 2013-05-09 00:37 PDT by :aceman
Modified: 2013-06-25 05:20 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (21.39 KB, patch)
2013-05-15 08:52 PDT, Suyash Agarwal (:sshagarwal)
no flags Details | Diff | Splinter Review
Patch (21.17 KB, patch)
2013-05-15 09:39 PDT, Suyash Agarwal (:sshagarwal)
no flags Details | Diff | Splinter Review
Patch v2 (21.71 KB, patch)
2013-05-16 02:10 PDT, Suyash Agarwal (:sshagarwal)
no flags Details | Diff | Splinter Review
Patch v3 (22.28 KB, patch)
2013-05-16 08:47 PDT, Suyash Agarwal (:sshagarwal)
no flags Details | Diff | Splinter Review
Patch v4 (26.42 KB, patch)
2013-05-16 13:48 PDT, Suyash Agarwal (:sshagarwal)
acelists: feedback+
Details | Diff | Splinter Review
Patch v4 (revised) (26.63 KB, patch)
2013-05-16 14:25 PDT, Suyash Agarwal (:sshagarwal)
rkent: review-
acelists: feedback+
Details | Diff | Splinter Review
Patch v5 (26.75 KB, patch)
2013-05-30 00:29 PDT, Suyash Agarwal (:sshagarwal)
no flags Details | Diff | Splinter Review
Patch v5 (26.76 KB, patch)
2013-05-30 00:34 PDT, Suyash Agarwal (:sshagarwal)
acelists: feedback+
Details | Diff | Splinter Review
Patch v5 (revised) (27.94 KB, patch)
2013-05-30 09:17 PDT, Suyash Agarwal (:sshagarwal)
rkent: review+
acelists: feedback+
Details | Diff | Splinter Review

Description :aceman 2013-05-09 00:37:48 PDT
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) 2013-05-09 16:26:31 PDT
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 2013-05-09 23:44:56 PDT
(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) 2013-05-15 08:52:47 PDT
Created attachment 749919 [details] [diff] [review]
Patch
Comment 4 Suyash Agarwal (:sshagarwal) 2013-05-15 08:53:39 PDT
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) 2013-05-15 09:10:21 PDT
Comment on attachment 749919 [details] [diff] [review]
Patch

I missed a few things in this. Sorry.
Comment 6 Suyash Agarwal (:sshagarwal) 2013-05-15 09:39:16 PDT
Created attachment 749936 [details] [diff] [review]
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 2013-05-16 00:13:06 PDT
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) 2013-05-16 02:10:37 PDT
Created attachment 750349 [details] [diff] [review]
Patch v2

Sir,
Thanks for your feedback. I have made the changes you suggested.
Comment 9 :aceman 2013-05-16 07:45:59 PDT
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) 2013-05-16 08:47:31 PDT
Created attachment 750473 [details] [diff] [review]
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) 2013-05-16 13:48:14 PDT
Created attachment 750670 [details] [diff] [review]
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 2013-05-16 14:08:24 PDT
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) 2013-05-16 14:25:54 PDT
Created attachment 750704 [details] [diff] [review]
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 2013-05-16 14:32:14 PDT
Comment on attachment 750704 [details] [diff] [review]
Patch v4 (revised)

Great!
Comment 15 Kent James (:rkent) 2013-05-29 16:44:28 PDT
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 2013-05-30 00:00:58 PDT
(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) 2013-05-30 00:29:55 PDT
Created attachment 755820 [details] [diff] [review]
Patch v5

I have made the changes proposed.
Comment 18 Suyash Agarwal (:sshagarwal) 2013-05-30 00:34:16 PDT
Created attachment 755822 [details] [diff] [review]
Patch v5
Comment 19 :aceman 2013-05-30 01:03:47 PDT
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) 2013-05-30 09:17:35 PDT
Created attachment 756021 [details] [diff] [review]
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 2013-05-30 10:12:20 PDT
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) 2013-06-18 15:55:52 PDT
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) 2013-06-18 23:09:59 PDT
If there are a few changes to suggest, I don't know why you are not suggesting them.
Comment 24 Kent James (:rkent) 2013-06-20 08:59:52 PDT
(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] 2013-06-20 13:50:19 PDT
https://hg.mozilla.org/comm-central/rev/25027c73dd53

Note You need to log in before you can comment on or make changes to this bug.