Note: There are a few cases of duplicates in user autocompletion which are being worked on.

CreateUnique is very slow and returns bogus error for write-protected directory

RESOLVED FIXED

Status

Core Graveyard
File Handling
--
major
RESOLVED FIXED
10 years ago
a year ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

({perf, verified1.8.1.10})

Trunk
x86
All
perf, verified1.8.1.10
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Hardware: All → PC
(Assignee)

Comment 1

10 years ago
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #287320 - Flags: review?(benjamin)
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.
Attachment #287320 - Flags: review?(benjamin) → review-
(Assignee)

Comment 3

10 years ago
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.
Attachment #287320 - Flags: review- → review?(benjamin)
Comment on attachment 287320 [details] [diff] [review]
Proposed patch

Oh, I misread the diff.
Attachment #287320 - Flags: review?(benjamin) → review+
(Assignee)

Comment 5

10 years ago
Comment on attachment 287320 [details] [diff] [review]
Proposed patch

Looking for this on branch as well to resolve the dependent bug.
Attachment #287320 - Flags: approval1.9?
Attachment #287320 - Flags: approval1.8.1.10?

Updated

10 years ago
Attachment #287320 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 6

10 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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
Attachment #287320 - Flags: approval1.8.1.10? → approval1.8.1.10+
(Assignee)

Comment 8

10 years ago
Fix checked in to the branch.
Keywords: fixed1.8.1.10
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?

Updated

10 years ago
Depends on: 403989
(Assignee)

Comment 10

10 years ago
(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.
dveditz, is that enough for us to mark this as verified for branch?
Keywords: fixed1.8.1.10 → verified1.8.1.10

Updated

10 years ago
Depends on: 420310

Comment 12

10 years ago
This seems to have caused bug 420310.

Updated

9 years ago
Depends on: 452217
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.