When address book name is Japanese character(doesn't contain ASCII alpha-numeric), address book is lost when new address book is created due to overlay of ldap_2.servers.user_directory_N.xxxx entries.

VERIFIED FIXED in mozilla1.8.1

Status

MailNews Core
Address Book
--
critical
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: World, Assigned: emk)

Tracking

({dataloss, verified1.8.0.2, verified1.8.1})

Trunk
mozilla1.8.1
dataloss, verified1.8.0.2, verified1.8.1
Bug Flags:
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
This problem occurs on both Thunderbird 1.0.7 release build and Thunderbird/nightly/latest-trunk "version 1.6a1 (20051116)".

If address book name doesn't contain ASCII alpha-numeric character, address book is lost when new address book is created due to overlay of ldap_2.servers.user_directory_N.xxxx in prefs.js.
This happnes always if address book name consists of Japanese Character(double byte characters in Shift_JIS etc.) only.

If address book name contains ASCII alpha-numeric,
ldap_2.servers.<ASCII alpha-numeric portion only>.xxxx is used,
and no problem because ldap_2.servers.<ASCII alpha-numeric portion only>_<Suffix>.xxxx is used if dulicate <ASCII alpha-numeric portion only>.

See Bug 218839 Comment #7 for latest usage of ldap_2.servers.xxxx.yyyy.
Changes described in Bug 218839 Comment #7 looks to be already applied to Tb 1.0.7 in addition to trunk nightlies.

I think this is "Core" issue, although I didn't complete test with Seamonky trunk.

[Problem re-creation procedure]
(1)Start Thunderbird
(2)Create new address book named "+".
(3)Shutdown Thunderbird
 prefs.js entries
 > user_pref("ldap_2.servers.user_directory_1.description", "+");
 > user_pref("ldap_2.servers.user_directory_1.dirType", 2);
 > user_pref("ldap_2.servers.user_directory_1.filename", "abook-1.mab");
 > user_pref("ldap_2.servers.user_directory_1.isOffline", false);
 > user_pref("ldap_2.servers.user_directory_1.protocolVersion", "2");
(3)Restart Thunderbird
(4)Create new address book named "-".
 ==> Both "+" and "-" is displayed.
(5)Shutdown Thunderbird
 ==> ldap_2.servers.user_directory_1.xxxx is overlayed.
 prefs.js entries
 > user_pref("ldap_2.servers.user_directory_1.description", "-");
 > user_pref("ldap_2.servers.user_directory_1.dirType", 2);
 > user_pref("ldap_2.servers.user_directory_1.filename", "abook-2.mab");
 > user_pref("ldap_2.servers.user_directory_1.isOffline", false);
 > user_pref("ldap_2.servers.user_directory_1.protocolVersion", "2");
(6)Restart Thunderbird
 ==> Only "-" is displayed.
 ==> address book created at step (2) ("+") is lost.

[Workaround]
Create address book with ASCII alpha-numeric only, then rename thru property of the address book.
Note: whilst this may look like dataloss to most users, you can still get your original address book back by creating a new a then picking up the older abook-x.mab, i.e. in the case above it would still exist as abook-1.mab.
(Reporter)

Comment 2

12 years ago
Mscott or David, how about using "X" of "abook-X.mab" as "N" of "user_directory_N"?
(Reporter)

Comment 3

12 years ago
This problem also occurs on Seamonkey 2005111809-trunk/Win-2K.
Changing to Product=Core.
Component: Address Book → MailNews: Address Book
Product: Thunderbird → Core
(Reporter)

Updated

12 years ago
Assignee: mscott → nobody
QA Contact: address-book → addressbook
(Reporter)

Comment 4

12 years ago
A Japanese user has reported "This bug occurs on Thunderbird 1.0.6 release build" to Bugzilla in Japan.
When changes of Bug 218839 Comment #7 was introduced to 1.0.x branch?
The problem is here: http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsDirPrefs.cpp#2439 in that function we're incrementing dir_UserId but then not saving it back to prefs (as ldap_2.user_id). Ideally, I think we should be removing dir_UserId completely based on other comments in that file, but we'd need to check the implications of doing that.
OS: Windows 2000 → All
Hardware: PC → All
(Reporter)

Comment 6

12 years ago
Cc-ing to David, because your help is needed.

Logic in 1560 char *DIR_CreateServerPrefName is as follows.

1569   if (name)
1570         leafName = nsCRT::strdup(name);
1571   else
1572     leafName = dir_ConvertDescriptionToPrefName (server);

1573   if (leafName)
1574   {
(snip, prefName is defined in this block)
1605   } /* if leafName */

1607   if (!prefName)
          /* last resort if we still don't have a pref name is to use
             user_directory string */
1608     return PR_smprintf(PREF_LDAP_SERVER_TREE_NAME".user_directory_%d",
                            ++dir_UserId);
1609   else
1610     return prefName;

When leafName is returned dir_ConvertDescriptionToPrefName(ASCII AlphaNumeric is included in name), duplication check is perfectly done by line 1573 thru 1605.       

But if no leafName is returned("\0" is retuned when this bug's case), line 1607 thru 1608 is executed.
In this case, before changes described in Bug 218839 Comment #7, I guess that %d was the last entry number then uniequness was always kept.
However, last entry number is not kept in prefs.js any more after the changes, and %d always starts from 1 when restart of Thunderbird, then overaly of prefs.js entry always occurs if no leafName is returned("\0" is returned).

I think next logic between line 1572 and 1573 can be a workaround.
  if (leafName=="" || leafName=="\0")
    {
      leafName = PR_smprintf(PREF_LDAP_SERVER_TREE_NAME".user_directory_%d",
                             ++dir_UserId);
    }
(But it becomes user_directory_NN_XX except first one... Will it be successfully handled?)

And next(%d is not used) seems to be sufficient since suffix of "_X" is increased until unieque name is obtained.
  if (leafName=="" || leafName=="\0")
    {
      leafName = PR_smprintf(PREF_LDAP_SERVER_TREE_NAME".user_directory",
                             ++dir_UserId);
    }
(But it bocomes "user_directory", no suffix, when first one... Will it be successfully handled?)

David, is it right?

Another concern is fall back.
Can previous version of Thunderbird handle entry of "user_directory" or entry of  user_directory_NN_XX"?
(Reporter)

Comment 7

12 years ago
Correction of previous comment.
leafName should not include PREF_LDAP_SERVER_TREE_NAME part.
Should be ;
  leafName = PR_smprintf("user_directory_%d",++dir_UserId) ;
 or
  ++dir_UserId ; leafName = "user_directory" ;
(Assignee)

Comment 8

12 years ago
Created attachment 209452 [details] [diff] [review]
Based on comment #6 idea with some modifications
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Attachment #209452 - Flags: superreview?(bienvenu)
Attachment #209452 - Flags: review?(bienvenu)

Comment 9

12 years ago
Comment on attachment 209452 [details] [diff] [review]
Based on comment #6 idea with some modifications

Mark has been doing stuff with dirPrefs recently, and is much more up to date on it than I am.
Attachment #209452 - Flags: review?(bienvenu) → review?(bugzilla)
Flags: blocking1.8.0.2?
Flags: blocking-thunderbird2?
(Assignee)

Comment 10

12 years ago
Created attachment 209582 [details] [diff] [review]
Patch rv 1.1: adding more safety check
Attachment #209452 - Attachment is obsolete: true
Attachment #209582 - Flags: review?(bugzilla)
Attachment #209452 - Flags: superreview?(bienvenu)
Attachment #209452 - Flags: review?(bugzilla)
Comment on attachment 209582 [details] [diff] [review]
Patch rv 1.1: adding more safety check

This seems to work alright, just a couple of nits:

+    if (!leafName || !*leafName)
+    {
+        // we need to handle this in case the description has no alphanumeric chars
+        // it's very common for cjk users
+        leafName = nsCRT::strdup("_nonascii");
+    }

Please add one blank line either side of this statement.

Also, we normally use 2 space indentation now, so please change the section above to 2 spaces indentation and the additional section to 4 spaces indentation.

I'm not entirely sure the second section is necessary, but I don't see much harm in adding it.

r=me with the nits fixed.
Attachment #209582 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 12

12 years ago
Created attachment 209860 [details] [diff] [review]
with the nits fixed

> Also, we normally use 2 space indentation now, so please change the section
> above to 2 spaces indentation and the additional section to 4 spaces
> indentation.
But some other part of this function seems to assume 4 spaces tab.
So I've changed the whole function to 2 spaces indentation.
Attachment #209582 - Attachment is obsolete: true
Attachment #209860 - Flags: superreview?(bienvenu)
Attachment #209860 - Flags: review+
(Assignee)

Comment 13

12 years ago
Created attachment 209861 [details] [diff] [review]
diff -w for review purpose only

Updated

12 years ago
Attachment #209860 - Flags: superreview?(bienvenu) → superreview+
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Comment on attachment 209860 [details] [diff] [review]
with the nits fixed

This is critical issue and very unpopularity bug in Japan. Let's go to 1.8.0.x and 1.8.1.
Attachment #209860 - Flags: branch-1.8.1?(bienvenu)
Attachment #209860 - Flags: approval1.8.1?
Attachment #209860 - Flags: approval1.8.0.2?
(In reply to comment #15)
> (From update of attachment 209860 [details] [diff] [review] [edit])
> This is critical issue and very unpopularity bug in Japan. Let's go to 1.8.0.x
> and 1.8.1.
> 
Have you checked that the patch will apply to 1.8.x branch? There's been significant changes to some of the rest of nsDirPrefs since we branched.
Attachment #209860 - Flags: approval1.8.1?
Comment on attachment 209860 [details] [diff] [review]
with the nits fixed

Oops. This cannot apply to 1.8 branch. But previous patch can apply to 1.8 branch.

I'll attach new patch for 1.8 branch.
Attachment #209860 - Flags: branch-1.8.1?(bienvenu)
Attachment #209860 - Flags: approval1.8.0.2?
Created attachment 210227 [details] [diff] [review]
Patch for 1.8 branch
Attachment #210227 - Flags: superreview+
Attachment #210227 - Flags: review+
Attachment #210227 - Flags: branch-1.8.1?(bienvenu)
Attachment #210227 - Flags: approval1.8.0.2?
Created attachment 210228 [details] [diff] [review]
Patch for 1.8 branch (-u8pw)

Comment 20

12 years ago
Comment on attachment 210227 [details] [diff] [review]
Patch for 1.8 branch

thx, let me know if you want me to check this into the 1.8.1 branch.
Attachment #210227 - Flags: branch-1.8.1?(bienvenu) → branch-1.8.1+
checked-in to 1.8 branch.
Flags: blocking-thunderbird2?
Keywords: fixed1.8.1
Target Milestone: --- → mozilla1.8.1

Comment 22

12 years ago
Comment on attachment 210227 [details] [diff] [review]
Patch for 1.8 branch

we're not goint to take this for the security branch. This issue was also present in 1.0.x and has already been fixed for Thunderbid 2.
Attachment #210227 - Flags: approval1.8.0.2? → approval1.8.0.2-
(Reporter)

Comment 23

12 years ago
(In reply to comment #22)
> we're not goint to take this for the security branch. This issue was also
> present in 1.0.x and has already been fixed for Thunderbid 2.

As I said in Bug 218839 Comment #12, this bug was found to be regression over 0.9, regression which should have been resolved before shipment of 1.0.0.
So "This issue was also present in 1.0.x" sounds very funny for me as reason of denial.

Firefox team seems to have changed(improved) release rule of 1.5.x from one for 1.0.x. (Not security fix only. If big issue, and if possible, resolve it by next minor release. See http://wiki.mozilla.org/Global:1.9_Trunk_1.8_Branch_Plan)
Mscott, Thunderbird team won't change(improve) rule for 1.5.x from one for 1.0.x?
  

(Reporter)

Comment 24

12 years ago
Needless to say, I should have found this bug when TB 1.0 RC 1 is released, but I couldn't find until 2005/11/16, then solving of this bug before shipment of TB 1.0.0 was impossible.
Seems like a reasonable stability branch fix that got missed, plussing for reconsideration
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Comment on attachment 210227 [details] [diff] [review]
Patch for 1.8 branch

Re-check this. Daniel agreed to fix this bug on 1.8.0.2.
Attachment #210227 - Flags: approval1.8.0.2- → approval1.8.0.2?

Comment 27

12 years ago
Your right, we should fix this for 1.8.0.2.

Masayuki, can you find some folks to verify the fix on the 1.8.1 branch before we land this on 1.8.0.2? Thanks!
(Reporter)

Comment 28

12 years ago
(In reply to comment #27)
> Masayuki, can you find some folks to verify the fix on the 1.8.1 branch before
> we land this on 1.8.0.2?
Verification itself is possible and very easy with folder name of "-" and "+".
Didn't you read comment #0?
Comment on attachment 210227 [details] [diff] [review]
Patch for 1.8 branch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #210227 - Flags: approval1.8.0.2? → approval1.8.0.2+
Keywords: fixed1.8.0.2
-> v. (trunk and 1.8 branch)
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
Keywords: fixed1.8.0.2 → verified1.8.0.2
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.