Last Comment Bug 316812 - 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.
: When address book name is Japanese character(doesn't contain ASCII alpha-nume...
Status: VERIFIED FIXED
: dataloss, verified1.8.0.2, verified1.8.1
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: -- critical with 1 vote (vote)
: mozilla1.8.1
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-11-16 23:41 PST by WADA
Modified: 2008-07-31 04:30 PDT (History)
4 users (show)
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Based on comment #6 idea with some modifications (1.14 KB, patch)
2006-01-24 09:01 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Patch rv 1.1: adding more safety check (1.98 KB, patch)
2006-01-25 07:54 PST, Masatoshi Kimura [:emk]
standard8: review+
Details | Diff | Review
with the nits fixed (5.03 KB, patch)
2006-01-27 09:55 PST, Masatoshi Kimura [:emk]
VYV03354: review+
mozilla: superreview+
Details | Diff | Review
diff -w for review purpose only (1.95 KB, patch)
2006-01-27 09:56 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Patch for 1.8 branch (4.90 KB, patch)
2006-01-31 03:46 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
masayuki: review+
masayuki: superreview+
mozilla: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Review
Patch for 1.8 branch (-u8pw) (1.89 KB, patch)
2006-01-31 03:50 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
no flags Details | Diff | Review

Description WADA 2005-11-16 23:41:41 PST
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.
Comment 1 Mark Banner (:standard8) 2005-11-17 00:05:27 PST
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.
Comment 2 WADA 2005-11-17 00:15:07 PST
Mscott or David, how about using "X" of "abook-X.mab" as "N" of "user_directory_N"?
Comment 3 WADA 2005-11-18 19:24:21 PST
This problem also occurs on Seamonkey 2005111809-trunk/Win-2K.
Changing to Product=Core.
Comment 4 WADA 2005-11-18 19:32:37 PST
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?
Comment 5 Mark Banner (:standard8) 2005-12-08 13:50:07 PST
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.
Comment 6 WADA 2006-01-12 20:11:20 PST
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"?
Comment 7 WADA 2006-01-13 18:57:15 PST
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" ;
Comment 8 Masatoshi Kimura [:emk] 2006-01-24 09:01:06 PST
Created attachment 209452 [details] [diff] [review]
Based on comment #6 idea with some modifications
Comment 9 David :Bienvenu 2006-01-24 09:10:57 PST
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.
Comment 10 Masatoshi Kimura [:emk] 2006-01-25 07:54:35 PST
Created attachment 209582 [details] [diff] [review]
Patch rv 1.1: adding more safety check
Comment 11 Mark Banner (:standard8) 2006-01-26 13:35:18 PST
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.
Comment 12 Masatoshi Kimura [:emk] 2006-01-27 09:55:30 PST
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.
Comment 13 Masatoshi Kimura [:emk] 2006-01-27 09:56:06 PST
Created attachment 209861 [details] [diff] [review]
diff -w for review purpose only
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-01-30 10:48:56 PST
checked-in.
Comment 15 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-01-30 10:51:15 PST
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.
Comment 16 Mark Banner (:standard8) 2006-01-30 12:35:05 PST
(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.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-01-31 02:46:41 PST
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.
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-01-31 03:46:52 PST
Created attachment 210227 [details] [diff] [review]
Patch for 1.8 branch
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-01-31 03:50:58 PST
Created attachment 210228 [details] [diff] [review]
Patch for 1.8 branch (-u8pw)
Comment 20 David :Bienvenu 2006-01-31 08:14:36 PST
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.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-01-31 08:53:52 PST
checked-in to 1.8 branch.
Comment 22 Scott MacGregor 2006-02-07 11:16:11 PST
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.
Comment 23 WADA 2006-02-07 13:43:06 PST
(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?
  

Comment 24 WADA 2006-02-07 16:21:33 PST
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.
Comment 25 Daniel Veditz [:dveditz] 2006-02-09 15:42:31 PST
Seems like a reasonable stability branch fix that got missed, plussing for reconsideration
Comment 26 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-10 07:23:22 PST
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.
Comment 27 Scott MacGregor 2006-02-14 09:43:03 PST
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!
Comment 28 WADA 2006-02-14 11:03:45 PST
(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 29 Daniel Veditz [:dveditz] 2006-02-14 15:51:04 PST
Comment on attachment 210227 [details] [diff] [review]
Patch for 1.8 branch

approved for 1.8.0 branch, a=dveditz for drivers
Comment 30 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-15 07:24:22 PST
-> v. (trunk and 1.8 branch)

Note You need to log in before you can comment on or make changes to this bug.