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)
Core
Internationalization
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
Reporter | ||
Comment 1•24 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 ?
Reporter | ||
Updated•24 years ago
|
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.
Comment 3•24 years ago
|
||
I can make this a necko utility if you want.
Comment 4•24 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...
Putting on PDT- radar for beta1. Too risky.
Whiteboard: have fix need review → [PDT-]have fix need review
Reporter | ||
Comment 6•24 years ago
|
||
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
Comment 9•24 years ago
|
||
per PDT, need info on risk
Whiteboard: have fix need review → [NEED INFO] have fix need review
Comment 10•24 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•24 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [NEED INFO] have fix need review → schedule to check in 2/28 night
Reporter | ||
Updated•24 years ago
|
Target Milestone: M14 → M15
Reporter | ||
Comment 11•24 years ago
|
||
ok I will check in tonight.
Comment 12•24 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•24 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•24 years ago
|
||
fix and check in
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 16•24 years ago
|
||
I spoke soon. I cannot verify this on mac until bug 31054 is fixed.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Comment 17•24 years ago
|
||
I changed this as Fixed.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 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•24 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•24 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•24 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•24 years ago
|
||
this is regression from nsbeta1. mark it as nsbeta3+ per i18n bug meeting.
Keywords: regression
Whiteboard: [PDT+] → [PDT+][nsbeta3+]
Reporter | ||
Updated•24 years ago
|
Whiteboard: [PDT+][nsbeta3+] → [PDT+][nsbeta3+]ETA:8/11
Reporter | ||
Comment 22•24 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•24 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•24 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•24 years ago
|
||
seems like these changes should occur in the docshell. Assigning to rpotts.
Assignee: gagan → rpotts
Updated•24 years ago
|
Comment 28•23 years ago
|
||
Clearing very old milestone (M15) in hope of reevaluation.
Target Milestone: M15 → ---
Updated•23 years ago
|
Target Milestone: --- → mozilla1.0.1
Comment 29•23 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
Comment 31•23 years ago
|
||
Chak - Please reconsider for earlier milestone (M0.9.1), and clean up the the Status Whiteboard.
Assignee | ||
Comment 32•23 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•23 years ago
|
||
Assignee | ||
Comment 34•23 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•23 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•23 years ago
|
||
Not sure. Cc:ing Adam Lock since he knows a bit more about the FixupUri service.
Assignee | ||
Comment 37•23 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•23 years ago
|
||
Chak, this is probably a dup or dependent on bug 75063.
Reporter | ||
Comment 39•23 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•23 years ago
|
||
frank, you're the one that checked that code in; now you want it out?
Assignee | ||
Comment 41•23 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•23 years ago
|
||
chak's right. I'm a moron. please move along.
Comment 43•23 years ago
|
||
r=nhotta
Comment 44•23 years ago
|
||
sr=blizzard
Comment 45•23 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•23 years ago
|
||
Apologies, I meant bug 75063 :)
Assignee | ||
Comment 47•23 years ago
|
||
Fix checked in.
Status: NEW → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
Comment 48•23 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.
Description
•