Closed Bug 300265 Opened 14 years ago Closed 14 years ago

Trying to select an extension installed on a read-only network (smb, samba) share hangs Firefox for ages

Categories

(Toolkit :: Add-ons Manager, defect)

x86
Windows Server 2003
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.8final

People

(Reporter: bugzilla-mozilla-20000923, Assigned: rstrong)

Details

Attachments

(1 file, 1 obsolete file)

This is most obviously a problem if Firefox itself is running from a read-only
share, which is where I found it.

When I click an extension (Talkback, DOMI, etc.) that is installed globally,
Firefox hangs for a minute or so. It was not clear why until I ran File Monitor
from Sys Internals... Firefox is doing an access test on <install
location>\extensions\ not once, not twice, but 20,000 times! That's right, it
tries 20,000 times to create a file called "Access Privileges Text-NNNNN".

I can't even begin to think why it would try that many times.
The hang is easily seen when using a read only local directory as well. Also
noticed that the call from the EM code to create the file only happens a couple
of times and the call to createUnique takes quite a while to return.
The use of createUnique is obviously part of the cause...
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileCommon.cpp#112

I think for the short term using create instead of createUnique and the EM's
getRandomFileName function.
Attached patch simple patch (obsolete) — Splinter Review
I just changed it to use the getRandomFileName function and added a file
extension to the access check file that is created since getRandomFileName
requires an extension to generate the file name properly. I don't think this is
the only thing that needs to happen here but it is a start. I know it is late
in the game but this may be appropriate for 1.1a2 and is about as safe as they
get.
Attachment #188902 - Flags: review?(benjamin)
Perhaps it would be better to make createUnique realise that ACCESS DENIED means
that is is never going to work, rather than keep trying?
(In reply to comment #4)

You would also need to test for the file's existence, because if you get ACCESS
DENIED and the file *does* exist you should try again with a new seed.
Comment on attachment 188902 [details] [diff] [review]
simple patch

Hrm, I suppose this is ok, though I vaguely prefer a check for
testFile.exists() before we try to createUnique() it (and try a new randomname
if it does exist).
Attachment #188902 - Flags: review?(benjamin) → review+
I considered checking for existence but decided not to do this until the issues
are better understood. For example, I'm not entirely sure that existence will
suffice in some situations without additional checks (e.g. the parent directory
does not exist and I suspect others). Also, there are other things I believe
need to be done here like only checking if we have access once per location vs.
once per call to canAccess for a location which is why it tried 20,000 times
instead of 10,000 times when using createUnique.
Comment on attachment 188902 [details] [diff] [review]
simple patch

Since this affects local read only installs (e.g. a noticeable delay) and is a
safe / simple fix I think this should land for 1.1a2 if possible.
Attachment #188902 - Flags: approval-aviary1.1a2?
Assignee: nobody → rob_strong
Note... the return code when creating a file is hardcoded to always return
NS_ERROR_FILE_ALREADY_EXISTS though it does return a reasonable err when
creating a directory. It would help with this if it returned the real err for
files as well or we could possibly just create a directory instead. :/

http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#762
I pointed this out to bsmedberg on IRC yesterday, but he seems disinterested in
any such change.

I will point out that I changed nsLocalFileWin.cpp so that is used the proper
API error --> XPCOM error mapping, it did stop the hangs (because it got the
real error instead of a fake one). It seems to work fine, too, though I found
one interesting effect (I don't know if this happens without the change): if a
file called "Access Privilege Test" exists, and the program has enough
permissions, it gets deleted. I wouldn't be surprised if this is CreateUnique
not realising when the create failed and when it succeded.
Attached patch patchSplinter Review
An easy way to reproduce this locally including the hang on NTFS is to make the
app's extensions directory everyone -> read only and then in advanced edit the
permissions for everyone and add delete subfolders and files. This throws a
NS_ERROR_FILE_TOO_BIG when trying to createUnique a file but it correctly
throws a NS_ERROR_FILE_ACCESS_DENIED when trying to createUnique a directory
and does not cause a hang.

I'm not comfortable with changing nsLocalFileWin.cpp due to the number of
consumers that may be depending on it returning NS_ERROR_FILE_ALREADY_EXISTS so
this patch creates a directory using createUnique.
Attachment #188902 - Attachment is obsolete: true
Attachment #189124 - Flags: review?(benjamin)
Comment on attachment 189124 [details] [diff] [review]
patch

please file a followup bug about nsIFile.createUnique not working properly
Attachment #189124 - Flags: review?(benjamin)
Attachment #189124 - Flags: review+
Attachment #189124 - Flags: approval1.8b4+
Filed bug 300692 for Win32 nsIFile.create always returning
NS_ERROR_FILE_ALREADY_EXISTS on error and how that affects nsIFile.createUnique.
Whiteboard: [checkin needed][a+]
Fixed on trunk for 1.8b4
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox1.1
Whiteboard: [checkin needed][a+]
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.