Closed Bug 1271733 Opened 8 years ago Closed 7 years ago

problem separating query string when searching in All Addressbooks


(MailNews Core :: Address Book, defect)

Not set


(thunderbird46 wontfix, thunderbird47 wontfix, thunderbird48 fixed, thunderbird49 fixed, thunderbird_esr45 wontfix)

Thunderbird 49.0
Tracking Status
thunderbird46 --- wontfix
thunderbird47 --- wontfix
thunderbird48 --- fixed
thunderbird49 --- fixed
thunderbird_esr45 --- wontfix


(Reporter: aceman, Assigned: aceman)




(1 file)

In the adressbook code in the file mailnews/addrbook/src/nsAbView.cpp in function nsAbView::SetView(), there is code like this:
  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("?")) {

  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:

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.
Suyash, can you look at what this code was intended to do?
Flags: needinfo?(syshagarwal)
How is this related to bug 1270651 which has the exact same summary?
The problem was found when working on bug 1270651 and the summary got copied by cloning the bug. Thanks for noticing:)
Summary: contacts cannot be deleted when they have been found through a search → problem separating query string when searching in All Addressbooks
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 :)

Flags: needinfo?(syshagarwal)
Attached patch patchSplinter Review
OK, thanks.
Attachment #8757637 - Flags: review?(rkent)
Comment on attachment 8757637 [details] [diff] [review]

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.
Attachment #8757637 - Flags: review?(rkent) → review+
Keywords: checkin-needed
Sorry, missed that when preparing a push.
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 49.0
I assume this is something that we would uplift?
As you wish, but nobody could find a real world scenario where the bug would manifest itself. So maybe it is not urgent.
Comment on attachment 8757637 [details] [diff] [review]

[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?
Attachment #8757637 - Flags: approval-comm-esr45?
Attachment #8757637 - Flags: approval-comm-beta?
Attachment #8757637 - Flags: approval-comm-aurora+
(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):

which is hard to do, as the good behaving frontend should pass in:

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 on attachment 8757637 [details] [diff] [review]

Clearing beta and esr uplift requests as per previous comment.
Attachment #8757637 - Flags: approval-comm-esr45?
Attachment #8757637 - Flags: approval-comm-beta?
You need to log in before you can comment on or make changes to this bug.