Last Comment Bug 111428 - If a folder name ends '\' or '/' in IE Favorites, the folder cannot be imported
: If a folder name ends '\' or '/' in IE Favorites, the folder cannot be imported
Status: RESOLVED FIXED
[tjp-dl]
: fixed1.8.1, intl, jp-critical, verified1.8.0.2
Product: NSPR
Classification: Components
Component: NSPR (show other bugs)
: 3.0
: x86 Windows 2000
: -- normal with 13 votes (vote)
: 4.6.2
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
: Yuying Long
:
Mentors:
http://www.computerra.ru
: 112533 140362 (view as bug list)
Depends on:
Blocks: 120814
  Show dependency treegraph
 
Reported: 2001-11-22 02:25 PST by Michael Entin
Modified: 2006-03-17 08:32 PST (History)
18 users (show)
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch rv1.0 (7.28 KB, patch)
2006-02-08 03:43 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
jshin1987: review+
masayuki: review-
Details | Diff | Splinter Review
Patch rv2.0 (8.00 KB, patch)
2006-02-08 08:16 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
wtc: review+
Details | Diff | Splinter Review
Patch rv3.0 (6.49 KB, patch)
2006-02-08 21:13 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
masayuki: review+
VYV03354: review+
Details | Diff | Splinter Review
additional patch (4.17 KB, patch)
2006-02-09 02:46 PST, Jungshik Shin
no flags Details | Diff | Splinter Review
Patch for check-in (10.40 KB, patch)
2006-02-09 06:46 PST, Masayuki Nakano [:masayuki] (Mozilla Japan)
wtc: review-
Details | Diff | Splinter Review
Patch for check-in, v2 (10.91 KB, patch)
2006-02-16 17:08 PST, Wan-Teh Chang
masayuki: review+
jshin1987: review+
darin.moz: superreview+
wtc: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
Patch for MOZILLA_1_8_BRANCH (11.22 KB, patch)
2006-02-20 14:47 PST, Wan-Teh Chang
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review

Description Michael Entin 2001-11-22 02:25:24 PST
Repro:
1. Open IE, open page http://www.computerra.ru, bookmark it, close IE
2. Open Mozilla, click Bookmarks\Imported IE Favorites

Result: bookmark is not there
Western-character-only bookmarks are OK

Expected: Bookmark displayed
Comment 1 Matthias Versen [:Matti] 2001-11-28 19:35:42 PST
*** Bug 112533 has been marked as a duplicate of this bug. ***
Comment 2 Peter Trudelle 2002-01-05 15:36:39 PST
mass reassign of pchen bookmark bugs to ben
Comment 3 Andrew Hagen 2002-02-25 13:30:06 PST
This appears to be fixed. See bug 122816 for a related problem.

Can we resolve this as worksforme?
Comment 4 Andrew Hagen 2002-04-20 14:24:25 PDT
Old Summary: IE bookmarks containing non-western characters are not exported

New Summary: IE favorites with non-western characters are not imported
Comment 5 Pierre Chanial 2002-06-10 13:39:58 PDT
*** Bug 140362 has been marked as a duplicate of this bug. ***
Comment 6 marina 2002-06-10 13:48:59 PDT
i just want to add some comments from bug # 140362: this problem occurs when the
browser charset is different from system locale, e.g: japanese bookmarks would
be imported/displayed on the japanese system, as well as russian on the cyrillic
locale.
Comment 7 Asa Dotzler [:asa] 2002-10-16 17:02:28 PDT
*** Bug 174734 has been marked as a duplicate of this bug. ***
Comment 8 Peter Kirk 2003-05-29 12:53:11 PDT
Not fixed for me. I have an IE6 favourite with partly Hebrew characters in
Win2000 western setup. There are two separate issues here:

1) The bookmark doesn't appear at all under "imported IE favorites".

2) I exported my IE favourites to a file and reimported to Mozilla. The bookmark
now appears but with Hebrew characters replaced by ?'s. But the ?'s are in the
exported file, so this one is IE's problem.
Comment 9 Ben Goodger (use ben at mozilla dot org for email) 2004-02-17 16:55:57 PST
Mass reassign of my non-Firefox bugs to ben_seamonkey@hotmail.com
Comment 10 carl 2004-10-01 06:15:56 PDT
Can confirm this. The bookmark´s description and title is using cyrillic characters.
A bookmark with German umlauts works instead. (Happy me)


Another problem with importing/editing bookmarks in FF:
You can´t delete the personal tollbar folder. If you import bookmarks from
Mozilla   you will probably have two of those folders then. You are only able to
delete the imported folder.

Several times importing IE Favorites creates folder "imported IE favorites"
several times.
Comment 11 Josh Powell 2004-10-20 08:03:59 PDT
It would be great if this was fixed.. I had a large selection of bookmarks in
IE, and when I switched to Firefox lots of them weren't imported correctly..
and, its not just non-western characters, some characters such as quotation
marks and others similar to that are the same way.
Comment 12 eric 2004-10-27 16:16:29 PDT
L10N is the localized builds right? so this should definately blocking them i'm
going to request it to block that build.
Comment 13 Jason Daly 2004-10-29 15:14:43 PDT
I think it's very important that this bug is fixed, especially because a alot of
people who will be switching from IE will want to have all there bookmarks imported.
Comment 14 Asa Dotzler [:asa] 2004-11-05 15:10:11 PST
This is a Mozilla bug and not a Firefox bug.
Comment 15 Bill Gianopoulos [:WG9s] 2004-12-01 14:09:43 PST
Asa-

See comment #7.  You duped a firefox bug to this bug yet in comment #14 you say
"This is a Mozilla bug and not a Firefox bug."  which is it?
Comment 16 Michael Lefevre 2004-12-02 05:18:40 PST
AIUI, there's different code doing this in Seamonkey and Firefox, and this was
clearly a Seamonkey bug. I'll undupe the duplicate bug and bug 174734 can cover
this issue for Firefox.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-08 03:43:12 PST
Created attachment 211114 [details] [diff] [review]
Patch rv1.0

This is a NSPR bug. It is not checking the DBCS lead byte.
Comment 18 Jungshik Shin 2006-02-08 06:58:19 PST
As far as xpcom is concerned, this doesn't matter any more if bug 162361 is fixed. Nonetheless, this needs to be fixed in NSPR.
Comment 19 Jungshik Shin 2006-02-08 07:00:04 PST
Comment on attachment 211114 [details] [diff] [review]
Patch rv1.0

r=jshin
I'm not so fond of 'IsChar', but I don't have any better name.
Comment 20 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-08 07:09:05 PST
Comment on attachment 211114 [details] [diff] [review]
Patch rv1.0

Sorry. This patch has a problem. I get the feedback.
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-08 08:16:07 PST
Created attachment 211138 [details] [diff] [review]
Patch rv2.0

On previous patch, if the last character is '\' and its previous character's second byte is in leading byte range, it is not working fine. This patch may fix the problem.
Comment 22 Wan-Teh Chang 2006-02-08 12:00:59 PST
Comment on attachment 211138 [details] [diff] [review]
Patch rv2.0

Thanks a lot for your patch.  Please attach a new
patch with the following changes.

1. "primpl.h" already includes <windows.h>, so we
don't need to include <windows.h> again.

2. In NSPR source code, the indentation offset is
4, not 2 (the indentation offset of Mozilla source
code).

3. Is it possible to use functions in <mbstring.h>
so we can avoid linking with user32.lib?  Should we
replace CharPrevA in this patch by _mbsdec?

We are using two <mbstring.h> functions (_mbsinc and
_mbspbrk) in ntio.c and w95io.c.  Should we replace
_mbsinc by CharNextA?  But I can't find a replacement
for _mbspbrk.

4. Modify IsLastChar to take another 'const char *'
argument, pass 'filename + len' or 'fn + len' (or equivalently, '&filename[len]' or '&fn[len]') as
that argument.  The function would be renamed
IsPrevChar.

A further optimization is to rename the function
IsPrevCharSlashBackslash and hardcode the comparisons
with '/' and '\\' in the function.
Comment 23 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-08 21:13:38 PST
Created attachment 211232 [details] [diff] [review]
Patch rv3.0
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-08 21:16:17 PST
Comment on attachment 211232 [details] [diff] [review]
Patch rv3.0

Kimura-san:

I changed |CharPrevA| to |_mbsdec|, is it right?
Comment 25 Jungshik Shin 2006-02-09 02:42:58 PST
Comment on attachment 211232 [details] [diff] [review]
Patch rv3.0


there are three other places with a similar problem in w95io/ntio.c. I'll upload a patch that should be applied on top of this patch. Why don't you combine it with yours?
Comment 26 Jungshik Shin 2006-02-09 02:46:08 PST
Created attachment 211244 [details] [diff] [review]
additional patch
Comment 27 Jungshik Shin 2006-02-09 03:04:45 PST
Masayuki, I just realized that you had metamorphosed this bug into something different from what's originally reported. What's originally reported was basically the same as bug 174734 (aside from firefox vs seamonkey difference) and has little to do with NSPR. I'm not saying we should not fix  the NSPR bug being fixed now, but you should have filed a new bug for that. 


Comment 28 Masatoshi Kimura [:emk] 2006-02-09 04:28:24 PST
Comment on attachment 211232 [details] [diff] [review]
Patch rv3.0

Looks good except that you should check whether aCurrent is greater or equal to aStr + 1 first.
Comment 29 Masatoshi Kimura [:emk] 2006-02-09 04:34:06 PST
Comment on attachment 211232 [details] [diff] [review]
Patch rv3.0

Sorry, please replace of the following phrase:
> aCurrent is greater or equal to aStr + 1
by:
whether aCurrent is greater than aStr
Comment 30 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-09 06:25:38 PST
(In reply to comment #27)
> Masayuki, I just realized that you had metamorphosed this bug into something
> different from what's originally reported. What's originally reported was
> basically the same as bug 174734 (aside from firefox vs seamonkey difference)
> and has little to do with NSPR. 

If you are right, our report(bug 112533) should not be duped to this bug...
See comment 1.
Comment 31 Jungshik Shin 2006-02-09 06:35:14 PST
(In reply to comment #30)
> (In reply to comment #27)
> > Masayuki, I just realized that you had metamorphosed this bug into something
> > different from what's originally reported. 
> If you are right, our report(bug 112533) should not be duped to this bug...
> See comment 1.

Your  fix doesn't fix what's reported orignally but it only fixes bug 112533. What more do you need to be convinced about what I wrote? 

The fact that bug 112533 was *wrongly* duped to this bug  doesn't make the orignal report the same as what you're fixing (which is bug 112533). bug 112533 should NOT have been duped to this bug. All other comments (4, 6, 8, 10 through 16, etc) are about not being able to import IE favorites whose names include characters outside the current default code page (i.e. Arabic in Japanese Windows, Hebrew in French Windows). 



Comment 32 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-09 06:43:53 PST
(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #27)
> > > Masayuki, I just realized that you had metamorphosed this bug into something
> > > different from what's originally reported. 
> > If you are right, our report(bug 112533) should not be duped to this bug...
> > See comment 1.
> 
> Your  fix doesn't fix what's reported orignally but it only fixes bug 112533.
> What more do you need to be convinced about what I wrote? 
> 
> The fact that bug 112533 was *wrongly* duped to this bug  doesn't make the
> orignal report the same as what you're fixing (which is bug 112533). bug 112533
> should NOT have been duped to this bug. All other comments (4, 6, 8, 10 through
> 16, etc) are about not being able to import IE favorites whose names include
> characters outside the current default code page (i.e. Arabic in Japanese
> Windows, Hebrew in French Windows). 
> 

Yeah, but what should we do?

reopening bug 112533 -> fixed and this bug being dupped to bug 174734?
Or marking to fixed this bug is same as bug 112533?
Comment 33 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-09 06:46:37 PST
Created attachment 211259 [details] [diff] [review]
Patch for check-in
Comment 34 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-09 07:15:30 PST
O.K. I filed bug 326544 for original report of this bug.
This bug should be closed, because this has many non-related attachments.
Comment 35 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-09 07:24:08 PST
Wan-Teh Chang:

Oh, I cannot check-in to NSPR. Could you check-in attachment 211259 [details] [diff] [review]? That cleared my build test.
Comment 36 Wan-Teh Chang 2006-02-16 16:59:33 PST
Comment on attachment 211259 [details] [diff] [review]
Patch for check-in

This patch looks good.  The reason I gave it review- is
the following change:

>-        if (!_PR_IS_SLASH(pathbuf[len - 1])) {
>+        if (IsPrevChar(pathbuf, pathbuf + len, '\\') ||
>+            IsPrevChar(pathbuf, pathbuf + len, '/')) {

I also found that we always make two IsPrevChar calls
together:

  IsPrevChar(..., '/') || IsPrevChar(..., '\\')

so we should combine the two calls into one function. I
will attach a new patch.
Comment 37 Wan-Teh Chang 2006-02-16 17:08:35 PST
Created attachment 212170 [details] [diff] [review]
Patch for check-in, v2

1. Fix a bug in jshin's "additional patch" (the ! in
front of _PR_IS_SLASH(pathbuf[len - 1]) was lost in
the new code).

2. Combine the paired IsPrevChar calls into the
IsPrevCharSlash function.

3. Other minor coding style changes.  One of them is
to change the _mbspbrk call back to the do-while loop.
This is a matter of personal preference.  The reason
I like the do-while loop better is that there is another
same do-while loop (with the comment "look for the final
slash") right below, so it's good to have some symmetry.
Comment 38 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-17 09:06:42 PST
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

Thanks. r=me.
Comment 39 Jungshik Shin 2006-02-17 12:37:09 PST
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

Thanks for catching my mistake and fixing it. 
I'm not a superreviewer. Passing it to Darin.
Comment 40 Darin Fisher 2006-02-17 14:55:25 PST
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

sr=darin

be sure to land this on the NSPR trunk as well as on the NSPRPUB_PRE_4_2_CLIENT_BRANCH, which the mozilla trunk uses.
Comment 41 Wan-Teh Chang 2006-02-17 15:17:19 PST
I checked in the patch on the NSPR trunk (NSPR 4.7) and
the NSPRPUB_PRE_4_2_CLIENT_BRANCH (mozilla 1.9 alpha).

Checking in ntio.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntio.c,v  <--  ntio.c
new revision: 3.42; previous revision: 3.41
done
Checking in w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.28; previous revision: 3.27
done

Checking in ntio.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntio.c,v  <--  ntio.c
new revision: 3.36.2.6; previous revision: 3.36.2.5
done
Checking in w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.22.4.5; previous revision: 3.22.4.4
done
Comment 42 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-18 07:13:29 PST
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

Thank you.

Wan-Teh:

If we don't have any regression on Trunk, I hope that this will go to 1.8 branch and 1.8.0 branch too. Is it possible?
Comment 43 Wan-Teh Chang 2006-02-20 14:47:00 PST
Created attachment 212529 [details] [diff] [review]
Patch for MOZILLA_1_8_BRANCH

I checked in this patch on the NSPR_4_6_BRANCH (NSPR 4.6.2)
and MOZILLA_1_8_BRANCH (Mozilla 1.8.1).

This bug may not be critical enough for the MOZILLA_1_8_0_BRANCH.
I don't know what kind of non-security fixes can go into that
branch.
Comment 44 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-21 07:51:52 PST
Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

> This bug may not be critical enough for the MOZILLA_1_8_0_BRANCH.
> I don't know what kind of non-security fixes can go into that
> branch.

This should go to 1.8.0.2, because this problem is critical for Japanese marketing.
By shift_jis encoding, '่กจ' has 0x5c('\') on its second byte. This character is often used on tail of the text.
Comment 45 Daniel Veditz [:dveditz] 2006-02-21 15:46:33 PST
Comment on attachment 212529 [details] [diff] [review]
Patch for MOZILLA_1_8_BRANCH

approved for 180 branch, a=dveditz for drivers
Comment 46 Wan-Teh Chang 2006-02-22 10:50:42 PST
I checked in the "patch for MOZILLA_1_8_BRANCH" on the
MOZILLA_1_8_0_BRANCH for Mozilla 1.8.0.2.
Comment 47 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-02-22 10:52:53 PST
Thank you for your work!

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