415761 - nsIconChannel::GetHIconFromFile does not support Unicode filenames on Windows

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P4)

Description

[Martijn Wargers (dead)]

To reproduce:
- go to http://www.baidu.com/
- Got to File->Save Page As... and save the page.

Expected result:
The file is saved (named 百度一下，你就知道.htm currently), the html icon is used should be showing at the left for the download item in the download manager.

Actual result:
No icon at all is shown for the download item at the left in the download manager.

Maybe there is some relation to bug 355461 here?

It seems to happen on this site, but my suspicion is that this happens as soon as saved filename contains at least one utf-8 character.
That's why I'm asking blocking-firefox3?, although the issue itself is rather smallish.

Comment 1

[Shawn Wilsher :sdwilsh]

Is this a regression from 2.0?  This probably has something to do with the moz-icon code...

Comment 2

[Shawn Wilsher :sdwilsh]

Simon - might this also be related to bug 393488, or am I barking up the wrong tree?

Comment 3

[Ed Lee :Mardak]

I'm guessing it's some issue with the windows implementation of moz-icon because it works okay on os x. CCing some people that might know if it's so.

http://mxr.mozilla.org/seamonkey/source/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp

Shawn: I suppose we could work around this by requesting a generic moz-icon if we think the icon will choke on UTF8 files.

e.g., moz-icon://.dmg?size=32 moz-icon://.txt?size=32 moz-icon://.pdf?size=32

Comment 4

[[e-mail]]

The icon code still uses ANSI APIs. It probably needs to use the Unicode APIs.

Comment 5

[Shawn Wilsher :sdwilsh]

Rob - you wanted a bug (see comment 4)?

Comment 6

[Mike Connor [:mconnor]]

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Confirmed on Vista as well.  I'm going to play with this one.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Could somebody with bug edit access please do the following:

-Assign the bug to me
-Move it from download manager to Core:Imagelib
-Change the title to "nsIconChannel::GetHIconFromFile does not support Unicode filenames on Windows"
-Change the platform to Windows


I believe I'll have a patch ready for this late tomorrow or by friday at the latest.

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #8)

Done; thanks!

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for nsIconChannel::GetHIconFromFile

Safe, simple, should do the trick.  Apparently when this code was Unicode converted somebody missed the string handling, because it uses wide syscalls but handles the strings in ASCII.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Once reviewed and checked in on trunk I think we should back this into 1.9.1 as well.  I'll set those flags down the road though.

Comment 12

[Ed Lee :Mardak]

Kyle, how did you create that patch file? Is it on a mq or committed?

Could you export the patch using hg export.. something like:
hg export --config "diff.unified=8" tip

(assuming your patch is applied and is on the tip)

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for nsIconChannel::GetHIconFromFile()

Code same as before, patch is slightly different per Edward's comment.

Comment 14

[[e-mail]]

Comment on attachment 377425 [details] [diff] [review]
Patch for nsIconChannel::GetHIconFromFile()

>+  nsAutoString filePath(NS_ConvertASCIItoUTF16(fileExt).get());
You definitely don't need the .get() in this particular case. In fact, you might find that NS_ConvertASCIItoUTF16 filePath(fileExt); works.

Comment 15

[Joe Drew (not getting mail)]

Comment on attachment 377425 [details] [diff] [review]
Patch for nsIconChannel::GetHIconFromFile()

>+  // Declare as nsAutoString for wide character support (see Bug 415761)

No need for this comment.

Otherwise, looks good. Check Neil Rashbrook's comments too, though.

(Also: I'm now the imagelib maintainer - you'll have more luck getting reviews out of me than out of Stuart. :)

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Thanks Joe.  I'll post a slightly cleaned up version before I ask for superreview.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch for nsIconChannel::GetHIconFromFile

Comment 18

[Joe Drew (not getting mail)]

(In reply to comment #14)
> You definitely don't need the .get() in this particular case. In fact, you
> might find that NS_ConvertASCIItoUTF16 filePath(fileExt); works.

Neil, doesn't that rely on the implementation of NS_ConvertASCIItoUTF16 being an object? Is that a safe assumption?

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Is it ever not an object?  I mean I'm not terribly familiar with the mozilla code yet, but everything I've read on MDC indicates it's a class that only differs from nsAutoString in the constructor.

Comment 20

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 377847 [details] [diff] [review]
Patch for nsIconChannel::GetHIconFromFile


>-  nsCAutoString filePath(fileExt);
>-  if (localFile)
>+  NS_ConvertASCIItoUTF16 filePath(fileExt);
>+  if (localFile) 

This should probably be allocated from a nsString, initialized from a NS_Convert... -- filePath may be modified in the if block, and we really shouldn't modify an object of type NS_ConvertASCIItoUTF16.


>-    filePath = NS_LITERAL_CSTRING(".") + defFileExt;
>+    filePath = NS_ConvertASCIItoUTF16(NS_LITERAL_CSTRING(".") + defFileExt);

This should probably be NS_LITERAL_STRING(".") + defFileExt


>-    shellResult = ::SHGetFileInfoW(NS_ConvertUTF8toUTF16(filePath).get(),
>+    shellResult = ::SHGetFileInfoW(filePath.get(),

I -think-, to be fully correct, you want to use nsPromiseFlatString(filePath).get() here to ensure you get a null-terminated string.

Comment 21

[[e-mail]]

(In reply to comment #20)
> >-  nsCAutoString filePath(fileExt);
> >-  if (localFile)
> >+  NS_ConvertASCIItoUTF16 filePath(fileExt);
> >+  if (localFile) 
> This should probably be allocated from a nsString, initialized from a
> NS_Convert... -- filePath may be modified in the if block, and we really
> shouldn't modify an object of type NS_ConvertASCIItoUTF16.
Well, I thought I'd seen that done before, but if you don't like that, then I believe the next best thing is to use CopyASCIItoUTF16(fileExt, filePath);

> >-    filePath = NS_LITERAL_CSTRING(".") + defFileExt;
> >+    filePath = NS_ConvertASCIItoUTF16(NS_LITERAL_CSTRING(".") + defFileExt);
> This should probably be NS_LITERAL_STRING(".") + defFileExt
I don't think that's right; defFileExt isn't Unicode, but you could use
filePath.AssignLiteral(".");
AppendASCIItoUTF16(defFileExt, filePath);
for instance.

> >-    shellResult = ::SHGetFileInfoW(NS_ConvertUTF8toUTF16(filePath).get(),
> >+    shellResult = ::SHGetFileInfoW(filePath.get(),
> I -think-, to be fully correct, you want to use
> nsPromiseFlatString(filePath).get() here to ensure you get a null-terminated
> string.
ns(Auto)String is null-terminated. nsAString needs PromiseFlatString (no ns).

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 377425 [details] [diff] [review]
Patch for nsIconChannel::GetHIconFromFile()

># HG changeset patch
># User kyle@kyle-laptop
># Date 1242317025 14400
># Node ID 32f644221c8c2d5ec197fb5a8961fe5d2dc1a353
># Parent  f677422fdf56910731d45f6ed9430abc1bdb2206
>This patch fixes bug 415761 by making the internal string handling in
>nsIconChannel::GetHIconFromFile() Unicode safe
>
>diff -r f677422fdf56 -r 32f644221c8c modules/libpr0n/decoders/icon/win/nsIconChannel.cpp
>--- a/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp    Thu May 14 16:17:53 2009 +0200
>+++ b/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp    Thu May 14 12:03:45 2009 -0400
>@@ -323,32 +323,33 @@
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   // if the file exists, we are going to use it's real attributes...otherwise we only want to use it for it's extension...
>   SHFILEINFOW      sfi;
>   UINT infoFlags = SHGFI_ICON;
>   
>   PRBool fileExists = PR_FALSE;
>  
>-  nsCAutoString filePath(fileExt);
>+  nsAutoString filePath;
>+  CopyASCIItoUTF16(fileExt, filePath);
>   if (localFile)
>   {
>     rv = localFile->Normalize();
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>-    localFile->GetNativePath(filePath);
>+    localFile->GetPath(filePath);
>     if (filePath.Length() < 2 || filePath[1] != ':')
>       return NS_ERROR_MALFORMED_URI; // UNC
> 
>     if (filePath.Last() == ':')
>       filePath.Append('\\');
>     else {
>       localFile->Exists(&fileExists);
>       if (!fileExists)
>-       localFile->GetNativeLeafName(filePath);
>+       localFile->GetLeafName(filePath);
>     }
>   }
> 
>   if (!fileExists)
>    infoFlags |= SHGFI_USEFILEATTRIBUTES;
> 
>   infoFlags |= GetSizeInfoFlag(desiredImageSize);
> 
>@@ -359,34 +360,34 @@
>     nsCOMPtr<nsIMIMEService> mimeService (do_GetService(NS_MIMESERVICE_CONTRACTID, &rv));
>     NS_ENSURE_SUCCESS(rv, rv);
> 
>     nsCAutoString defFileExt;
>     mimeService->GetPrimaryExtension(contentType, fileExt, defFileExt);
>     // If the mime service does not know about this mime type, we show
>     // the generic icon.
>     // In any case, we need to insert a '.' before the extension.
>-    filePath = NS_LITERAL_CSTRING(".") + defFileExt;
>+    filePath = NS_LITERAL_STRING(".") + NS_ConvertASCIItoUTF16(defFileExt);
>   }
> 
>   // Is this the "Desktop" folder?
>   DWORD shellResult = GetSpecialFolderIcon(localFile, CSIDL_DESKTOP, &sfi, infoFlags);
>   if (!shellResult) {
>     // Is this the "My Documents" folder?
>     shellResult = GetSpecialFolderIcon(localFile, CSIDL_PERSONAL, &sfi, infoFlags);
>   }
> 
>   // There are other "Special Folders" and Namespace entities that we are not 
>   // fetching icons for, see: 
>   // http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/reference/enums/csidl.asp
>   // If we ever need to get them, code to do so would be inserted here. 
> 
>   // Not a special folder, or something else failed above.
>   if (!shellResult)
>-    shellResult = ::SHGetFileInfoW(NS_ConvertUTF8toUTF16(filePath).get(),
>+    shellResult = ::SHGetFileInfoW(filePath.get(),
>                                    FILE_ATTRIBUTE_ARCHIVE, &sfi, sizeof(sfi), infoFlags);
> 
>   if (shellResult && sfi.hIcon)
>     *hIcon = sfi.hIcon;
>   else
>     rv = NS_ERROR_NOT_AVAILABLE;
> 
>   return rv;

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch nsIconChannel::GetHIconFromFile

OK I've cleaned up the string handling per Vladimir and Neil's suggestions.

Comment 24

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 377906 [details] [diff] [review]
Patch nsIconChannel::GetHIconFromFile

Looks fine, thanks; Neil's comments were correct, that's what I get for doing reviews right after waking up :)

Comment 25

[[e-mail]]

(In reply to comment #24)
> Neil's comments were correct, that's what I get for doing reviews right after waking up :)
I have the opposite problem, I do reviews when I should have gone to sleep ;-)

Comment 26

[Serge Gautherie (:sgautherie)]

Comment on attachment 377906 [details] [diff] [review]
Patch nsIconChannel::GetHIconFromFile

>     else {
>       localFile->Exists(&fileExists);
>       if (!fileExists)
>-       localFile->GetNativeLeafName(filePath);
>-    }
>+       localFile->GetLeafName(filePath);
>+	}

Nit: "}" should not move.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch w/nit

Patch that doesn't screw with whitespacing.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/3b4b7d84ee13

Thanks!

Comment 30

[党亮]

How to fix the problem?
It appears at Firefox 3.5, and I don't know what to do.
I have report the bug last day.
https://bugzilla.mozilla.org/show_bug.cgi?id=501958

Comment 31

[pal-moz]

(In reply to comment #30)
> How to fix the problem?
> It appears at Firefox 3.5, and I don't know what to do.
> I have report the bug last day.
> https://bugzilla.mozilla.org/show_bug.cgi?id=501958

patch is not landed on 3.5 branch yet.
so checkin needed

Comment 32

[Martijn Wargers (dead)]

Afaik, this will not be fixed on the 3.5 branch, unless the patch gets approval for it.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 385000 [details] [diff] [review]
Patch w/nit

Nominating for approval on 1.9.1 branch per requests.  

Patch adds Unicode support to nsIconChannel::GetHIconFromFile, resolving a number of visual bugs mostly in the Download Manager that result in the wrong or no icon being displayed when non-ASCII characters are involved on Windows.

There is no anticipated risk; ASCII strings should pass through the modifications with no trouble, so there should be no regressions in functionality or performance.  This has been on trunk for a few days and no complaints have been reported.

Comment 36

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 385000 [details] [diff] [review]
Patch w/nit

Patch looks safe, but we need tests for it before we can consider it.

Comment 38

[[e-mail]]

Attachment: Reftest

Comment 39

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

With the accelerated schedule for 1.9.2 I'm inclined to believe that there is little point in taking this on branch.

Comment 40

[Jeff Walden [:Waldo]]

Comment on attachment 390225 [details] [diff] [review]
Reftest

Rather than adding __LOCATION__ I'd prefer if you used the same methods as this test:

http://mxr.mozilla.org/mozilla-central/source/content/media/test/contentDuration1.sjs

I'm not sure whether I like the idea of __LOCATION__ or not, need to give it more thought; the above method should suffice for now.  With that change all cool, I think.

Comment 41

[[e-mail]]

Comment on attachment 390225 [details] [diff] [review]
Reftest

The whole idea of the review was to approve the creation of __LOCATION__, so if you don't want it then minus the patch.

Comment 43

[[e-mail]]

Attachment: Alternative way of providing the location of the sjs file

Comment 44

[[e-mail]]

Comment on attachment 425459 [details] [diff] [review]
Alternative way of providing the location of the sjs file

Pushed changeset f58bacef76d4 to mozilla-central.