57995 - Make nsIFile followLinks work correctly everywhere

Status: NEW

(Core :: XPCOM, defect, P3)

Description

[Brian Ryner (not reading)]

Filing this to address the general issues brought up in bug 56779.  The
followLinks setting doesn't work correctly on Unix, and there apparently might
be problems on Windows and Mac also.  Operations should never return an error
code because the target of the link doesn't exist, if followLinks is set to false.

Comment 1

[Doug Turner (:dougt)]

Windows works fine.

Linux directory enumeration sucked.  Attached is a diff which replaces it with
the windows version which I wrote.  Also included is shaver's patch which fixes
honoring symlink during the validate_stat call:
   http://bugzilla.mozilla.org/showattachment.cgi?attach_id=17391

For the Mac, I need to find a mac buddy.

Comment 2

[Doug Turner (:dougt)]

Attachment: Fixes followSymlinks on unix.

Comment 3

[Brendan Eich [:brendan]]

-    NS_DECL_ISUPPORTS
+        NS_DECL_ISUPPORTS

Argh!  Don't add tabs!  If you use MSVC, click to Tools / Options / Tabs /
Expand spaces.  If you use vi, switch to vim and :set expandtab.

Also, and I'm entitled cuz I hacked on this file in conforming style, please use
the prevailing brace style, which is not

+            if (filepath == nsnull)
+            {
+                parent->GetPath(&filepath);
+            }
+            
+            if (filepath == nsnull)
+            {
+                return NS_ERROR_OUT_OF_MEMORY;
+            }

Also, in this particular case, put the second if inside the first if's then part
to avoid a useless test in the case where filepath is non-null:

+            if (filepath == nsnull) {
+                parent->GetPath(&filepath);
+                if (filepath == nsnull)
+                    return NS_ERROR_OUT_OF_MEMORY;
+            }

Finally, wouldn't it be better to capture the return value with nsresult rv and
return that if not NS_SUCCEEDED(rv)?

Gotta run, more in a bit.

/be

Comment 4

[Doug Turner (:dougt)]

I am pretty sure that there were no tabs in that diff (or in my files) I send
you.  Looking at the diff quickly, I can see how this does look like a tab or
respacing.  But in that area of the diff, I ripped out the old implemention of
the nsDirEnumerator and replaced it with another.  I think that the DIFF tool is
just trying to align itself.  Nonetheless, I untabified both files and the diffs
look the same.

The style was copied along with the code from nsLocalFileWin.  I don't think
that I should restylize it, but I will pull this code out into a new file if it
would make you feel better.  let me know.

Fixed up the double check and now returning the rv from GetPath().  Good suggests.

Comment 5

[Doug Turner (:dougt)]

Attachment: Draft 2. Adjustments based on /be's comments.

Comment 6

[pete collins]

> If you use vi, switch to vim and :set expandtab.
If you use vi and *not* vim like myself just do this.

:%!expand -2

this will expand your tabs to 2 spaces

if you want 4 spaces just do -4

Real men don't use vim.  (Accept only on windows) ;-)

--pete

Comment 7

[Doug Turner (:dougt)]

If the only problem with this patch is the whitespace (which has been fixed),
lets get it checked in!!

B <TAB> R <TAB> E <TAB> N <TAB> D <TAB> A <TAB> N

can you finish the review, please  :-)

Comment 8

[Brendan Eich [:brendan]]

dougt, sorry for the delay; also, I misread the diff: why are you overindenting
the members of class nsDirEnumerator by four spaces?  There is no sane reason
for the public: and protected: labels to consume an entire indentation level,
and none of the other code nearby (in the file, in other xpcom/io files) does
anything like that.

Why am I bothering?  Well, first: you bothered to lay things out differently,
which took some effort against "When in Rome", and I'm a citizen; second, I care
about the readability and unity of style in this particular file, since I've
hacked on it and conserved style in my efforts; third, I think shaver and
blizzard probably care a little.  This is all a small matter, but *not* a matter
of indifference.  It's a bigger deal in my mind than (although related to) the
discordant brace style. There are also gratuitous changes from nsLocalFile *file
to nsLocalFile* file; and if (!file) to if (file == nsnull), to boot.

All these style clashes make me wonder what would happen if I came over to help
you finish remodeling your kitchen, and for my part of the job (the cabinets
above the sink) I used European-style doors and a 20th century modern style,
while the rest of your kitchen, including the cabinet, window, and trim work you
were doing, which you had already described and shown to me, was American
Arts&Crafts style.  Mozilla is a big neighborhood, with many houses and many
rooms, but nsLocalFileUnix.cpp is one room, dammit!

waterson put it simply to [e-mail] in bug 60199:

>Fix the patch to use a bracing and parenthesizing style that is consistent with
>the rest of this file. Do that, and a=waterson

On to substance, and I'm attaching my version of your patch:

nsDirEnumerator::Init leaks filepath on early return after PR_OpenDir failure.

The old directory enumerator did not skip entries that seemed not to exist just
after being returned by PR_ReadDir.  nsIFile.idl does not fully specify the
directoryEntries attribute, but I don't think skipping entries that seem not to
exist (due to a dangling symlink or a racing unlink) is good, either -- let the
caller prune those out if it cares.  Racing unlinks mean that the caller must
check anyway.

(If non-existent entries should be pruned, roll that tail-recursive call to
HasMoreElements into a loop!)

I believe null_nsCOMPtr() is an anachronism -- just use nsnull.

Your patch to Exists breaks its _de facto_ semantics (not specified by any
doc-comment in nsIFile.idl), because the only way *_retval can be PR_FALSE on
return is if the return value is a failure code.  Non-existence is not a failure
condition, particularly for a boolean exists method ;-).  The old version would
return NS_OK always, with a proper boolean in *_retval.  The new version only
ever returns PR_TRUE in *_retval when the return value is NS_OK.

What's more, exists should not consult the stat cache, precisely because of
racing deletes.  My patch fixes this.

BTW, isWritable etc. all use access(2) still, which means they follow links.
This may be ok for isWritable at least, as a symlink is never writable, so why
not check its target?  But it is not well-specified by doc-comments in
io/nsIFile.idl, and it should be.

Indentation in FillStatCache was a little whacked (one extra space in front of
five added lines).

My comment last time about nesting the GetPath retry if (file == nsnull) after
GetTarget applies to nsLocalFileWin.cpp too.  My comment this time about leaking
filepath after PR_OpenDir failure applies to that file's directory enumerator
Init too.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: revised io/nsLocalFileUnix.{cpp,h} patch, plus components/Makefile.in patch required by introduction of lstat into nsLocalFileUnix.h (!)

Comment 10

[Brendan Eich [:brendan]]

BTW, my patch includes the longstanding (in my tree) patch for bug 55406, to
AppendRelativePath.

/be

Comment 11

[Brian Ryner (not reading)]

Actually, I think "0" is preferable to nsnull for assigning to a COMPtr.  Can't
remember where I heard that at though...

Comment 12

[Brendan Eich [:brendan]]

bryner: nsnull *is* 0, always and everywhere.  The entire reason for defining
nsnull rather than just using NULL from stddef.h is because some systems define
NULL as ((void*)0), which is not a generic null pointer in C++.

And even though 0 is fine for null in C++, since the style in almost all Mozilla
C++ code, especially in XPCOM code, is to use nsnull as a manifest for "the null
pointer", it seems to me we ought to use nsnull in xpcom/io/*.cpp.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: latest xpcom-relative patch, please test and review once

Comment 14

[Brendan Eich [:brendan]]

Doug asked me to check in his patch with my changes (but without the withdrawn
".."-illegal-in-AppendRelativePath patch for bug 55406).  These changes have
been reviewed to death already, but I'd feel better if someone tried out my
exact patch and took a quick look over it.  Doug?

/be

Comment 15

[Christopher Blizzard (:blizzard)]

Why are you not using a nsXPIDLCString for this code:

+    nsresult Init(nsILocalFile *parent)
+    {
+        char* filepath;
+        parent->GetTarget(&filepath);

...

+        mDir = PR_OpenDir(filepath);
+        nsMemory::Free(filepath);

There's a null_nsCOMPtr() in there:

+        mNext = null_nsCOMPtr();

I don't actually have a tree to test this on at the moment.

Comment 16

[Doug Turner (:dougt)]

over to you brendan.  Running out of time for 0.9, do you think you can verify
on windows and linux?

Comment 17

[Brendan Eich [:brendan]]

I'm going to wave this off, do it when tinderbox is in a less touchy state. 
Sorry, I didn't get to it sooner -- I suck!

/be

Comment 18

[Brian Ryner (not reading)]

Brendan, are you still planning to get this in for 0.9?

Comment 19

[Brendan Eich [:brendan]]

Yes, this is on my list, but I'm not going to get to it before Monday.  If you
want to take it away from me, feel free.

/be

Comment 20

[Brendan Eich [:brendan]]

bryner, you need this for 0.9?

/be

Comment 21

[Brian Ryner (not reading)]

Personally, no, since the filepicker rewrite won't land until at least 0.9.1.

Comment 22

[Brendan Eich [:brendan]]

I'd better do this, but bryner should feel free to steal it.

/be

Comment 23

[Brendan Eich [:brendan]]

I need to review and update the patch.  Not gonna hold 0.9.1 for this.

/be

Comment 24

[Brendan Eich [:brendan]]

bryner: which patch do you like?

dmose: make your case to drivers once we have a reviewed, super-reviewed patch
to check in.

/be

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Actually, I don't think this is critical for 0.9.1.

Comment 26

[Brendan Eich [:brendan]]

My patch has rusted, I suck.  Help welcome, FastLoad preoccupies me.

/be

Comment 27

[Brendan Eich [:brendan]]

petejc, would you mind taking this one too?  If you do mind, bounce it back at
me, ok?  Thanks.

/be

Comment 28

[Pete Collins (MDG)]

This bug will be resolved as part of bug 94323.

--pete

Comment 29

[Pete Collins (MDG)]

pushing back

Comment 30

[Pete Collins (MDG)]

pushing back

Comment 31

[Pete Collins (MDG)]

Attachment: patch to change api for incremental "safer" landing

Comment 32

[Pete Collins (MDG)]

Hold on, i need to revert nsLocalFileUnix.cpp it has changes for a different bug
merged in w/ this one.

updated patch forthcoming.

--pete

Comment 33

[Pete Collins (MDG)]

Attachment: patch for review

Comment 34

[Doug Turner (:dougt)]

Pete, can this be done for mozilla 1.0?  If not, please move it off the radar.

Comment 35

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.