cannot type non ASCII name in url bar to access file

VERIFIED FIXED in mozilla1.0.1

Status

()

Core
Internationalization
P3
normal
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: Frank Tang, Assigned: Chak Nanga)

Tracking

({intl, regression})

Trunk
mozilla1.0.1
intl, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PDT+][nsbeta3-])

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
This is a cross platform bug. Although I use Japanese NT as the reproduce platform, it have the same problem across all platform. I create this bug to track file: specific problem from 14155. IT also depend on 28171 to make this work

how to reproduce
1. create some non ASCII file name (for example, in Japanese system, create folder c:\日本語 and create a file called 本田.txt
2. In 4.x, if you type c\日本語\本田.txt in the url bar, it will access to that file, in SeaMonkey, it won't
(Reporter)

Comment 1

18 years ago
I think this shoudl be beta1. non ASCII file name is very common. On  Mac, even English user use a lot of non ascill file path.
The fix patch is as below (part of them have already reviewed by valeski in 28171)
Index: nsWebShell.cpp
===================================================================
RCS file: /m/pub/mozilla/webshell/src/nsWebShell.cpp,v
retrieving revision 1.396
diff -c -r1.396 nsWebShell.cpp
*** nsWebShell.cpp      2000/02/19 02:54:32     1.396
--- nsWebShell.cpp      2000/02/22 07:12:30
***************
*** 87,92 ****
--- 87,95 ----
  #include "nsIDocShellTreeOwner.h"
  #include "nsCURILoader.h"
  #include "nsIDOMWindow.h"
+ #include "nsEscape.h"
+ #include "nsIPlatformCharset.h"
+ #include "nsICharsetConverterManager.h"
  
  #include "nsIHTTPChannel.h" // add this to the ick include list...we need it to QI for post data interface
  #include "nsHTTPEnums.h"
***************
*** 97,102 ****
--- 100,107 ----
  static NS_DEFINE_CID(kLocaleServiceCID, NS_LOCALESERVICE_CID);
  static NS_DEFINE_CID(kPrefServiceCID, NS_PREF_CID);
  static NS_DEFINE_CID(kCStringBundleServiceCID,  NS_STRINGBUNDLESERVICE_CID);
+ static NS_DEFINE_CID(kPlatformCharsetCID,  NS_PLATFORMCHARSET_CID);
+ static NS_DEFINE_CID(kCharsetConverterManagerCID,  NS_ICHARSETCONVERTERMANAGER_CID);
  
  
  #ifdef XP_PC
***************
*** 1375,1419 ****
  
  static void convertFileToURL(const nsString &aIn, nsString &aOut)
  {
!   char szFile[1000];
!   aIn.ToCString(szFile, sizeof(szFile));
  #ifdef XP_PC
    // Check for \ in the url-string (PC)
!   if (PL_strchr(szFile, '\\')) {
  #else
  #if XP_UNIX
    // Check if it starts with / or \ (UNIX)
!   if (*(szFile) == '/' || *(szFile) == '\\') {
  #else
    if (0) {  
    // Do nothing (All others for now) 
  #endif
  #endif
-     PRInt32 len = strlen(szFile);
-     PRInt32 sum = len + sizeof(FILE_PROTOCOL);
-     char* lpszFileURL = (char *)PR_Malloc(sum + 1);
  
  #ifdef XP_PC
      // Translate '\' to '/'
!     for (PRInt32 i = 0; i < len; i++) {
!       if (szFile[i] == '\\') {
!         szFile[i] = '/';
!       }
!       if (szFile[i] == ':') {
!         szFile[i] = '|';
!       }
!     }
  #endif
  
      // Build the file URL
!     PR_snprintf(lpszFileURL, sum, "%s%s", FILE_PROTOCOL, szFile);
!     aOut = lpszFileURL;
!     PR_Free((void *)lpszFileURL);
    }
-   else
-   {
-     aOut = aIn;
-   }
  }
  
  NS_IMETHODIMP
--- 1380,1410 ----
  
  static void convertFileToURL(const nsString &aIn, nsString &aOut)
  {
!   aOut = aIn;
  #ifdef XP_PC
    // Check for \ in the url-string (PC)
!   if(kNotFound != aIn.FindChar(PRUnichar('\\'))) {
  #else
  #if XP_UNIX
    // Check if it starts with / or \ (UNIX)
!   const PRUnichar * up = aIn.GetUnicode();
!   if((PRUnichar('/') == *up) ||
!      (PRUnichar('\\') == *up)) {
  #else
    if (0) {  
    // Do nothing (All others for now) 
  #endif
  #endif
  
  #ifdef XP_PC
      // Translate '\' to '/'
!     aOut.ReplaceChar(PRUnichar('\\'), PRUnichar('/'));
!     aOut.ReplaceChar(PRUnichar(':'), PRUnichar('|'));
  #endif
  
      // Build the file URL
!     aOut.Insert(FILE_PROTOCOL,0);
    }
  }
  
  NS_IMETHODIMP
***************
*** 1971,1976 ****
--- 1962,2009 ----
    return rv;
  }
  
+ 
+ static nsresult convertURLToFileCharset(nsString& aIn, nsCString& aOut)
+ {
+     nsresult rv=NS_OK;
+     aOut = "";
+     // for file url, we need to convert the nsString to the file system
+     // charset before we pass to NS_NewURI
+     static nsAutoString fsCharset("");
+     // find out the file system charset first
+     if(0 == fsCharset.Length()) {
+        fsCharset = "ISO-8859-1"; // set the fallback first.
+        NS_WITH_SERVICE(nsIPlatformCharset, plat, kPlatformCharsetCID, &rv);
+        NS_ASSERTION(NS_SUCCEEDED(rv), "cannot get nsIPlatformCharset");
+        if(NS_SUCCEEDED(rv)) {
+          rv = plat->GetCharset(kPlatformCharsetSel_FileName, fsCharset);
+          NS_ASSERTION(NS_SUCCEEDED(rv), "nsIPlatformCharset GetCharset failed");
+        }
+     }
+     // We probably should cache ccm here.
+     // get a charset converter from the manager
+     NS_WITH_SERVICE(nsICharsetConverterManager, ccm, kCharsetConverterManagerCID, &rv);
+     NS_ASSERTION(NS_SUCCEEDED(rv), "cannot get CharsetConverterManager");
+     nsCOMPtr<nsIUnicodeEncoder> fsEncoder;
+     if(NS_SUCCEEDED(rv)){
+        rv = ccm->GetUnicodeEncoder(&fsCharset, getter_AddRefs(fsEncoder));
+        NS_ASSERTION(NS_SUCCEEDED(rv), "cannot get encoder");
+        if(NS_SUCCEEDED(rv)){
+           PRInt32 bufLen = 0;
+           rv = fsEncoder->GetMaxLength(aIn.GetUnicode(), aIn.Length(), &bufLen);
+           NS_ASSERTION(NS_SUCCEEDED(rv), "GetMaxLength failed");
+           aOut.SetCapacity(bufLen+1);
+           PRInt32 srclen = aIn.Length();
+           rv = fsEncoder->Convert(aIn.GetUnicode(), &srclen, 
+                     (char*)aOut.GetBuffer(), &bufLen);
+           if(NS_SUCCEEDED(rv)) {
+              ((char*)aOut.GetBuffer())[bufLen]='\0';
+              aOut.SetLength(bufLen);
+           }
+        }
+     }
+     return rv;
+ }
  NS_IMETHODIMP
  nsWebShell::LoadURL(const PRUnichar *aURLSpec,
                      const char* aCommand,
***************
*** 1996,2002 ****
    // first things first. try to create a uri out of the string.
    nsCOMPtr<nsIURI> uri;
    nsXPIDLCString  spec;
!   rv = NS_NewURI(getter_AddRefs(uri), urlStr, nsnull);
    if (NS_FAILED(rv)) {
      // no dice.
      nsAutoString urlSpec;
--- 2029,2045 ----
    // first things first. try to create a uri out of the string.
    nsCOMPtr<nsIURI> uri;
    nsXPIDLCString  spec;
!   if(0==urlStr.Find("file:",0)) {
!     // if this is file url, we need to  convert the URI
!     // from Unicode to the FS charset
!     nsCAutoString inFSCharset;
!     rv = convertURLToFileCharset(urlStr, inFSCharset);
!     NS_ASSERTION(NS_SUCCEEDED(rv), "convertURLToFielCharset failed");
!     if (NS_SUCCEEDED(rv)) 
!       rv = NS_NewURI(getter_AddRefs(uri), inFSCharset.GetBuffer(), nsnull);
!   } else {
!     rv = NS_NewURI(getter_AddRefs(uri), urlStr, nsnull);
!   }
    if (NS_FAILED(rv)) {
      // no dice.
      nsAutoString urlSpec;
***************
*** 2004,2010 ****
  
      // see if we've got a file url.
      convertFileToURL(urlStr, urlSpec);
!     rv = NS_NewURI(getter_AddRefs(uri), urlSpec, nsnull);
      if (NS_FAILED(rv)) {
        NS_ASSERTION(mPrefs, "the webshell's pref service wasn't initialized");
  
--- 2047,2058 ----
  
      // see if we've got a file url.
      convertFileToURL(urlStr, urlSpec);
!     nsCAutoString inFSCharset;
!     rv = convertURLToFileCharset(urlSpec, inFSCharset);
!     NS_ASSERTION(NS_SUCCEEDED(rv), "convertURLToFielCharset failed");
!     if (NS_SUCCEEDED(rv)) 
!       rv = NS_NewURI(getter_AddRefs(uri), inFSCharset.GetBuffer(), nsnull);
! 
      if (NS_FAILED(rv)) {
        NS_ASSERTION(mPrefs, "the webshell's pref service wasn't initialized");
  

travis- can  you review it ?
Blocks: 14155
Status: NEW → ASSIGNED
Depends on: 28171
Keywords: beta1
Whiteboard: have fix need review
(Reporter)

Updated

18 years ago
Target Milestone: M14

Comment 2

18 years ago
I still don't see why this work isn't done in the necko utils.  Doing all this 
work in webshell seems a waste since it means these same bugs show up any other 
place we accept user URLS.

Comment 3

18 years ago
I can make this a necko utility if you want.

Comment 4

18 years ago
I had some old code in my tree to attach this as a method to the necko service.
I can send you the patch if you want and you can move it to nsNeckoUtil if you
are interested...

Comment 5

18 years ago
Putting on PDT- radar for beta1.  Too risky.
Whiteboard: have fix need review → [PDT-]have fix need review
(Reporter)

Comment 6

18 years ago
reassign to warren since he want to make it into necko utility.
Assignee: ftang → warren
Status: ASSIGNED → NEW

Comment 7

18 years ago
Cleared PDT-. 
Let's have this code reviewed before declaring this too risky (which should
not be the sole factor on PDT+/- decisions anyway).

Correcting the string handling should not be that risky.  That is what this
bug is about.  If post-bet1, we want to move code around between webshell and
necko, that is a different issue.  We just want to correct the bad code.

As Frank points out in earlier comment, this bug affects to prevents using
non-ASCII file URLs.  Besides breaking Japanese users, this even breaks many
English users on Mac since it is common to use non-ASCII in things like the
 end of Mac folder names.
Whiteboard: [PDT-]have fix need review → have fix need review

Comment 8

18 years ago
Let us know how risky this is once reviewed.

Comment 9

18 years ago
per PDT, need info on risk
Whiteboard: have fix need review → [NEED INFO] have fix need review

Comment 10

18 years ago
I think you should check this in to the webshell for now. Later we should make 
it a necko utility if that makes sense. Issues are:

- nsStdURL has GetFile/SetFile which convert file: urls to and from nsIFile. 
Given that you can get to and from a native path from an nsIFile, this should be 
sufficient.

- nsStdURL currently doesn't handle all the escaping in the platform (file 
system) encoding as Frank says it has to. We should file a separate bug to fix 
that. Cc'ing Andreas.

- My first thought was to make convertFileToURL and convertURLToFileCharset be 
utilities on nsIIOService. Maybe that still would be a good thing to do, but 
given that nsIIOService is an IDL interface, some reworking would need to be 
done to these routines to make them use PRUnichar* instead of nsStrings. I'm 
hoping that fixing up nsStdURL will be sufficient though.

Giving bug back to Frank for checkin.
Assignee: warren → ftang
(Reporter)

Updated

18 years ago
Status: NEW → ASSIGNED
Whiteboard: [NEED INFO] have fix need review → schedule to check in 2/28 night
(Reporter)

Updated

18 years ago
Target Milestone: M14 → M15
(Reporter)

Comment 11

18 years ago
ok I will check in tonight.

Comment 12

18 years ago
Inter-related and depended upon by 2 other PDT+ bugs which have been reviewed
by warren and approved by me: bug 28171 and bug 14155.

Comment 13

18 years ago
Putting on PDT+ radar for beta1.
Whiteboard: schedule to check in 2/28 night → [PDT+]schedule to check in 2/28 night
(Reporter)

Comment 14

18 years ago
fix and check in
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 15

18 years ago
I verified in 2000022909 build.
Status: RESOLVED → VERIFIED

Comment 16

18 years ago
I spoke soon.  I cannot verify this on mac until bug 31054 is fixed.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 17

18 years ago
I changed this as Fixed.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → FIXED
Whiteboard: [PDT+]schedule to check in 2/28 night → [PDT+]schedule to check in 2/28 night,cannot verify this on mac until bug 31054 is fixed

Comment 18

18 years ago
Ok, we found that if I put the file name extention.  It works file on Mac.  I verified this in 2000030809 Mac build.
Status: RESOLVED → VERIFIED

Comment 19

18 years ago
I tested this in 2000-07-25-08. This does not work. I reopen this.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 20

18 years ago
mark it as nsbeta3 after reopen
Status: REOPENED → ASSIGNED
Keywords: nsbeta3
Whiteboard: [PDT+]schedule to check in 2/28 night,cannot verify this on mac until bug 31054 is fixed → [PDT+]
(Reporter)

Comment 21

18 years ago
this is regression from nsbeta1. mark it as nsbeta3+ per i18n bug meeting.
Keywords: regression
Whiteboard: [PDT+] → [PDT+][nsbeta3+]
(Reporter)

Updated

18 years ago
Whiteboard: [PDT+][nsbeta3+] → [PDT+][nsbeta3+]ETA:8/11

Updated

17 years ago
Blocks: 29698
(Reporter)

Comment 22

17 years ago
The reason this is failing is because in docshell/base/nsDocShell.cpp the 
nsDocShell::CreateFixupURI does not execute FileURIFixup while it should. The 
first NS_NewURI always success even while it should not. The FileURIFixup should 
do the proper conversion .

2784 NS_IMETHODIMP nsDocShell::CreateFixupURI(const PRUnichar* aStringURI, 
2785    nsIURI** aURI)
2786 {
2787    *aURI = nsnull;
2788    nsAutoString uriString(aStringURI);
2789    uriString.Trim(" ");  // Cleanup the empty spaces that might be on each 
end.
2790 
2791    // Just try to create an URL out of it
2792    NS_NewURI(aURI, uriString.GetUnicode(), nsnull);
2793    if(*aURI)
2794       return NS_OK;
2795 
2796    // Check for if it is a file URL
2797    FileURIFixup(uriString.GetUnicode(), aURI);
2798    if(*aURI)
2799       return NS_OK;

Reassign to Gagan. Gagan, it seems tbogard@aol.net 's origional code assume that 
the first NS_NewURI will failed if the file does not exist. Could you take a 
look at it ? Why the NS_NewURI return true even the file does not exist ? 
Assignee: ftang → gagan
Status: ASSIGNED → NEW
Whiteboard: [PDT+][nsbeta3+]ETA:8/11 → [PDT+][nsbeta3+]

Comment 23

17 years ago
In general, this is the wrong way to write this stuff. If something fails, 
return NS_OK? That hides serious failures (out of memory, etc) from expected 
failures. You should return the failure, and let the caller decide 
whether that specific error code is ok. Something like this would be better:

2791    // Just try to create an URL out of it
2792    rv = NS_NewURI(aURI, uriString.GetUnicode(), nsnull);
2793    if (NS_FAILED(rv))
2794       return rv;
2795 
2796    // Check for if it is a file URL
2797    rv = FileURIFixup(uriString.GetUnicode(), aURI);
2798    if (NS_FAILED(rv))
2799       return rv;

You can add assertions that the aURI isn't null, but it should never happen in 
a correctly functioning system that you'd return NS_OK and a null aURI.

Furthermore, munging things in place (aURI) is generally a bad idea. Better to 
return a new thing. That's less error prone for someone coming along later to 
change things. There's no visual indication that aURI is getting munged.

Comment 24

17 years ago
NS_NewURI does not check if a file exists, it does also not check if a host
exists, or something like that. That is totaly out of scope for that function
that only does a parse and checks the syntax. This again brings up an old issue
that I have noted on several bugs before. The current fixup strategy for uris is
a dead end in my opinion, we nead something much more centralized  (but
extensible) which can also act on return codes from the loads and try different
fallbacks depending on who wants the url load. / <-> \ conversion is another of
these issues. Currently we have only bits and pieces all over the place.

Comment 25

17 years ago
seems like these changes should occur in the docshell.
Assigning to rpotts. 
Assignee: gagan → rpotts

Comment 26

17 years ago
minusing per PDT rules.
Whiteboard: [PDT+][nsbeta3+] → [PDT+][nsbeta3-]

Updated

17 years ago
Keywords: beta1 → nsbeta1

Updated

17 years ago
Keywords: intl

Comment 27

17 years ago
Changed QA contact to ylong@netscape.com.
QA Contact: teruko → ylong

Updated

17 years ago
Blocks: 63736

Comment 28

17 years ago
Clearing very old milestone (M15) in hope of reevaluation.
Target Milestone: M15 → ---

Updated

17 years ago
Target Milestone: --- → mozilla1.0.1

Comment 29

17 years ago
Please reconsider this for an earlier Target Milestone.

Not only is this very bad for non-English users because non-ASCII file names
are very common, it is also very bad for English Mac users (as ftang
commented on 2000-02-21 23:28) because a non-ASCII character (script 'f')
is used very frequently to terminate folder names.

Added 4xp keyword.
Keywords: 4xp
(Assignee)

Comment 30

17 years ago
->chak
Assignee: rpotts → chak

Comment 31

17 years ago
Chak - Please reconsider for earlier milestone (M0.9.1), and clean up the the
Status Whiteboard.
(Assignee)

Comment 32

17 years ago
A lot of stuff has changed and/or moved aroubd since this bug was filed (over 
a year ago) so many of the comments in this bug are obviously outdated.

Since i do not have an intl' version of an OS here's how i tested/reproduced 
this on my English Windows NT 4.0 box:

1. Created a file named дкожэ.txt in my c:\temp dir
2. Typed "file:///C|/TEMP/дкожэ.txt in the URL bar
3. Got a ton of asserts and the URL failed to load.

I'd appreciate if someone can tell me if this is a correct test/reproduction for 
this bug.....Thanks

(Assignee)

Comment 33

17 years ago
Created attachment 32910 [details] [diff] [review]
Patch to be able to access local files with non-ascii chars in their names
(Assignee)

Comment 34

17 years ago
Please note that in the above patch all i'm doing is to move the call to 
FileURIFixup() before the call to NS_NewURI()
So, in the case of a file: url we first go thru' the FileURIFixup() code which 
does the filename char conversions properly for a file with non-ascii chars in 
its name.

Comment 35

17 years ago
How does nsDefaultURIFixup::CreateFixupURI() relate to 
nsDocShell::CreateFixupURI()?

In nsDocShell::CreateFixupURI(), similar code was rewrittin to call a global 
service.  

See http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#3405

     3405 NS_IMETHODIMP
     3406 nsDocShell::CreateFixupURI(const PRUnichar * 
     aStringURI, nsIURI ** aURI)
     3407 {
     ... 
     3415     // Create the fixup object if necessary
     3416     if (!mURIFixup) {
     3417         mURIFixup = 
     do_GetService(NS_URIFIXUP_CONTRACTID);
     3418         if (!mURIFixup) {
     3419             // No fixup service so try and create a URI 
     and see what happens
     3420             return NS_NewURI(aURI, uriString, nsnull);
     3421         }
     3422     }
     ...
(Assignee)

Comment 36

17 years ago
Not sure. Cc:ing Adam Lock since he knows a bit more about the FixupUri service.
(Assignee)

Comment 37

17 years ago
Bob :I think i understand your question now.

nsDocShell::CreateFixupURI() actually calls nsDefaultURIFixup::CreateFixupURI()
as shown in the stack trace below:

nsDefaultURIFixup::CreateFixupURI(nsDefaultURIFixup * const 0x015832f0, const 
unsigned short * 0x0012fe0c, nsIURI * * 0x0012fdb8) line 63
nsDocShell::CreateFixupURI(nsDocShell * const 0x0154c930, const unsigned short * 
0x0012fe0c, nsIURI * * 0x0012fdb8) line 3425 + 46 bytes
nsDocShell::LoadURI(nsDocShell * const 0x0154c940, const unsigned short * 
0x0012fe0c, unsigned int 0) line 1663 + 50 bytes
nsWebBrowser::LoadURI(nsWebBrowser * const 0x01548764, const unsigned short * 
0x0012fe0c, unsigned int 0) line 534

Comment 38

17 years ago
Chak, this is probably a dup or dependent on bug 75063.
(Reporter)

Comment 39

17 years ago
looks right to me. nhotta: can you verify this one and put r= if it is ok?
sorry for my delay.

Comment 40

17 years ago
frank, you're the one that checked that code in; now you want it out?
(Assignee)

Comment 41

17 years ago
Jud : I'm guessing you're thinking of bug 66515? (the one where Frank made a 
change to call ToNewUTF8String() instead of ToNewCString())

Comment 42

17 years ago
chak's right. I'm a moron. please move along.

Comment 43

17 years ago
r=nhotta
sr=blizzard

Comment 45

17 years ago
Just to confirm, putting NS_NewURI after file fixup shouldn't pose problems. 
However, the changes I'm doing to bug 66515 may have localization issues, so 
anyone interested in ensuring my changes don't break non ASCII file names should 
CC themselves to that bug.

Comment 46

17 years ago
Apologies, I meant bug 75063 :)
(Assignee)

Comment 47

17 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 18 years ago17 years ago
Resolution: --- → FIXED

Comment 48

17 years ago
Fixed verified on 05-11 trunk build on all platforms.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.