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.
Created attachment 188902 [details] [diff] [review] simple patch 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.
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.
Created attachment 189124 [details] [diff] [review] patch 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.
Comment on attachment 189124 [details] [diff] [review] patch please file a followup bug about nsIFile.createUnique not working properly
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
Last Resolved: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox1.1
Whiteboard: [checkin needed][a+]
You need to log in before you can comment on or make changes to this bug.