111428 - If a folder name ends '\' or '/' in IE Favorites, the folder cannot be imported

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Michael Entin]

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]]

*** Bug 112533 has been marked as a duplicate of this bug. ***

Comment 2

[Peter Trudelle]

mass reassign of pchen bookmark bugs to ben

Comment 3

[Andrew Hagen]

This appears to be fixed. See bug 122816 for a related problem.

Can we resolve this as worksforme?

Comment 4

[Andrew Hagen]

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]

*** Bug 140362 has been marked as a duplicate of this bug. ***

Comment 6

[marina]

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]]

*** Bug 174734 has been marked as a duplicate of this bug. ***

Comment 8

[Peter Kirk]

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)]

Mass reassign of my non-Firefox bugs to [e-mail]

Comment 10

[carl]

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]

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]

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]

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]]

This is a Mozilla bug and not a Firefox bug.

Comment 15

[Bill Gianopoulos [:WG9s]]

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]

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] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

This is a NSPR bug. It is not checking the DBCS lead byte.

Comment 18

[Jungshik Shin]

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]

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] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

Attachment: 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]

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] (he/him)(JST, +0900)]

Attachment: Patch rv3.0

Comment 24

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 211232 [details] [diff] [review]
Patch rv3.0

Kimura-san:

I changed |CharPrevA| to |_mbsdec|, is it right?

Comment 25

[Jungshik Shin]

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]

Attachment: additional patch

Comment 27

[Jungshik Shin]

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]]

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]]

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] (he/him)(JST, +0900)]

(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]

(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] (he/him)(JST, +0900)]

(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] (he/him)(JST, +0900)]

Attachment: Patch for check-in

Comment 34

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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] (he/him)(JST, +0900)]

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]

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]

Attachment: 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] (he/him)(JST, +0900)]

Comment on attachment 212170 [details] [diff] [review]
Patch for check-in, v2

Thanks. r=me.

Comment 39

[Jungshik Shin]

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]

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]

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] (he/him)(JST, +0900)]

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]

Attachment: 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] (he/him)(JST, +0900)]

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]]

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]

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] (he/him)(JST, +0900)]

Thank you for your work!