1270651 - contacts cannot be deleted when they have been found through a search

Status: VERIFIED FIXED

(MailNews Core :: Address Book, defect)

Description

[u547509]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 Iceweasel/38.8.0
Build ID: 20160426225641

Steps to reproduce:

1. Click on "Address Book"
2. Search for a contact(s)
3. Try to delete any contact found by the search


Actual results:

Nothing happens.


Expected results:

The contact should be deleted. Since v45.0, contacts can be deleted only if the search bar is empty.

Comment 1

[Jorg K (CEST = GMT+2)]

Alice, can you get me a regression window here? Thank you in advance.

Comment 2

[Alice0775 White]

Last Good:
20160106030205
https://hg.mozilla.org/mozilla-central/rev/9d6ffc7a08b6b47056eefe1e652710a3849adbf7
https://hg.mozilla.org/comm-central/rev/3f5e39b93615

First Bad:
20160112060958
https://hg.mozilla.org/mozilla-central/rev/e790bba372f14241addda469a4bdb7ab00786ab3
https://hg.mozilla.org/comm-central/rev/1eb99f6f98f7

Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9d6ffc7a08b6b47056eefe1e652710a3849adbf7&tochange=e790bba372f14241addda469a4bdb7ab00786ab3
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=3f5e39b93615&tochange=1eb99f6f98f7

* The win32/64 builds(2016-JAN-07 to 11) are broken(nothing is shown)
  If someone has MAC , try these build to narrow the range.

Comment 3

[Alice0775 White]

Errors show in Error Console after click [OK] button in Confirm dialog to datale.

Timestamp: 2016/05/06 17:07:37
Error: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIAbView.deleteSelectedCards]
Source File: chrome://messenger/content/addressbook/abCommon.js
Line: 299

Timestamp: 2016/05/06 17:07:37
Error: An error occurred executing the button_delete command: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIAbView.deleteSelectedCards]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: chrome://messenger/content/addressbook/abCommon.js :: AbDelete :: line 299"  data: no]
Source File: chrome://global/content/globalOverlay.js
Line: 103

Comment 4

[Alice0775 White]

Via local build,
Last Good : c-c 625d871a9669 m-c ea04fff1bfd4
First Bad : c-c 4f4f6a3674c9 m-c ea04fff1bfd4

Regressed by: 4f4f6a3674c9	Thomas Düllmann — Bug 118624 - Implement customizable prefs: mail.addr_book.quicksearchquery.format, autocompletequery.format; add Nickname & other fields to AB quick search, contacts side bar and SM's Select Addresses dialogue. r=aceman, rs=rkent, a=rkent

Comment 5

[Kent James (:rkent)]

I'd like to use tracking + as blocking next release, and ? as watch. So far this is inconsistent, but I'd like to move that way.

Comment 6

[:aceman]

I'm looking at this for a while now, but can't see how 118624 could have caused this.

Alice, how was the regression range determined and how reliable is it?

The failure is happening deep in AB C++ code, the GetDatabase is failing.
I can't see yet how the frontend only bug 118624 could have caused it. Maybe if the query string it produced would be invalid, but I see no hint in that direction.

[27728] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /var/SSD/TB-hg/mailnews/addrbook/src/nsAbMDBDirectory.cpp, line 329
[27728] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /var/SSD/TB-hg/mailnews/addrbook/src/nsAbMDBDirectory.cpp, line 354
[27728] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /var/SSD/TB-hg/mailnews/addrbook/src/nsAbMDBDirectory.cpp, line 431
[27728] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xC1F30001: file /var/SSD/TB-hg/mailnews/addrbook/src/nsAbView.cpp, line 1208
JavaScript error: chrome://messenger/content/addressbook/abCommon.js, line 299: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIAbView.
deleteSelectedCards]

Comment 7

[Jorg K (CEST = GMT+2)]

Usually Alice is 100% spot on ;-) (and it's a fantastic service that one can't praise highly enough!)
However, at times knowing the regression doesn't help much in the fix, so it's better to tackle the problem with forward debugging instead of looking back.

What's running behind the OK button on the confirm panel and how is that different to what's running when you just delete a contact from the list without prior searching (which works)?

Comment 8

[:aceman]

I just asked about the process ;)
I backed out bug 118624 locally and it made the problem go away.
So we have a starting point to trace this mystery from.
Thanks Alice!

This also affects Seamonkey.

Comment 9

[:aceman]

Wow, I've found the problem. It was a backend bug triggered by the specific characters found in the new search query string used in 118624 :) So no bug in 118624.
Need to strip the patch of debugging and will attach tomorrow.

Comment 10

[:aceman]

Attachment: patch

The patch contains these changes:
1. The full AB URI, when a search is run, that is passed to nsAbMDBDirectory::Init looks like this:

moz-abmdbdirectory://abook-1.mab?(and(or(DisplayName,c,test)(FirstName,c,test)(LastName,c,test)(NickName,c,test)(PrimaryEmail,c,test)(SecondEmail,c,test)(and(IsMailList,=,TRUE)
(Notes,c,test))(Company,c,test)(Department,c,test)(JobTitle,c,test)))

The .Find("MailList") runs on this full string (including the query part after the ? character). It is incorrect that it finds it in the "IsMailList" substring thus mistakenly deciding we are running on a mailinglist, whiel we are not (we run on a normal addressbook). So the fix is to only search in the part before ?.

2. The "??" case is similar. We only want to see if the "??" are the first characters in the searchquery. That is evidenced by the subsequent Substring(searchQuery, 1) command (we strip the first character, regardless of where the "??" was found). But if you were searching for a string containing ??, the "??" sequence could be find in the search query in other places than just the beginning.


Try run: https://hg.mozilla.org/try-comm-central/rev/dd27cf8bed18bec588d57613324f65642c286cb2

Comment 11

[:aceman]

Forgot to note that if we really were in a maillinglist, the URI would look like this:
moz-abmdbdirectory://abook.mab/MailList7?query so I think the code is looking for the "MailList" before the ?. Also, searching inside a mailinglist and the trying to delete cards still does not work, but that is bug 465535.

Comment 12

[:aceman]

Attachment: patch v1.1

Optimized version.

Comment 13

[Kent James (:rkent)]

Comment on attachment 8750179 [details] [diff] [review]
patch v1.1

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

Generally, I think there are simpler ways to fix the issue - and I am objecting to unnecessary cleanup changes in a patch I would like to uplift. But thanks for looking into this, the approach you are trying to do is fine and I would like to see it landed in a simpler version.

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +52,5 @@
>  {
>    // We need to ensure  that the m_DirPrefId is initialized properly
>    nsDependentCString uri(aUri);
>  
> +  // Find the first ? (of the search params) if there is one.

Yes you are just copying code from this ancient method, but really we should not be trying to parse the URL our self like this. It is very fragile, as this bug itself shows.

If you want to parse the URI, use an NS_STANDARD_URL and extract the path. But really I thing that the simpler answer is this: the code that creates MailLists (nsAbMDBDirectory::AddMailList) does this:

listUri.AppendLiteral("/MailList");

So can't you change the simple URI search to:

if (uri.Find("/MailList") != -1)

and accomplish what you want? That would be a much simpler change.

@@ +63,5 @@
> +    URINoQuery.SetLength(searchCharLocation);
> +  }
> +
> +  // In the non-query part of the URI, check if we are a mailinglist
> +  if (URINoQuery.Find("MailList") != kNotFound)

I think that this move will not be needed if you use my simpler approach.

@@ +73,4 @@
>      nsAutoCString filename;
>  
>      // extract the filename from the uri.
> +    filename = Substring(URINoQuery, kMDBDirectoryRootLen);

If you must parse the filename, again use NS_STANDARD_URL.  But again I would leave this alone and just solve the immediate problem.

@@ +327,5 @@
>  {
>    NS_ENSURE_ARG_POINTER(aResult);
>  
>    nsCString fileName;
> +  nsresult rv = GetFileName(fileName);

This kind of cleanup change does not belong in a patch that is trying to fix a regression and that we hope to upload to release branches. Please remove, and only include what you need for the actual fix.

@@ +445,5 @@
>  
>      rv = directory->DeleteCards(aCards);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    return database->RemoveListener(this);

What is the point of this change? It removes a warning. This is not part of the regression, please remove unless you can justify why it is needed here.

@@ +977,5 @@
>  
>      int32_t pos = parentURI.RFindChar('/');
>  
>      // If we didn't find a / something really bad has happened
> +    if (pos == kNotFound)

Again I prefer that you overload a simple regression fix with lots of cleanup changes. Please remove unless this is related to the regression.

::: mailnews/addrbook/src/nsAbView.cpp
@@ +204,5 @@
>      // 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("??") == 0)

Notice how I now I have to figure out if this is an important change, or an attempt at cleanup, as it is very similar to a previous cleanup change. That's why I hate when cleanup issues are combined with important regression changes.

Because you added a comment specifically on this issue, I assume it is an important change.

I find it very odd that we are using a double "??" in a query with a special meaning. But that issue predates this bug. I can't think of any cases where this "??" would occur in a valid query string, but the change that you are suggesting seems harmless so I will not protest. Is there a valid case where this fixes a real issue?

Comment 14

[:aceman]

(In reply to Kent James (:rkent) from comment #13)
> Yes you are just copying code from this ancient method, but really we should
> not be trying to parse the URL our self like this. It is very fragile, as
> this bug itself shows.
> 
> If you want to parse the URI, use an NS_STANDARD_URL and extract the path.

What is NS_STANDARD_URL? No occurrences of it in the tree: http://mxr.mozilla.org/comm-central/search?string=NS_STANDARD_URL&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=comm-central .

> But really I thing that the simpler answer is this: the code that creates
> MailLists (nsAbMDBDirectory::AddMailList) does this:
> 
> listUri.AppendLiteral("/MailList");
> 
> So can't you change the simple URI search to:
> 
> if (uri.Find("/MailList") != -1)
> 
> and accomplish what you want? That would be a much simpler change.

No, what if the user inputs /MailList as the search word? Do you think the / will be escaped in the query string?

Why are you still wanting to guess what the searchquery can/can not contain instead of just cutting it away?
This kind of approach caused the bug in the first place.

> @@ +327,5 @@
> >  {
> >    NS_ENSURE_ARG_POINTER(aResult);
> >  
> >    nsCString fileName;
> > +  nsresult rv = GetFileName(fileName);
> 
> This kind of cleanup change does not belong in a patch that is trying to fix
> a regression and that we hope to upload to release branches. Please remove,
> and only include what you need for the actual fix.

Well, in the comment above you suggest to use some NS_STANDARD_URL (probably some helper function) and here you object to using an existing helper instead of open-coding some call. Yes, it does not belong here to fix any regression. But then, there will probably be no bug where it belongs to.

> 
> @@ +445,5 @@
> >  
> >      rv = directory->DeleteCards(aCards);
> >      NS_ENSURE_SUCCESS(rv, rv);
> >  
> > +    return database->RemoveListener(this);
> 
> What is the point of this change? It removes a warning. This is not part of
> the regression, please remove unless you can justify why it is needed here.

No, it is not needed. I've never seen a construct like this to intentionally emit a warning. If it is intentional, it should probably be commented, because it seems we use NS_ENSURE and just returning an error code directly (without a warning) randomly. Also, we always fold constructs like "rv = fn(); return rv;" into "return fn()".
But OK, I can remove it. And let it to be forgotten.

> @@ +977,5 @@
> >      // If we didn't find a / something really bad has happened
> > +    if (pos == kNotFound)
> 
> Again I prefer that you overload a simple regression fix with lots of
> cleanup changes. Please remove unless this is related to the regression.

This one I can agree with :)

> ::: mailnews/addrbook/src/nsAbView.cpp
> @@ +204,5 @@
> >      // 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("??") == 0)
> 
> Notice how I now I have to figure out if this is an important change, or an
> attempt at cleanup, as it is very similar to a previous cleanup change.
> That's why I hate when cleanup issues are combined with important regression
> changes.

This is not a clean up. It fixes a semantically incorrect code. The original code of:
    if (searchQuery.Find("??") != kNotFound)
       searchQuery = Substring(searchQuery, 1);

is not correct in my opinion. Finding ?? anywhere in the query, then cutting off the first char? What?

> Because you added a comment specifically on this issue, I assume it is an
> important change.
> 
> I find it very odd that we are using a double "??" in a query with a special
> meaning. But that issue predates this bug. I can't think of any cases where
> this "??" would occur in a valid query string, but the change that you are
> suggesting seems harmless so I will not protest. Is there a valid case where
> this fixes a real issue?

Tried searching for ?? in the addressbook? I didn't yet, but I am not sure the ?? would be escaped.
I just think the code has the same bug as the main one (maillist) being fixed in this bug. We MAY find something in the middle of the query while we only intended to find it at the start of it.

But I am fine with splitting this change into a new bug. This code was added in bug 170270 so I can talk to Suyash about the intention of that line.

Comment 15

[Kent James (:rkent)]

Sorry, try searching NS_STANDARDURL.

Looking at the query limitations some more, the query escaping is not well defined, so there is no "right" way to do it. 

RFC 3986 states: "The query component is indicated by the first question mark ("?") character" and "The characters slash ("/") and question mark ("?") may represent data within the query component ... it is sometimes better for usability to avoid percent-encoding those characters.

Given that, I'll agree that the safer thing is to not make any assumptions about / and ? encoding, that is to do what you did in your patch. But can you do a patch where you don't roll your own decoding, but use NS_STANDARDURL to do the decoding when reasonable?

The code I find particularly objectionable is this:

+  int32_t searchCharLocation = uri.FindChar('?', kMDBDirectoryRootLen);
+  nsAutoCString URINoQuery(uri);
+  if (searchCharLocation != kNotFound)
+  {
+    URINoQuery.SetLength(searchCharLocation);
+  }
+

with the saved kMDBDirectoryRootLen and reset of length of URINoQuery. This code is just parsing for the query, and there is existing reliable code to do that which should be used (NS_STANDARDURL)

Comment 16

[:aceman]

Comment on attachment 8750179 [details] [diff] [review]
patch v1.1

So pulling in a new framework that is not used anywhere else in the AB code and may have slightly different parsing in edge cases is less risky for the 45 branch than just moving existing patterns around?

Let's try it.

Comment 17

[:aceman]

I experimented with using the nsIURI its parser on the AB string
The uris look like moz-abmdbdirectory://abook-1.mab?query.
Then uri.GetHost() returns empty, uri.GetPath() returns "//abook-1.mab". There is actually no 'host' component of the URI so it is not sure if we can depend on this parsing. See http://mxr.mozilla.org/comm-central/source/mozilla/netwerk/base/nsIURI.idl#16 .

Comment 18

[:aceman]

Attachment: patch v1.2

So this is the reduced patch.

Comment 19

[Kent James (:rkent)]

Comment on attachment 8750947 [details] [diff] [review]
patch v1.2

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

Thanks for simplifying, much easier to review.

Apart from the glaring problem, the patch is fine. r+ if you agree with my analysis and add a default setting for URINoQUery. (It does not have to be done with the else, but that seemed to match the style you were using).

::: mailnews/addrbook/src/nsAbMDBDirectory.cpp
@@ +60,5 @@
> +  nsAutoCString URINoQuery;
> +  if (searchCharLocation != kNotFound)
> +  {
> +    URINoQuery = Substring(uri, 0, searchCharLocation);
> +  }

What happens if you Init a mailing list with no query? URINoQuery is blank, right, so the Find of "MailList" will fail.

I think that you need an else clause here, assigning uri to URINoQuery in that case.

else
{
  URINoQuery.Assign(uri);
}

@@ +73,4 @@
>      nsAutoCString filename;
>  
> +    // Extract the filename from the uri.
> +    filename = Substring(URINoQuery, kMDBDirectoryRootLen);

With blank URINoQuery, fliename is also blank, and it is game over. Fixing that solves this.

Comment 20

[:aceman]

Attachment: patch v1.3

Yes, thanks for catching that. The previous version was initializing URINoQuery to uri, but you seemed to not like the shortening of it with .SetLength(). And the rewrite wasn't successful.

Comment 21

[:aceman]

https://hg.mozilla.org/comm-central/rev/9b747672e00d707968dbf3e051ffd7d748d3efcd

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8751957 [details] [diff] [review]
patch v1.3

[Triage Comment]

[Approval Request Comment]
Regression caused by (bug #): bug 118624 exposed pre-existing problem.
User impact if declined: can't delete addresses from address book, not nice.
Testing completed (on c-c, etc.): Manual testing.
Risk to taking this patch (and alternatives if risky):
I suppose not so risky, small patch, Kent reviewed ;-)

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8751957 [details] [diff] [review]
patch v1.3

Flipping the flag.

Comment 24

[Jorg K (CEST = GMT+2)]

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

Comment 25

[Kent James (:rkent)]

Comment on attachment 8751957 [details] [diff] [review]
patch v1.3

http://hg.mozilla.org/releases/comm-beta/rev/07e540ffe86f
http://hg.mozilla.org/releases/comm-esr45/rev/d151630abae7

Comment 26

[Rainer Bielefeld]

As per comment 8 REPRODUCIBLE with  English SeaMonkey 2.45a1  (Windows NT 6.1; WOW64; rv:48.0)  Gecko/20100101 Firefox/48.0 Build 20160308001946  (Default Classic Theme)  on German WIN7 64bit.

I will check fix for SM 2.46 when available

Comment 29

[Rainer Bielefeld]

This problem indeed started after SeaMonkey 2.41a1 Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:44.0 from akalla download area)  Gecko/20100101  Firefox/ 44.0  Build 20150926114543,  (Classic Theme) on German WIN7 64bit

No longer reproducible with  English SeaMonkey (unofficial by frg) 2.47a1  (NT 6.1; Win64; x64; rv:50.0)  Gecko/20100101 Firefox/50.0 Build 20160628114904  (Default Classic Theme)  on German WIN7 64bit, no unwanted side effects observed until now, so VERIFIED