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

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[James Ross]

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.

Comment 1

[Robert Strong (they/them - no direct email)]

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.

Comment 2

[Robert Strong (they/them - no direct email)]

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.

Comment 3

[Robert Strong (they/them - no direct email)]

Attachment: 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.

Comment 4

[James Ross]

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

[Benjamin Smedberg]

(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

[Benjamin Smedberg]

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).

Comment 7

[Robert Strong (they/them - no direct email)]

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 8

[Robert Strong (they/them - no direct email)]

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.

Comment 9

[Robert Strong (they/them - no direct email)]

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

Comment 10

[James Ross]

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.

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: 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 12

[Benjamin Smedberg]

Comment on attachment 189124 [details] [diff] [review]
patch

please file a followup bug about nsIFile.createUnique not working properly

Comment 13

[Robert Strong (they/them - no direct email)]

Filed bug 300692 for Win32 nsIFile.create always returning
NS_ERROR_FILE_ALREADY_EXISTS on error and how that affects nsIFile.createUnique.

Comment 14

[Benjamin Smedberg]

Fixed on trunk for 1.8b4