1271733 - problem separating query string when searching in All Addressbooks

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[:aceman]

In the adressbook code in the file mailnews/addrbook/src/nsAbView.cpp in function nsAbView::SetView(), there is code like this:
  aAddressBook->GetURI(uri);
  int32_t searchBegin = uri.FindChar('?');
  nsCString searchQuery(Substring(uri, searchBegin));
  // This is a special case, a workaround basically, to just have all ABs.
  if (searchQuery.EqualsLiteral("?")) {
    searchQuery.AssignLiteral("");
  }

  if (Substring(uri, 0, searchBegin).EqualsLiteral(kAllDirectoryRoot)) {
    mIsAllDirectoryRootView = true;
    // We have special request case to search all addressbooks, so we need
    // to iterate over all addressbooks.
    // Since the request is for all addressbooks, the URI must have been
    // passed with an extra '?'. We still check it for sanity and trim it here.
    if (searchQuery.Find("??") != kNotFound)
      searchQuery = Substring(searchQuery, 1);

I assume this code is looking for Ab strings that look like this:
moz-abdirectory://??queryString

In these last lines, the searchQuery variable contains "??queryString". We search for "??" in the whole query. If the sequence is found anywhere we cut off the first char of string.

I think this is a logic bug. If we only want to cut off the first char, to get a proper query with single question mark, why do we not search for the ?? sequence only at the beginning of the string? The ?? sequence could appear also anywhere in the queryString (the user could search for it) and we would get false matches.

My proposal is to change the check to:
if (searchQuery.Find("??") != 0)

This code was introduced in bug 170270.

Comment 1

[:aceman]

Suyash, can you look at what this code was intended to do?

Comment 2

[Jorg K (CEST = GMT+2)]

How is this related to bug 1270651 which has the exact same summary?

Comment 3

[:aceman]

The problem was found when working on bug 1270651 and the summary got copied by cloning the bug. Thanks for noticing:)

Comment 4

[Suyash Agarwal (:sshagarwal)]

Hi,
So far, I am unable to reproduce an issue because of this check. But, yes, so far the purpose of this code seems to be to look for "??" in the beginning.
I'll get into the details just to know why I did this but, it seems your suggestion for replacing this contains with beginsWith is fine :)

Thanks.

Comment 5

[:aceman]

Attachment: patch

OK, thanks.

Comment 6

[Kent James (:rkent)]

Comment on attachment 8757637 [details] [diff] [review]
patch

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

I didn't try to build and test with this, but I agree that the code as written has an error, and this seems to be an improvement. So let's go with it.

Comment 7

[:aceman]

Thanks.

Comment 8

[Jorg K (CEST = GMT+2)]

Sorry, missed that when preparing a push.

Comment 9

[:aceman]

https://hg.mozilla.org/comm-central/rev/2ea521366c72695ab72b22391f3030b855708f03

Comment 10

[Kent James (:rkent)]

I assume this is something that we would uplift?

Comment 11

[:aceman]

As you wish, but nobody could find a real world scenario where the bug would manifest itself. So maybe it is not urgent.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8757637 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): Bug 170270

Did someone say uplift ;-)
I guess Kent knows about the risk of this one.

How was the testing done?

Comment 13

[Jorg K (CEST = GMT+2)]

Aurora (TB 48):
https://hg.mozilla.org/releases/comm-aurora/rev/12128f286d4d

Comment 14

[:aceman]

(In reply to Jorg K (PTO during summer, NI me) from comment #12)
> Did someone say uplift ;-)
> I guess Kent knows about the risk of this one.
> How was the testing done?
Testing was only that searching in All ABs still works after the patch. We have no specific steps to make this error happen.

You would need to pass a string like (searching for a ?? string):
moz-abdirectory://?(or(FirstName,c,??))

which is hard to do, as the good behaving frontend should pass in:
moz-abdirectory://??(or(FirstName,c,??))

So always the first ?? should be found.

So this bug seems theoretical, just that the condition was not matching the subsequent action. But there are other safeguards so that the condition would not report a false positive.

As such this bug is low severity (maybe even none) and does not need to be backported to 45 unless a real testcase is found. It is safer to let it bake on trunk.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8757637 [details] [diff] [review]
patch

Clearing beta and esr uplift requests as per previous comment.