problem separating query string when searching in All Addressbooks

RESOLVED FIXED in Thunderbird 49.0

Status

defect
--
minor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 49.0
Dependency tree / graph

Thunderbird Tracking Flags

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

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
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.
Assignee

Comment 1

3 years ago
Suyash, can you look at what this code was intended to do?
Flags: needinfo?(syshagarwal)

Comment 2

3 years ago
How is this related to bug 1270651 which has the exact same summary?
Assignee

Comment 3

3 years ago
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
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.
Flags: needinfo?(syshagarwal)
Assignee

Comment 5

3 years ago
Posted patch patchSplinter Review
OK, thanks.
Attachment #8757637 - Flags: review?(rkent)
Assignee

Updated

3 years ago
Status: NEW → ASSIGNED
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.
Attachment #8757637 - Flags: review?(rkent) → review+
Assignee

Comment 7

3 years ago
Thanks.
Keywords: checkin-needed

Comment 8

3 years ago
Sorry, missed that when preparing a push.
Assignee

Comment 9

3 years ago
https://hg.mozilla.org/comm-central/rev/2ea521366c72695ab72b22391f3030b855708f03
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 49.0
I assume this is something that we would uplift?
Assignee

Comment 11

3 years ago
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

3 years ago
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?
Attachment #8757637 - Flags: approval-comm-esr45?
Attachment #8757637 - Flags: approval-comm-beta?
Attachment #8757637 - Flags: approval-comm-aurora+
Assignee

Comment 14

3 years ago
(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

3 years ago
Comment on attachment 8757637 [details] [diff] [review]
patch

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.