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

RESOLVED FIXED in mozilla1.8final

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: James Ross, Assigned: rstrong)

Tracking

Trunk
mozilla1.8final
x86
Windows Server 2003
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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)
(Reporter)

Comment 4

13 years ago
Perhaps it would be better to make createUnique realise that ACCESS DENIED means
that is is never going to work, rather than keep trying?

Comment 5

13 years ago
(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 6

13 years ago
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
(Reporter)

Comment 10

13 years ago
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.
Attachment #188902 - Flags: approval-aviary1.1a2?
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.
Attachment #188902 - Attachment is obsolete: true
Attachment #189124 - Flags: review?(benjamin)

Comment 12

13 years ago
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+]

Comment 14

13 years ago
Fixed on trunk for 1.8b4
Status: NEW → RESOLVED
Last Resolved: 13 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.