Closed Bug 415761 Opened 12 years ago Closed 11 years ago

nsIconChannel::GetHIconFromFile does not support Unicode filenames on Windows

Categories

(Core :: ImageLib, defect, P4)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: martijn.martijn, Assigned: khuey)

References

()

Details

Attachments

(4 files, 3 obsolete files)

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.
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Is this a regression from 2.0?  This probably has something to do with the moz-icon code...
Whiteboard: [needs assignee]
Priority: P3 → P4
Simon - might this also be related to bug 393488, or am I barking up the wrong tree?
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
The icon code still uses ANSI APIs. It probably needs to use the Unicode APIs.
Rob - you wanted a bug (see comment 4)?
Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.
Flags: wanted-firefox3+
Flags: blocking-firefox3-
Flags: blocking-firefox3+
Product: Firefox → Toolkit
Confirmed on Vista as well.  I'm going to play with this one.
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.
Component: Download Manager → ImageLib
Product: Toolkit → Core
QA Contact: download.manager → imagelib
Assignee: nobody → me
Summary: No icon for the saved file in the download manager when the filename contains unicode character → nsIconChannel::GetHIconFromFile does not support Unicode filenames on Windows
(In reply to comment #8)

Done; thanks!
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.
Attachment #377416 - Flags: review?
Attachment #377416 - Flags: review? → review?(pavlov)
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.
Status: NEW → ASSIGNED
Whiteboard: [needs assignee]
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)
Code same as before, patch is slightly different per Edward's comment.
Attachment #377416 - Attachment is obsolete: true
Attachment #377425 - Flags: review?
Attachment #377416 - Flags: review?(pavlov)
Attachment #377425 - Flags: review? → review?(pavlov)
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 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. :)
Attachment #377425 - Flags: review?(pavlov) → review+
Thanks Joe.  I'll post a slightly cleaned up version before I ask for superreview.
Attachment #377425 - Attachment is obsolete: true
Attachment #377847 - Flags: superreview?
Attachment #377847 - Flags: review+
Attachment #377847 - Flags: superreview? → superreview?(tor)
(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?
Attachment #377847 - Flags: superreview?(tor) → superreview?(vladimir)
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 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.
(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 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;
Attachment #377425 - Attachment is obsolete: false
Attachment #377847 - Attachment is obsolete: true
Attachment #377847 - Flags: superreview?(vladimir)
OK I've cleaned up the string handling per Vladimir and Neil's suggestions.
Attachment #377906 - Flags: superreview?(vladimir)
Attachment #377906 - Flags: review+
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 :)
Attachment #377906 - Flags: superreview?(vladimir) → superreview+
Whiteboard: [needs landing]
(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 ;-)
Keywords: checkin-needed
Whiteboard: [needs landing]
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.
Keywords: checkin-needed
Whiteboard: [c-n: needs new patch]
Attached patch Patch w/nitSplinter Review
Patch that doesn't screw with whitespacing.
Whiteboard: [c-n: needs new patch] → checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 454247
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
(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
Afaik, this will not be fixed on the 3.5 branch, unless the patch gets approval for it.
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.
Attachment #385000 - Flags: approval1.9.1?
Duplicate of this bug: 502845
Attachment #385000 - Flags: approval1.9.1? → approval1.9.1.1?
Duplicate of this bug: 504350
Comment on attachment 385000 [details] [diff] [review]
Patch w/nit

Patch looks safe, but we need tests for it before we can consider it.
Attachment #385000 - Flags: approval1.9.1.1? → approval1.9.1.1-
Flags: in-testsuite?
Duplicate of this bug: 505913
Attached patch ReftestSplinter Review
Attachment #390225 - Flags: review?(jwalden+bmo)
With the accelerated schedule for 1.9.2 I'm inclined to believe that there is little point in taking this on branch.
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.
Attachment #390225 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #390225 - Flags: review+ → review-
Target Milestone: --- → mozilla1.9.2a1
Duplicate of this bug: 518451
Attachment #425459 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 425459 [details] [diff] [review]
Alternative way of providing the location of the sjs file

Pushed changeset f58bacef76d4 to mozilla-central.
Depends on: 553874
Flags: in-testsuite? → in-testsuite+
Duplicate of this bug: 526163
Duplicate of this bug: 526973
Duplicate of this bug: 572324
You need to log in before you can comment on or make changes to this bug.