Closed
Bug 40750
Opened 25 years ago
Closed 24 years ago
add flag to nsIFile for "no symbolic links allowed"
Categories
(Core :: XPCOM, defect, P3)
Tracking
()
VERIFIED
FIXED
People
(Reporter: warrensomebody, Assigned: dougt)
Details
(Keywords: perf, Whiteboard: patch reviewed!)
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
It has been reported to me that a large amount of time during startup (and in
general?) goes to resolving symbolic links (aliases, shortcuts) for nsIFile, or
at least checking to see if we need to resolve (usually non-existent) symbolic
links.
Perhaps we should consider adding a flag to nsILocalFile init routine(s) to say
whether symbolic links are allowed or not. Then when resolving resources (e.g.
chrome urls) we would just say "these are never allowed to be symbolic links"
and bypass the need to check for links.
I don't think allowing symbolic links in our resource hierarchy is necessary.
(Note this optimization might be unnecessary when we switch most resources over
to jar files, but the flag in general might help us in other places.)
Assignee | ||
Comment 1•24 years ago
|
||
I know that we have hashed this once or twice before but: nsIFile - the symlink
autoresolution. we should just make resolution an explicit call. This is what
is expected on windows and the mac from the developers point of view anyway and
it will reduce complexity (probably improve performance). On unix, it could
just noop. The more I think about this, the more I think it is the right thing
to do. thoughts?
Reporter | ||
Comment 2•24 years ago
|
||
Are you sure that would yield a usable interface? It would be bad if code had
to test for symlinks and dereference them at every step. That's why I thought
that just declaring "this path is allowed to contain symlinks" vs. "this one
doesn't" might be better. But I'm open to suggestion.
Assignee | ||
Comment 3•24 years ago
|
||
what would it do on unix?
Assignee | ||
Comment 4•24 years ago
|
||
also, if we added a flag, I would rather have an attribute instead changing the
initWithPath(). This is so that someone can set it after it has been inited.
Maybe it should return an error if there were any symlinks in what the nsIFile
was initilized to.
Assignee | ||
Comment 6•24 years ago
|
||
I added this attribute and got everything building on windows. I have
tested directory enumeration, stating every file as we go. Windows was
implement so that shortcut resolution would only happen if there was a
file-not-found condition when we try to use the nsIFile. Hence, there is no
performance improvement on windows while interation over the path c:\winnt\.
Unix is always going to symlink resolve so this is a no brainer and no
improvement.
Mac is the one that may have improve performance. We are resolving alot.
Comment 7•24 years ago
|
||
Dougt, got a patch to attach to this bug yet?
/be
Assignee | ||
Comment 8•24 years ago
|
||
Assignee | ||
Comment 9•24 years ago
|
||
Not fully tested on linux/mac. Windows looks okay.
Assignee | ||
Comment 10•24 years ago
|
||
heck, i don't mean testing I mean implementing...
Assignee | ||
Comment 11•24 years ago
|
||
Assignee | ||
Comment 12•24 years ago
|
||
I have a r=blizzard on the patches. Need approval.
note this does note fix mac yet. there is another bug for that.
Whiteboard: patch reviewed!
Assignee | ||
Comment 13•24 years ago
|
||
fix is in. windows and unix work fine. Mac still does nothing with any values
set. I will fix that with bug 38481.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 14•24 years ago
|
||
dougt, how can I verify this? testcase? Thanks!
Reporter | ||
Comment 15•24 years ago
|
||
It's only useful to do this if you're going to fix the places in the code that
can't have links, and pass the flag in there, e.g. resource urls. Have you done
that? If not, we should reopen this.
Assignee | ||
Comment 16•24 years ago
|
||
warren, resource url/file transport need to resolve because the mac uses alias
in dist. I would love to disable this, but component registration, ect. would
stop working.
Reporter | ||
Comment 17•24 years ago
|
||
Why does the mac use aliases in dist? Seems unnecessary since other platforms
don't. Let's remove it.
Comment 18•24 years ago
|
||
Why are symlinks (aliases, whatever) in dist bad?
/be
Comment 19•24 years ago
|
||
No way, dude. Using aliase in dist allows Mac to track file dependencies
properly, and also allows developers to edit files via aliases in chrome/ etc,
without having to worry about the extra make step that windows developers need.
It also means that relinking a DLL is all that is required to update the one in
components or whatever.
The short: aliases make our lives easier. Please don't take that away.
Assignee | ||
Comment 20•24 years ago
|
||
This conversation should be put out on the mac/build ngs.
Reporter | ||
Comment 21•24 years ago
|
||
Aliases make your life much much slower (at least as measured on windows -- I
assume mac is similar). When we complete the jar file step, you won't have
them anyway, so both our arguments are moot.
I would like to identify places in the code that can never allow aliases, and
set the flag for them so that we get the performance benefit.
Assignee | ||
Comment 22•24 years ago
|
||
warren, acutally, I want to know where in the code we require them. i would
love to be able to just say to hell will alias resolution all together, but are
some small places that need them... more so on the mac than any platform.
Comment 23•24 years ago
|
||
Well, for one thing, we never use folder aliases. So it would be safe, I think,
to only permit leaf aliases for resource URLs. I think clients will want to use
aliases for other uses of nsILocalFile, e.g. I should be able to replace my
profile folder with an alias to a different folder.
Assignee | ||
Comment 24•24 years ago
|
||
we don't have that kind of gradularity with the current api. either it resolves
automatically or not. we should change this to take flags, but I don't like the
complexity it will create.
Comment 25•24 years ago
|
||
Sounds like still an open issue. Reopening...move along the bug train if you
disagree.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 26•24 years ago
|
||
that would be a different bug. This bug is fixed.
Status: REOPENED → RESOLVED
Closed: 24 years ago → 24 years ago
Resolution: --- → FIXED
Comment 27•24 years ago
|
||
please verify
You need to log in
before you can comment on or make changes to this bug.
Description
•