209110 - GetNativeTarget() function under win32 returns extra '\\'

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Sean Su]

On Win32, when calling GetNativeTarget() on a .lnk to a folder (my test case was
"c:\\"), GetNativeTarget() will return the native path with an extra backslash:
  "c:\\\\"

When calling on a non root folder, such as "C:\\Program Files", it returns:
  "C:\\Program Files\\"

Comment 1

[Doug Turner (:dougt)]

sean, did you have a patch that fixes this problem?

Comment 2

[Sean Su]

not yet.  haven't had a chance to investigate it further.  had to deal with
other bugs first.

I'll try to get a patch going, unless you get to it before I do.

Comment 3

[Dainis Jonitis]

Attachment: diff -u

Comment 4

[Dainis Jonitis]

Attachment: diff -uw

Comment 5

[Dainis Jonitis]

The problem was that GetPath () was returning appended slash for the root
directory, but not for other directories. 
1. Added check for root directory. 
2. Removed tab symbols.

Comment 6

[Sean Su]

Comment on attachment 127667 [details] [diff] [review]
diff -u

why not just check for a trailing backslash regardless of the path?  If there's
already backslash, then skip the appending of a backslash.

Comment 7

[Dainis Jonitis]

Sure. That should be even safer.

BTW the code that checks for shortcuts (".lnk") is not safe because it uses case
sensitive comparison. 

Comment 8

[Dainis Jonitis]

Attachment: diff -u

Append trailing slash only if it is not already present

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 127821 [details] [diff] [review]
diff -u

+		     // For root directory slash is already appended

please fix comment.


+		     if (temp [strlen (temp) - 1] != '\\')

strlen() instead  - no space.

ssu, what bug did you run into with the double slash?

Comment 10

[Dainis Jonitis]

What is wrong with a commnet? It just describes the non standart behaviour of
root directories. The fact that we are not appending slash if one already exists
is obvious from the code.

Comment 11

[Sean Su]

dougt, I was testing the fix to bug 201381 and one of my tests revealed the
double backslash for a migrated bookmark to a shortcut to the root of a drive.

Comment 12

[Sean Su]

Comment on attachment 127821 [details] [diff] [review]
diff -u

I don't think you can test for the trailing backslash that way because if the
string is multibyte, it will not work.

strlen() should return the correct length, but then dereferencing using '[]' is
not multibyte safe because it's dereferencing bytes instead of characters.

Looks like there are many other parts of nsLocalFileWin.cpp that are not
multibyte safe... *sigh*

dougt, shouldn't we be using the string class here for all string manipulation?

r=ssu if dougt is okay with the change, since it's broken everywhere else
already.  not that I'm advocating checking in broken code.  Just that since
there's already brokeness, this shouodn't make it any worse.

If this is to be checked in, please comment that it is not mutibyte safe.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 127821 [details] [diff] [review]
diff -u

fine.  check it in.

Comment 14

[Dainis Jonitis]

Can you check it in for me? I do not have access rights

Comment 15

[Jacek R. (Yano)]

On network printer path is the same:
user_pref("print.printer_\\TREE\\printer-queue.print_bgimages", false);
after next run is changed to:
user_pref("print.printer_\\\\TREE\\\\printer-queue.print_bgimages", false);
After few days my prefs.js had 23 megabytes of this junk!!!

Comment 16

[timeless]

I don't think that this is the right bug. I think you're suffering
from a regression based on converting prefs from js to properties.

Comment 17

[Ari Eskelinen]

I have had the same problem with our local network Filelist.. with IE it shows
"file:\\" + computer name + path... and with Mozilla "file:\\\\" and so on.. 
All other "\\" are correctly.. I see it ALLWAYS... no doubt there is a bug or
difference between IE and Mozilla... And I am using Win98 so it is a general
problem..

Comment 18

[timeless]

that too is an unrelated issue, further it isn't even a bug.

Comment 19

[timeless]

for people testing from home, create c:\test.lnk which points to c:\, then run
xpcshell:

js> a=Components.classes["@mozilla.org/file/local;1"].createInstance(Components.
interfaces.nsILocalFile)
[xpconnect wrapped nsILocalFile]
js> a.initWithPath("c:\\test.lnk")
js> a.path
c:\test.lnk
js> a.exists()
true
js> a.isSymlink()
true
js> a.path
c:\test.lnk
js> a.target
c:\test.lnk
js> a.followLinks
false
js> a.followLinks=true
true
js> a.target
C:\\

the last line, if this is working should only have one slash.

Comment 20

[Alec Flett]

nice! you should attach that as a script that people can run to test this bug
(rather than typing it in manually from your transcript)

Comment 21

[timeless]

Attachment: Testcase

Desktop>mozilla.17a\xpcshell.exe 209110.js || echo FAIL
built on Feb 19 2004 at 13:25:36
File resolved to a path containing two backslashes: C:\\
FAIL

Desktop>mozilla\xpcshell.exe 209110.js && echo PASS
built on Mar  2 2004 at 08:34:50
File resolved to a path with a single trailing backslash: C:\
PASS

Comment 22

[timeless]

mozilla/xpcom/io/nsLocalFileWin.cpp 	1.113