The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in 4.6.2

Status

NSPR
NSPR
RESOLVED FIXED
16 years ago
11 years ago

People

(Reporter: Michael Entin, Assigned: masayuki)

Tracking

(4 keywords)

4.6.2
x86
Windows 2000
fixed1.8.1, intl, jp-critical, verified1.8.0.2
Bug Flags:
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tjp-dl], URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

16 years ago
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
*** Bug 112533 has been marked as a duplicate of this bug. ***
Severity: minor → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

15 years ago
mass reassign of pchen bookmark bugs to ben
Assignee: pchen → ben
Status: NEW → ASSIGNED
Target Milestone: --- → Future

Updated

15 years ago
Blocks: 120814

Comment 3

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

Can we resolve this as worksforme?

Updated

15 years ago
Keywords: intl

Comment 4

15 years ago
Old Summary: IE bookmarks containing non-western characters are not exported

New Summary: IE favorites with non-western characters are not imported
Summary: IE bookmarks containing non-western characters are not exported → IE favorites with non-western characters are not imported

Comment 5

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

Comment 6

15 years ago
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

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

Comment 8

14 years ago
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.
Mass reassign of my non-Firefox bugs to ben_seamonkey@hotmail.com
Assignee: bugs → ben_seamonkey
Status: ASSIGNED → NEW

Comment 10

13 years ago
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

13 years ago
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

13 years ago
L10N is the localized builds right? so this should definately blocking them i'm
going to request it to block that build.
Flags: blocking-aviary1.0-L10N?

Updated

13 years ago
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0?

Comment 13

13 years ago
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

13 years ago
This is a Mozilla bug and not a Firefox bug.
Flags: blocking-aviary1.0mac?
Flags: blocking-aviary1.0mac-
Flags: blocking-aviary1.0?
Flags: blocking-aviary1.0-L10N?
Flags: blocking-aviary1.0-L10N-
Flags: blocking-aviary1.0-
Product: Browser → Seamonkey
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

13 years ago
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.
Flags: blocking-aviary1.0mac-
Flags: blocking-aviary1.0-L10N-
Flags: blocking-aviary1.0-
Assignee: ben_seamonkey → p_ch
QA Contact: claudius → bookmarks
(Assignee)

Updated

11 years ago
Component: Bookmarks → Internationalization
Product: Mozilla Application Suite → Core
Summary: IE favorites with non-western characters are not imported → If a folder ends '\' or '/' in IE Favorites, the folder cannot be imported
Target Milestone: Future → mozilla1.8.1
(Assignee)

Updated

11 years ago
Assignee: p_ch → smontagu
QA Contact: bookmarks → amyy
(Assignee)

Comment 17

11 years ago
Created attachment 211114 [details] [diff] [review]
Patch rv1.0

This is a NSPR bug. It is not checking the DBCS lead byte.
Assignee: smontagu → masayuki
Status: NEW → ASSIGNED
Attachment #211114 - Flags: superreview?(darin)
Attachment #211114 - Flags: review?(jshin1987)
(Assignee)

Updated

11 years ago
Summary: If a folder ends '\' or '/' in IE Favorites, the folder cannot be imported → If a folder name ends '\' or '/' in IE Favorites, the folder cannot be imported

Comment 18

11 years ago
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.
Component: Internationalization → NSPR
Product: Core → NSPR
Target Milestone: mozilla1.8.1 → ---
Version: Trunk → 3.0

Comment 19

11 years ago
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.
Attachment #211114 - Flags: review?(wtchang)
Attachment #211114 - Flags: review?(jshin1987)
Attachment #211114 - Flags: review+
(Assignee)

Comment 20

11 years ago
Comment on attachment 211114 [details] [diff] [review]
Patch rv1.0

Sorry. This patch has a problem. I get the feedback.
Attachment #211114 - Flags: superreview?(darin)
Attachment #211114 - Flags: review?(wtchang)
Attachment #211114 - Flags: review-
(Assignee)

Updated

11 years ago
Keywords: jp-critical
(Assignee)

Comment 21

11 years ago
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.
Attachment #211114 - Attachment is obsolete: true
Attachment #211138 - Flags: review?(jshin1987)
(Assignee)

Updated

11 years ago
Attachment #211138 - Flags: review?(wtchang)

Comment 22

11 years ago
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.
Attachment #211138 - Flags: review?(wtchang) → review+
(Assignee)

Comment 23

11 years ago
Created attachment 211232 [details] [diff] [review]
Patch rv3.0
Attachment #211138 - Attachment is obsolete: true
Attachment #211232 - Flags: review+
Attachment #211138 - Flags: review?(jshin1987)
(Assignee)

Updated

11 years ago
Attachment #211232 - Flags: review?(jshin1987)
(Assignee)

Comment 24

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

Kimura-san:

I changed |CharPrevA| to |_mbsdec|, is it right?
Attachment #211232 - Flags: review?(VYV03354)

Comment 25

11 years ago
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?
Attachment #211232 - Flags: review?(jshin1987)

Comment 26

11 years ago
Created attachment 211244 [details] [diff] [review]
additional patch

Comment 27

11 years ago
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 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.
Attachment #211232 - Flags: review?(VYV03354) → review+
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
(Assignee)

Comment 30

11 years ago
(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

11 years ago
(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). 



(Assignee)

Comment 32

11 years ago
(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?
(Assignee)

Comment 33

11 years ago
Created attachment 211259 [details] [diff] [review]
Patch for check-in
Attachment #211232 - Attachment is obsolete: true
Attachment #211244 - Attachment is obsolete: true
(Assignee)

Comment 34

11 years ago
O.K. I filed bug 326544 for original report of this bug.
This bug should be closed, because this has many non-related attachments.
(Assignee)

Comment 35

11 years ago
Wan-Teh Chang:

Oh, I cannot check-in to NSPR. Could you check-in attachment 211259 [details] [diff] [review]? That cleared my build test.
(Assignee)

Updated

11 years ago
Whiteboard: [needs check-in]

Comment 36

11 years ago
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.
Attachment #211259 - Flags: review-

Comment 37

11 years ago
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.
Attachment #211259 - Attachment is obsolete: true
Attachment #212170 - Flags: superreview?(jshin1987)
Attachment #212170 - Flags: review?(masayuki)
(Assignee)

Comment 38

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

Thanks. r=me.
Attachment #212170 - Flags: review?(masayuki) → review+

Comment 39

11 years ago
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.
Attachment #212170 - Flags: superreview?(jshin1987)
Attachment #212170 - Flags: superreview?(darin)
Attachment #212170 - Flags: review+

Comment 40

11 years ago
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.
Attachment #212170 - Flags: superreview?(darin) → superreview+

Comment 41

11 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 4.7
(Assignee)

Comment 42

11 years ago
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?
Attachment #212170 - Flags: approval-branch-1.8.1?(wtchang)

Updated

11 years ago
Attachment #212170 - Flags: approval-branch-1.8.1?(wtchang) → approval-branch-1.8.1+

Comment 43

11 years ago
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.

Updated

11 years ago
Whiteboard: [needs check-in]
Target Milestone: 4.7 → 4.6.2
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1
(Assignee)

Comment 44

11 years ago
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.
Attachment #212170 - Flags: approval1.8.0.2?
Comment on attachment 212529 [details] [diff] [review]
Patch for MOZILLA_1_8_BRANCH

approved for 180 branch, a=dveditz for drivers
Attachment #212529 - Flags: approval1.8.0.2+
Flags: blocking1.8.0.2+
Attachment #212170 - Flags: approval1.8.0.2?
(Assignee)

Updated

11 years ago
Whiteboard: [needs check-in to 1.8.0 branch]

Comment 46

11 years ago
I checked in the "patch for MOZILLA_1_8_BRANCH" on the
MOZILLA_1_8_0_BRANCH for Mozilla 1.8.0.2.
Keywords: fixed1.8.0.2
Whiteboard: [needs check-in to 1.8.0 branch]
(Assignee)

Comment 47

11 years ago
Thank you for your work!

Updated

11 years ago
Whiteboard: [tjp-dl]
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.2 → verified1.8.0.2
You need to log in before you can comment on or make changes to this bug.