Closed Bug 745664 Opened 12 years ago Closed 9 years ago

Rename Address book aaa to aaa_test, delete another address book bbb, and renamed address book aaa_test will lose its name and appear deleted after restart (dataloss! involving localized names)

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
critical

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed, thunderbird_esr31 affected)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed
thunderbird_esr31 --- affected

People

(Reporter: hwaki, Assigned: aceman)

References

Details

(Keywords: dataloss, regression)

Attachments

(7 files, 6 obsolete files)

Attached image Add two address books
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120310010446

Steps to reproduce:

1) Create two address books.
(Ex. aaa and bbb)
-> see attached Address01.png
2) Rename aaa to aaa_test.
-> see attached Address02.png
3) Delete bbb
-> see attached Address03.png


Actual results:

Address book aaa_test had its name lost. 
-> see attached Address04.png

Moreover,
Address book aaa_test will be deleted after TB restart.
-> see attached Address05.png


Expected results:

It should not change the address book name aaa_test to something at this timing.
Attached image Rename aaa to aaa_test
Attached image Delete bbb
Attached image Set no name to aaa_test
Attachment #615226 - Attachment description: Address01.png → Add two address books
I can reproduce since Thunderbird5.
http://hg.mozilla.org/releases/comm-miramar/rev/db3f28cc583a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0) Gecko/20110624 Thunderbird/5.0 ID:20110624125636


Regression window(Thunderbird)
Good:
http://hg.mozilla.org/comm-central/rev/29fd2b00d1ad
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0a2) Gecko/20110506 Thunderbird/3.3a4pre ID:20110506030011
Bad:
http://hg.mozilla.org/comm-central/rev/11e51879e4f7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110510 Thunderbird/3.4a1pre ID:20110510143446
Pushlog:
http://hg.mozilla.org/comm-central/pushloghtml?fromchange=29fd2b00d1ad&tochange=11e51879e4f7
Suspected:
6e2ec369d646	Mark Banner — Fix mozmill bustage from bug 422845
d5ac0eb39141	Joey Minta — Bug 422845 - Replace rdf-driven addressbook directory tree with js one; Patch originally by Joey Minta, updated and completed by Mike Conley. r=Standard8


Regression window(SeaMonkey)
Good:
http://hg.mozilla.org/comm-central/rev/e307f0c7a8f1
http://hg.mozilla.org/mozilla-central/rev/2f509e44172b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110703 Firefox/7.0a1 SeaMonkey/2.4a1
No Address book
http://hg.mozilla.org/comm-central/rev/ae84f8b74c90
http://hg.mozilla.org/mozilla-central/rev/26cce0d3e103
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110705 Firefox/7.0a1 SeaMonkey/2.4a1
Suspected:
e3f147cb26aa	Mike Conley — Bug 652855 - De-RDF the address book; r=standard8



Regression window(SeaMonkey)
No Address book
http://hg.mozilla.org/comm-central/rev/946b956a9bad
http://hg.mozilla.org/mozilla-central/rev/ad1655c2e5b1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110724 Firefox/8.0a1 SeaMonkey/2.5a1
Bad
http://hg.mozilla.org/comm-central/rev/1252c3a5daed
http://hg.mozilla.org/mozilla-central/rev/c53ee19aeffa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110726 Firefox/8.0a1 SeaMonkey/2.5a1
Suspected:
3b77cabb3de9	Jens Hatlak — Bug 654864 - Suite changes from |Bug 422845 - Replace rdf-driven addressbook directory tree with js one|. r=Mnyromyr sr=Neil
Blocks: 422845
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Linux → All
Version: 11 → Trunk
Confirming on TB14.
The renaming step of the first AB is required.
No errors in Error console.
If after the rename TB is restarted the problems does not happen.

I only get this in error console, but that happens even when the first AB is not broken (i.e. in the restart case):
Error: An error occurred updating the cmd_delete command: [Exception... "'[JavaScript Error: "cards[i] is null" {file: "chrome://messenger/content/addressbook/abResultsPane.js" line: 156}]' when calling method: [nsIController::isCommandEnabled]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/globalOverlay.js :: goUpdateCommand :: line 80"  data: yes]
Source File: chrome://global/content/globalOverlay.js
Line: 86
Attached patch Possible fix (obsolete) — Splinter Review
The description of address book is stored as localized string but it is overwritten by normal string while renaming the address book.

Tomorrow I will write a unit test for this issue.
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
Attachment #615303 - Flags: review?(mbanner)
Comment on attachment 615303 [details] [diff] [review]
Possible fix

I think mconley is the addressbook guy, so let's get him to look at it too. And it may get to it sooner than standard8 :)
Attachment #615303 - Flags: feedback?(mconley)
Attached patch The test (obsolete) — Splinter Review
Attachment #615532 - Flags: review?(mconley)
Comment on attachment 615303 [details] [diff] [review]
Possible fix

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

This looks like the right idea to me, and upon testing, fixes the problem highlighted by the STR.
Attachment #615303 - Flags: feedback?(mconley) → feedback+
Comment on attachment 615532 [details] [diff] [review]
The test

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

Hey Hiro,

Great job!  Awesome test.

Just two nits.  With those fixed, r=me.

-Mike

::: mailnews/addrbook/test/unit/test_nsAbManager3.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * Test that directory name is correctly renamed after another directory is removed.

I think this is more accurate:

Tests that an address book, once renamed, is not deleted when a sibling address book is deleted.

@@ +75,5 @@
> +
> +  let newDirectory1 = addDirectory("testAb1");
> +  let newDirectory2 = addDirectory("testAb2");
> +
> +  renameDirectory(newDirectory1, "newTestAb1");

To avoid repetition, let's set newTestAb1 as a const.
Attachment #615532 - Flags: review?(mconley) → review+
Comment on attachment 615303 [details] [diff] [review]
Possible fix

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

::: mailnews/addrbook/src/nsDirPrefs.cpp
@@ +167,2 @@
>        // Ensure the local copy of the description is kept up to date.
> +      server->description = DIR_GetLocalizedStringPref(prefname);

Shouldn't this be DIR_GetDescription?
Attachment #615303 - Flags: review?(mbanner) → review-
(In reply to Mark Banner (:standard8) from comment #12)
> Comment on attachment 615303 [details] [diff] [review]
> Possible fix
> 
> Review of attachment 615303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/addrbook/src/nsDirPrefs.cpp
> @@ +167,2 @@
> >        // Ensure the local copy of the description is kept up to date.
> > +      server->description = DIR_GetLocalizedStringPref(prefname);
> 
> Shouldn't this be DIR_GetDescription?

Yes. The prefName has ".description" there.
Whiteboard: [patchlove]
Wow! So we delete random address books when they happen to have been renamed from localized names. Dataloss at its finest.

Patch seems to be almost ready, r+ from comment 11, but r- with 1 open issue in comment 12, and patch author's reply in comment 13.
This should be fixed asap by someone who has the time (current assignee might be busy).
Severity: normal → critical
Keywords: dataloss
Summary: Delete a address book, another address book was lost the name → Rename Address book aaa to aaa_test, delete another address book bbb, and renamed address book aaa_test will lose its name and be deleted (dataloss! involving localized names)
Assignee: hiikezoe → nobody
Status: ASSIGNED → NEW
As I can reproduce it, I can try to finish the patch.

Wayne, is there anything we can do to prevent stuff like this? I mean to leave a bug marked critical+dataloss with a patch (with positive reviews) left rotting in bugzilla? :)
Assignee: nobody → acelists
Status: NEW → ASSIGNED
rkent, fyi -> comment 14, comment 15

(In reply to Thomas D. from comment #14)
> Wow! So we delete random address books when they happen to have been renamed
> from localized names. Dataloss at its finest.
> 
> Patch seems to be almost ready, r+ from comment 11, but r- with 1 open issue
> in comment 12, and patch author's reply in comment 13.
> This should be fixed asap by someone who has the time (current assignee
> might be busy).

(In reply to :aceman from comment #15)
> As I can reproduce it, I can try to finish the patch.
> 
> Wayne, is there anything we can do to prevent stuff like this? I mean to
> leave a bug marked critical+dataloss with a patch (with positive reviews)
> left rotting in bugzilla? :)

It wasn't even marked critical+dataloss until I came around...
Per below, the AB might not physically be deleted but just no longer accesible via the UI because TB gets confused about it, but for the user, that's still dataloss anyway!

(In reply to cht1128 from Bug 887140 comment #6)
> (In reply to Wayne Mery (:wsmwk) from comment #3)
> > After I restarted TB, no address book C (or A) is shown. gone
> Although ur abook is still in the profile dir, TB cant display it, probably
> because it does not have a name. If u rename the abook again after its name
> disappeared, then everything is ok.
> 
> ver 31.0
Summary: Rename Address book aaa to aaa_test, delete another address book bbb, and renamed address book aaa_test will lose its name and be deleted (dataloss! involving localized names) → Rename Address book aaa to aaa_test, delete another address book bbb, and renamed address book aaa_test will lose its name and appear deleted after restart (dataloss! involving localized names)
(In reply to Thomas D. from comment #16)
> > Wayne, is there anything we can do to prevent stuff like this? I mean to
> > leave a bug marked critical+dataloss with a patch (with positive reviews)
> > left rotting in bugzilla? :)
> 
> It wasn't even marked critical+dataloss until I came around...

Yeah, right. And that was now in 2015.

So I apologize to Wayne, that is nothing QA could solve with running some bug query.
"Wayne, is there anything we can do to prevent stuff like this? I mean to leave a bug marked critical+dataloss with a patch (with positive reviews) left rotting in bugzilla? :)"

It would be great if people interested in QA had some standard BMO searches they do, and then advocate for particular bugs that they think are important.

I've never understood the opposition that some people have to occasional pings in bugs that remind us to take action. Thanks Thomas D for finding this, and aceman for stepping forward!
(In reply to Kent James (:rkent) from comment #20)
> "Wayne, is there anything we can do to prevent stuff like this? I mean to
> leave a bug marked critical+dataloss with a patch (with positive reviews)
> left rotting in bugzilla? :)"
> 
> It would be great if people interested in QA had some standard BMO searches
> they do, and then advocate for particular bugs that they think are important.

If someone can come up with a query that finds plussed, unlanded patches then what you suggest will be possible. I don't know of such a beast.

Equally helpful, and an approximation to your request, is for people who care about patches to watch for bugs where [patchlove] has been added.
(In reply to Wayne Mery (:wsmwk, use Needinfo for questions) from comment #21)
> If someone can come up with a query that finds plussed, unlanded patches
> then what you suggest will be possible. I don't know of such a beast.
Maybe just ASSIGNED bugs with patches with last modification a long ago with dataloss keywords or hgih severity?

> Equally helpful, and an approximation to your request, is for people who
> care about patches to watch for bugs where [patchlove] has been added.

Yes, but that is too broad and pulls too many bugs. I'd imagine for QA team to make some priority lists, e.g. take [patchlove] bugs with high severity.

I would imagine some kind of dashboard page where anybody in the TB team could go and click these predefined queries to see if there is any high priority stuff to do.

(In reply to Kent James (:rkent) from comment #20)
> It would be great if people interested in QA had some standard BMO searches
> they do, and then advocate for particular bugs that they think are important.

Yes, something like this. Sometimes people like me would welcome being pointed to important stuff. Otherwise I keep cleaning up trivial compiler warnings in the dust ;)
(In reply to :aceman from comment #22) 
> (In reply to Kent James (:rkent) from comment #20)
> > It would be great if people interested in QA had some standard BMO searches
> > they do, and then advocate for particular bugs that they think are important.
> 
> Yes, something like this. Sometimes people like me would welcome being
> pointed to important stuff. Otherwise I keep cleaning up trivial compiler
> warnings in the dust ;)

This already happens at a heathly rate IMO. But there could be more, as long as people don't become demanding in their ping requests.

As for queries, some of this is easy to do on your own by, for example, simply adding severity column to your queries. click "Change Columns".  Other stuff not so simple.  Let's pursue these things outside the bug.
This is a version of the patch previously proposed for bug 745664 synced to comm-central hg sources as of 150421. I wanted to report that this patch seems to work for me. I'm not familiar with the test procedures so I'm submitting this as a comment as opposed to a formal patch.
Comment on attachment 8595657 [details]
Patch synced to hg 150421; reporting that it works for me

This is a version of the patch previously proposed for bug 745664 synced to comm-central hg sources as of 150421. This patch seems to fix some problems but I still see an issue after deleting address books; descriptions of other address books are reset.
(In reply to Robert Kiraly from comment #25)
> This patch seems to fix some problems
> but I still see an issue after deleting address books; descriptions of other
> address books are reset.

Exactly. I was also trying out the patch on my own, but it does not fix the bug. Even the attached test does not pass with the patch. It seems to me there are several places setting the preferences and they do not use the same state variables so use different values for the "description".
Comment on attachment 8595657 [details]
Patch synced to hg 150421; reporting that it works for me

>@@ -130,9 +131,10 @@
>       }
>     }
> 
>-    if (id == idDescription)
>-      // Ensure the local copy of the description is kept up to date.
>-      server->description = DIR_GetStringPref(prefname, "description", nullptr);
>+   if (id == idDescription) {
>+       // Ensure the local copy of the description is kept up to date.
>+       server->description = DIR_GetDescription (prefname);
>+   }
>   }

IIRC, |prefname| includes "description" here so the right thing to do is to use DIR_GetLocalizedStringPref as the original patch does.
I'd thought somebody had said to replace DIR_GetLocalizedStringPref with DIR_GetDescription so I did it that way. I'll try reverting the change during my next build.
(In reply to Robert Kiraly from comment #28)
> I'd thought somebody had said to replace DIR_GetLocalizedStringPref with
> DIR_GetDescription so I did it that way. I'll try reverting the change
> during my next build.

Yes, that was said in comment 12, but in comment 13 Hiro said something that could indicate that prefname already contains ".description" so it shouldn't be added to it again (neither in original code not with DIR_GetDescription). So let's try Hiro's original 'DIR_GetLocalizedStringPref(prefname);' change.
I made that one change to the synced patch that I posted. TB-hg is confirmed to work so far with the modified patch.

I need to switch my distro to TB 24.8.1 due to other issues with TB-hg. However, I'll attempt to maintain a copy of TB-hg in parallel. If further patches are posted in this thread, I may test them.
Attached patch updated working patch (obsolete) — Splinter Review
This is an updated patch with all hg metadata and it works for me now. Standard8 question was OK, but the patch was fine, the suggested change does not need to be done.
Attachment #615303 - Attachment is obsolete: true
Attachment #8595657 - Attachment is obsolete: true
Attachment #8596130 - Flags: review?(neil)
Attached patch updated test (obsolete) — Splinter Review
Updated and improved patch (tests 4 ABs to have some on both sides of the deletion). Also collected mconley's comments.
Attachment #615532 - Attachment is obsolete: true
Attachment #8596134 - Flags: review?(mkmelin+mozilla)
Product: Thunderbird → MailNews Core
Hardware: x86 → All
Whiteboard: [patchlove]
Attachment #8596134 - Attachment description: updated patch → updated test
Comment on attachment 8596130 [details] [diff] [review]
updated working patch

Why keep DIR_GetDescription if there's only one caller?
Attached patch updated working patch v1.1 (obsolete) — Splinter Review
There were 2 callers (the function existed before the patch). But it can also be reworked to have similar arguments as the other functions (for other pref types).
Attachment #8596130 - Attachment is obsolete: true
Attachment #8596130 - Flags: review?(neil)
Attachment #8597456 - Flags: review?(neil)
Comment on attachment 8597456 [details] [diff] [review]
updated working patch v1.1

>-    if (id == idDescription)
>+    if (id == idDescription) {
>       // Ensure the local copy of the description is kept up to date.
>-      server->description = DIR_GetStringPref(prefname, "description", nullptr);
>+      server->description = DIR_GetLocalizedStringPref(prefname, nullptr);
>+    }
[Is it me or does this leak the old value of server->description?]

>-          (t1 = DIR_GetStringPref(prefname, "description", nullptr)) != nullptr)
>+          (t1 = DIR_GetLocalizedStringPref(prefname, "description")) != nullptr)
[Technically we don't need this because we don't use the value of t1, but it's good for consistency.)

>+    prefLocation.AppendLiteral(".");
Append('.') [as is used in the existing code]. r=me with that fixed.

>-  rv = pPref->GetComplexValue(prefLocation.get(), NS_GET_IID(nsIPrefLocalizedString), getter_AddRefs(locStr));
>+  rv = pPref->GetComplexValue(prefLocation.get(),
>+                              NS_GET_IID(nsIPrefLocalizedString),
>+                              getter_AddRefs(locStr));
[Annyoing gratuitous whitespace-only change]

(In reply to aceman from comment #34)
> There were 2 callers (the function existed before the patch).

Actually it only had the one caller before the patch, but this patch is slightly better anyway.
Attachment #8597456 - Flags: review?(neil) → review+
Thanks.
Attachment #8597456 - Attachment is obsolete: true
Attachment #8597743 - Flags: review+
Keywords: checkin-needed
Test patch is not reviewed yet, so removing checkin-needed.
Keywords: checkin-needed
Yeah, thanks for noticing.

Magnus, can you look at this with highest priority? It should go into TB38. The test is partially reviewed by mconley, I just added some more lines to it.
Flags: needinfo?(mkmelin+mozilla)
Comment on attachment 8596134 [details] [diff] [review]
updated test

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

::: mailnews/addrbook/test/unit/test_nsAbManager3.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*

tiny nit: since this is documentation start with /**
also, the bug number is not that important so don't put it first

@@ +78,5 @@
> +  do_register_cleanup(function() {
> +    MailServices.ab.removeAddressBookListener(gAbListener);
> +  });
> +
> +  let dirNames = [ "testAb0", "testAb1", "testAb2", "testAb3" ];

nit: no extra space after [ and before ]
Attachment #8596134 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Magnus Melin from comment #39)
> also, the bug number is not that important so don't put it first
I used to do that in many other tests and it was never a problem. When either the whole test, or a whole function was to test a specific bug. But sure, I can put it elsewhere :)
Attachment #8596134 - Attachment is obsolete: true
Flags: needinfo?(mkmelin+mozilla)
Attachment #8598153 - Flags: review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
This looks like it should be uplifted?
Flags: needinfo?(acelists)
Yes, I think the flags are set properly so that rkent picks it up after it backes on trunk for some days.
Flags: needinfo?(acelists)
Attachment #8597743 - Flags: approval-comm-beta?
Attachment #8597743 - Flags: approval-comm-aurora?
Attachment #8598153 - Flags: approval-comm-beta?
Attachment #8598153 - Flags: approval-comm-aurora?
Comment on attachment 8597743 [details] [diff] [review]
updated working patch v1.2

https://hg.mozilla.org/releases/comm-beta/rev/f70c422c7904
https://hg.mozilla.org/releases/comm-aurora/rev/080b115a0f69
Attachment #8597743 - Flags: approval-comm-beta?
Attachment #8597743 - Flags: approval-comm-beta+
Attachment #8597743 - Flags: approval-comm-aurora?
Attachment #8597743 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.