56998 - nsLocalFile::Exists() is wrong

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Conrad Carlen (not reading bugmail)]

See http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileMac.cpp#1849. If
ResolveAndStat returns some error other than NS_ERROR_FILE_NOT_FOUND, it is
ignored. If the error caused ResolveAndStat to not set mTargetSpec, it will
(most likely) do the existence test on a zeroed out spec which will always
return noErr. Basically, this method can return TRUE for completely bad files
which don't exist.

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: simple fix for this

Comment 2

[Conrad Carlen (not reading bugmail)]

The patch above fixes this. This really needs to go in. I discovered this while
reviewing bug 56041. Not being able to reliably tell whether files exist
threatens the effectiveness of the patch for 56041.

Comment 3

[Simon Fraser [no longer active]]

You're using FSMakeFSSpec() as a surrogate for a file existence test; ideally, 
you should use PBGetCatInfoSync() for this. However FSMakeFSSpec() probably just 
calls PBGetCatInfoSync() under the hood.

Do you care if this is a file or folder? Your code won't tell you, but using 
PBGetCatInfoSync() and testing the bits in pBlock.hFileInfo.ioFlAttrib will.

Comment 4

[Conrad Carlen (not reading bugmail)]

Only if there was a difference in speed. I could use the microsecond timer and
see if there was any difference. I doubt but, while testing, I noticed that this
got called LOTS of times.

No, it doesn't matter if it's a file or folder.

Comment 5

[Simon Fraser [no longer active]]

ok, r=sfraser

Comment 6

[Conrad Carlen (not reading bugmail)]

Just for curiosity's sake (and because it was getting called so much), I did
measure it and found that both FSMakeFSSpec and PBGetCatInfo both average about
35 microseconds. Thanks for the quick r=.

Comment 7

[Conrad Carlen (not reading bugmail)]

Nominating for rtm. This needs to go in because because it blocks 56041 which is
(hopefully) going onto the branch. This fix is very small. I would say it is
extremely low risk.

Comment 8

[selmer (gone)]

Marking [rtm need info] since you are working on a patch.  The patch looks 
approvable if you can get your sr=.  When you have sr=, please change the 
whiteboard to [rtm+].

Comment 9

[Jason Eager]

This serious bug has been sitting here, waiting for a sr= for the last week now.
Adding waterson to cc to see if he can help us get a sr.

Comment 10

[Conrad Carlen (not reading bugmail)]

This has been checked into the trunk on 10/19 with, by now we can say, no ill
effect.

Comment 11

[Jason Eager]

This has now been sitting here for *2* weeks, waiting for a sr.

Comment 12

[Scott MacGregor]

I'm curious how this got checked into the trunk with out a super reviewer? Anyone?

Comment 13

[Conrad Carlen (not reading bugmail)]

Because Simon gave r= and he is a super-reviewer.

Comment 14

[selmer (gone)]

1.  It's claimed that this blocks 56041, but that bug is already checked into
the branch.  Was that badness, or is this not a blocker for that bug?

2.  What existing user-visible symptom exists for this bug?  (I am fully aware
of the potential you're describing, but would still like to know what errors a
real user would see with today's build.)

Has an email been sent to the reviewers alias asking for a super review?  Let's
get this resolved ASAP!

Comment 15

[Conrad Carlen (not reading bugmail)]

The patch for 56041 evolved several times. I think the final patch for 56041 was
not dependent on this. Either way, this patch is very simple, and the previous
code was clearly wrong. Normally, sfraser is the one to ask for Mac-specific
super-review. I'll try scc.

Comment 16

[Scott MacGregor]

this looks ok to me and I'm tempted to give it an sr=mscott but to be honest I
don't consider myself a mac expert to properly measure the behavior of
::FSMakeFSSpec.

scc, can you take a quick look at this patch and give the final sr=?

Comment 17

[Scott Collins]

looks good to me.  sr=scc.  Changing whiteboard accordingly.

Comment 18

[selmer (gone)]

OK, 56041 is out of the picture.  What user-visible symptoms can be seen if this
bug is not fixed?

Comment 19

[Conrad Carlen (not reading bugmail)]

I don't know of any user-visible symptoms. Being told that a file exists when it
doesn't - sounds like a user-visible symptom waiting to happen though. PDT's
call though. If it can't be checked in to the branch, I would like to mark it
fixed (it is on trunk) and forget it. The time spent on this page is now about
10x what it took to fix the bug.

Comment 20

[selmer (gone)]

rtm-, without such symptoms, can't take the risk now.

Comment 21

[Conrad Carlen (not reading bugmail)]

Since it's checked into trunk, at this point, isn't it fixed.

Comment 22

[Conrad Carlen (not reading bugmail)]

Checked into trunk long ago. It was left open back then waiting to go into 6.0
branch. Since that's long gone, this is fixed.