241708 - nsIFile support for unc paths is almost entirely broken

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect)

Description

[timeless]

I'm just going to post a huge series of testcases.

Comment 1

[timeless]

Attachment: nsIFile.parent of unc server throws an exception instead of returning null

Comment 2

[timeless]

Attachment: Make nsIFile.parent eventually reach null and add drive lists

I'll implement more of this stuff later.

This patch gives unc computers a parent of null and lets you reach all local
mapped logical drives by using logicaldrive.parent.directoryEntries.

Comment 3

[Darin Fisher]

Comment on attachment 147055 [details] [diff] [review]
Make nsIFile.parent eventually reach null and add drive lists

>Index: nsLocalFileWin.cpp

>+class nsDriveEnumerator : public nsISimpleEnumerator
>+{
>+public:
>+    nsDriveEnumerator();
>+    virtual ~nsDriveEnumerator();

dtor doens't need to be virtual.  it's called from your Release only.

>+nsresult nsDriveEnumerator::Init()
>+{
>+    DWORD length=GetLogicalDriveStrings(0, 0);

nit: space around '='


>+    mDrives.SetLength(length+1);
>+    if (!GetLogicalDriveStrings(length, mDrives.BeginWriting()))
>+        return NS_ERROR_FAILURE;

please add a comment explaining how |length| is used so the reader
doesn't have to visit MSDN to make sense of the |length+1| business.


>+NS_IMETHODIMP nsDriveEnumerator::HasMoreElements(PRBool *aHasMore)
>+{
>+    *aHasMore = !!*mLetter;

this reads better:

      *aHasMore = (*mLetter != '\0');


>+NS_IMETHODIMP nsDriveEnumerator::GetNext(nsISupports **aNext)
>+{
>+    if (!*mLetter) {
>+        *aNext = nsnull;
>+        return NS_OK;
>+    }
>+    const char *drive = mLetter;
>+    mLetter += strlen(drive) + 1;
>+    
>+    nsCOMPtr<nsILocalFile> localFile;
>+    nsresult rv =
>+        NS_NewNativeLocalFile(
>+           nsDependentCString(drive), PR_TRUE, getter_AddRefs(localFile));

is there really any value to passing PR_TRUE for resolveLinks?
drive letters can't be links (.lnk), so perhaps PR_FALSE would
be just fine.


>+    if(NS_SUCCEEDED(rv) && localFile)
>+    {
>+        return CallQueryInterface(localFile, aNext);
>+    }
>+    return rv;

This QI is not needed.	localFile is already a nsISupports
instance.  Also, you can assume that NS_SUCCEEDED(rv) implies
localFile != NULL.  Just do this:

   if (NS_SUCCEEDED(rv))
       NS_ADDREF(*aNext = localFile);
   return rv;


sr=me with these nits picked.

Comment 4

[timeless]

Comment on attachment 147055 [details] [diff] [review]
Make nsIFile.parent eventually reach null and add drive lists

mozilla/xpcom/io/nsLocalFileWin.cpp	1.118

\\. doesn't exist (which is ok).
unfortunately contains isn't implemented for it.

file:///c:/ will now show a parent, which is ok.
unfortunately that parent is listed as file:///// and doesn't do the right
thing (i'll work on that at some point - i believe the problem lies in
nsBaseURLParser::ParseFilePath).

Comment 5

[timeless]

unrelated to my patch, the parent of file://///127.0.0.1/c$ doesn't work.

steps:
1. edit>preferences>debug>networking
2. (*) html)
3. ok
4. load file://///127.0.0.1/c$
5. click Up to higher level directory
5' The file ///127.0.0.1 cannot be found. Please check the location and try again.
5` should get a share index.

\\servername needs to return true from isDirectory(), it doesn't seem to need to
need to return true for exists().

Comment 6

[David :Bienvenu]

I suspect this change has caused some regressions, e.g., bug 242579 - backing
out the checked in patch fixes some strange uri's I was seeing,and failure
copying multiple messages. It wouldn't surprise me if bug 243161 was also caused
by this change.

Comment 7

[timeless]

Attachment: Change getRelativeDescriptor to return "" for equivalent files and throw not available for files which are not contained

Comment 8

[Darin Fisher]

Comment on attachment 148191 [details] [diff] [review]
Change getRelativeDescriptor to return "" for equivalent files and throw not available for files which are not contained

why is the contains check needed?  is that just an optimization?  why wouldn't
the code below give the same result?  shouldn't it?  what am i missing?

Comment 9

[timeless]

the code that builds paths always ends up with:

\\.\c:\whatever
\\.\d:\somethingelse

the problem is that it decides that c: and d: are related animals. which they 
aren't. contains says: c: does not contain d: so don't do something silly.

Comment 10

[David :Bienvenu]

Is this going to fix all the horrible mail problems the original unc path patch
is causing?

Comment 11

[timeless]

i expect so. but it won't if it doesn't get a review and checked in.

Comment 12

[David :Bienvenu]

time is getting very short for 1.8a. If this doesn't get fixed real soon, I
think we'll have to temporarily back out the patch that broke mailnews for 1.8a.

Comment 13

[David :Bienvenu]

I have this patch applied and it doesn't fix the problem, from what I can see.

Comment 14

[David :Bienvenu]

Attachment: backout the part that breaks mail

Comment 15

[(not reading, please use [e-mail] instead)]

Comment on attachment 148953 [details] [diff] [review]
backout the part that breaks mail

r/sr=sspitzer

david tells me he is adding back two lines timeless removed.

david, can you add a comment that warns against removing the
check-offset-return code, and how it will bust mailnews?

thanks for fixing this.

Comment 16

[(not reading, please use [e-mail] instead)]

Comment on attachment 148953 [details] [diff] [review]
backout the part that breaks mail

a=sspitzer for 1.8a, david tells me this is not a problem for 1.7

Comment 17

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a1) Gecko/20040520
My mail directory is on drive D: and holds one directory for a POP3 account and
another one for Local Folders. 
BuildID 2004052013 gave me access to the mail account directory, but I still
lack access to Local Folders.

2004-05-20 10:46	bienvenu%nventure.com 	mozilla/ xpcom/ io/ nsLocalFileWin.cpp 
1.121 	7/0  	back out part of patch in bug 241708 that breaks mailnews dirs on a
different drive, r/sr/a=sspitzer

Comment 18

[Scott MacGregor]

*** Bug 244294 has been marked as a duplicate of this bug. ***

Comment 19

[Darin Fisher]

Comment on attachment 148191 [details] [diff] [review]
Change getRelativeDescriptor to return "" for equivalent files and throw not available for files which are not contained

minusing based on bienvenu's comments that this didn't fix the mailnews
problem.  if this patch is still valid, pleaes explain and re-request r=

Comment 20

[Benjamin Smedberg]

Somebody (Waldo?) really fixed this later.