455828 - get rid of FSRef caching in mac file code

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Josh Aas]

Attachment: fix v1.0

We should probably get rid of FSRef caching in mac files code. It doesn't stay valid in the face of external file modifications, see bug 322686. I doubt the cache saves us much time anyway, in my tests there is no Ts or Tp hit when it is removed.

Comment 1

[Markus Stange [:mstange]]

Comment on attachment 339194 [details] [diff] [review]
fix v1.0

(Side note: It would be cool if this came with more context and function names after the @@.)

Looks good to me, I think this patch is the way to go. An incorrect cache is of no use.
I looked at bug 164396 which added the cache, and the problem of invalid FSRefs because of external changes has apparently not been addressed at all...

r=me if you remove this change (there's a return in the if):

-  // CFURLGetFSRef only returns a Boolean for success,
-  // so we have to assume what the error was. This is
-  // the only probable cause.
-  return NS_ERROR_FILE_NOT_FOUND;
+  else {
+    // CFURLGetFSRef only returns a Boolean for success,
+    // so we have to assume what the error was. This is
+    // the only probable cause.
+    return NS_ERROR_FILE_NOT_FOUND;
+  }

Or was there a reason for this change?

Comment 2

[Josh Aas]

Attachment: fix v1.1

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 339305 [details] [diff] [review]
fix v1.1

diff -r dabae13daacb xpcom/io/nsLocalFileOSX.h

Lose the extra space before the new line after EqualsInternal



Did you run ts?  does this cost anything?  if not, looks good;

Comment 4

[Josh Aas]

I ran perf tests and didn't see any difference with this patch. As the patch that added caching in the first place noted, the caching isn't actually used that often.

Comment 5

[Josh Aas]

landed on trunk