Closed Bug 28784 Opened 24 years ago Closed 23 years ago

cannot type non ASCII name in url bar to access file

Categories

(Core :: Internationalization, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla1.0.1

People

(Reporter: ftang, Assigned: chak)

References

Details

(Keywords: intl, regression, Whiteboard: [PDT+][nsbeta3-])

Attachments

(1 file)

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
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
Target Milestone: M14
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.
I can make this a necko utility if you want.
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...
Putting on PDT- radar for beta1.  Too risky.
Whiteboard: have fix need review → [PDT-]have fix need review
reassign to warren since he want to make it into necko utility.
Assignee: ftang → warren
Status: ASSIGNED → NEW
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
Let us know how risky this is once reviewed.
per PDT, need info on risk
Whiteboard: have fix need review → [NEED INFO] have fix need review
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
Status: NEW → ASSIGNED
Whiteboard: [NEED INFO] have fix need review → schedule to check in 2/28 night
Target Milestone: M14 → M15
ok I will check in tonight.
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.
Putting on PDT+ radar for beta1.
Whiteboard: schedule to check in 2/28 night → [PDT+]schedule to check in 2/28 night
fix and check in
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
I verified in 2000022909 build.
Status: RESOLVED → VERIFIED
I spoke soon.  I cannot verify this on mac until bug 31054 is fixed.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
I changed this as Fixed.
Status: REOPENED → RESOLVED
Closed: 24 years ago24 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
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
I tested this in 2000-07-25-08. This does not work. I reopen this.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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+]
this is regression from nsbeta1. mark it as nsbeta3+ per i18n bug meeting.
Keywords: regression
Whiteboard: [PDT+] → [PDT+][nsbeta3+]
Whiteboard: [PDT+][nsbeta3+] → [PDT+][nsbeta3+]ETA:8/11
Blocks: 29698
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+]
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.
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.
seems like these changes should occur in the docshell.
Assigning to rpotts. 
Assignee: gagan → rpotts
minusing per PDT rules.
Whiteboard: [PDT+][nsbeta3+] → [PDT+][nsbeta3-]
Keywords: beta1nsbeta1
Keywords: intl
Changed QA contact to ylong@netscape.com.
QA Contact: teruko → ylong
Blocks: 63736
Clearing very old milestone (M15) in hope of reevaluation.
Target Milestone: M15 → ---
Target Milestone: --- → mozilla1.0.1
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
->chak
Assignee: rpotts → chak
Chak - Please reconsider for earlier milestone (M0.9.1), and clean up the the
Status Whiteboard.
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

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.
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     }
     ...
Not sure. Cc:ing Adam Lock since he knows a bit more about the FixupUri service.
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
Chak, this is probably a dup or dependent on bug 75063.
looks right to me. nhotta: can you verify this one and put r= if it is ok?
sorry for my delay.
frank, you're the one that checked that code in; now you want it out?
Jud : I'm guessing you're thinking of bug 66515? (the one where Frank made a 
change to call ToNewUTF8String() instead of ToNewCString())
chak's right. I'm a moron. please move along.
r=nhotta
sr=blizzard
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.
Apologies, I meant bug 75063 :)
Fix checked in.
Status: NEW → RESOLVED
Closed: 24 years ago23 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: