Last Comment Bug 402460 - CreateUnique is very slow and returns bogus error for write-protected directory
: CreateUnique is very slow and returns bogus error for write-protected directory
Status: RESOLVED FIXED
: perf, verified1.8.1.10
Product: Core Graveyard
Classification: Graveyard
Component: File Handling (show other bugs)
: Trunk
: x86 All
: -- major (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 403989 420310 452217
Blocks: 394713
  Show dependency treegraph
 
Reported: 2007-11-04 11:43 PST by neil@parkwaycc.co.uk
Modified: 2016-06-22 12:16 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (1.64 KB, patch)
2007-11-04 12:01 PST, neil@parkwaycc.co.uk
benjamin: review+
dveditz: approval1.8.1.10+
mtschrep: approval1.9+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2007-11-04 11:43:17 PST
Trying to CreateUnique into a write-protected directory fails very slowly, as it attempts to create 9999 files before giving up with NS_ERROR_FILE_TOO_BIG. This is because Create always fails with NS_ERROR_FILE_ALREADY_EXISTS although NS_ERROR_FILE_ACCESS_DENIED is the correct error to use in this case.

This bites SeaMonkey branch builds because bug 375102 changed the behaviour of rdfXMLFlush to use a safe output stream which tries to CreateUnique the file. It also causes error returns when there weren't before but we work around that.
Comment 1 neil@parkwaycc.co.uk 2007-11-04 12:01:49 PST
Created attachment 287320 [details] [diff] [review]
Proposed patch

Simply propagating the return value from Open fixes the bug.

I also noticed the same error in OS/2, so I copied code from OpenNSPRFileDesc.
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2007-11-09 10:58:29 PST
Comment on attachment 287320 [details] [diff] [review]
Proposed patch

This looks like it is re-introducing the insecurity fixed by bug 43314, where the leak was introduced intentionally (at least as far as I can tell). Please re-request review if this is not the case.
Comment 3 neil@parkwaycc.co.uk 2007-11-09 13:45:46 PST
Comment on attachment 287320 [details] [diff] [review]
Proposed patch

I'm not sure what you mean by the leak:

>-        if (!file) return NS_ERROR_FILE_ALREADY_EXISTS;
The error code may not correctly describe the actual failure.
>+        if (!file)
>+            return NS_ErrorAccordingToNSPR();
The fix is to return the correct error code.
>         PR_Close(file);
>         return NS_OK;
Either way the file is closed.

>-        OpenFile(mResolvedPath,
>-                 PR_RDONLY | PR_CREATE_FILE | PR_APPEND | PR_EXCL, attributes,
>-                 &file);
>-        if (!file) return NS_ERROR_FILE_ALREADY_EXISTS;
>-
>-        PR_Close(file);
>-        return NS_OK;
This closes the file on success but again the error code might be bogus.
>+        rv = OpenFile(mResolvedPath,
>+                      PR_RDONLY | PR_CREATE_FILE | PR_APPEND | PR_EXCL, attributes,
>+                      &file);
>+        if (file)
>+            PR_Close(file);
>+        return rv;
The fix is to preseve the return code while closing the file on success.
Comment 4 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2007-11-09 13:57:56 PST
Comment on attachment 287320 [details] [diff] [review]
Proposed patch

Oh, I misread the diff.
Comment 5 neil@parkwaycc.co.uk 2007-11-09 14:27:35 PST
Comment on attachment 287320 [details] [diff] [review]
Proposed patch

Looking for this on branch as well to resolve the dependent bug.
Comment 6 neil@parkwaycc.co.uk 2007-11-12 01:24:38 PST
Fix checked in.
Comment 7 Daniel Veditz [:dveditz] 2007-11-13 11:36:28 PST
Comment on attachment 287320 [details] [diff] [review]
Proposed patch

approved for 1.8.1.10, a=dveditz for release-drivers

must land _today_ or wait for 1.8.1.11
Comment 8 neil@parkwaycc.co.uk 2007-11-13 16:17:01 PST
Fix checked in to the branch.
Comment 9 Al Billings [:abillings] 2007-11-15 16:31:30 PST
Can the reporter check with the Firefox 2.0.0.10 release candidate (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.10-candidates/rc1/) and verify if this is fixed or does it only express in Seamonkey?
Comment 10 neil@parkwaycc.co.uk 2007-11-16 08:08:48 PST
(In reply to comment #9)
>Can the reporter check with the Firefox 2.0.0.10 release candidate
>(http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.10-candidates/rc1/)
>and verify if this is fixed or does it only express in Seamonkey?
The problem is only readily reproducible in SeaMonkey, but I have tried the latest SeaMonkey 1.1.7pre nightly and the problem does not exhibit itself.
Comment 11 Al Billings [:abillings] 2007-11-16 10:23:50 PST
dveditz, is that enough for us to mark this as verified for branch?
Comment 12 Boris Zbarsky [:bz] 2008-03-04 10:39:17 PST
This seems to have caused bug 420310.

Note You need to log in before you can comment on or make changes to this bug.