Closed Bug 452217 Opened 11 years ago Closed 11 years ago

nsLocalFileWin::OpenFile propagates inconsistent Windows error codes

Categories

(Core :: XPCOM, defect, P1)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: dveditz)

References

Details

(4 keywords)

Attachments

(1 file, 1 obsolete file)

When attempting to create a new file, the Unix open call will always fail with EEXIST if a directory entry exists with the same name but the equivalent Windows call will fail with the equivalent of EACCES if a directory exists.

Unfortunately CreateUnique needs to rely on Create returning EEXIST - under normal circumstances there's no point trying to create 10000 files as if access is denied on the first call then it will probably be denied on the other 9999.

This came to light because the fix to bug 402460 caused bug 420310.
This is a regression from bug 402460 as far as I can tell.  The real problem here is that createUnique fails when it should not.  This is an incompatible change to a frozen interface, and we need to fix this ASAP (or back out bug 402460).
Blocks: 402460
Flags: blocking1.9.1?
Flags: blocking1.9.0.3?
Flags: blocking1.8.1.17?
Keywords: regression
(In reply to comment #1)
> (or back out bug 402460).
And what about the chain of bugs that led to regression bug 402460 being filed?
Flags: wanted1.8.1.x+
Flags: blocking1.9.0.3?
Flags: blocking1.9.0.3+
Flags: blocking1.8.1.18+
Flags: blocking1.8.1.17?
We were at a known-good state on this stuff in 1.8.1.0 right?  Or at least known-acceptable.  If we can't move from there without breaking things, we need to revert to that.

Honestly, I would hope that it doesn't come to that.  But regressing things on stable branches (especially regressing frozen APIs!) is just not acceptable.
dveditz: What do you want to do here? We need to find an owner one way or another...
Assignee: nobody → dveditz
Flags: blocking1.9.0.5+
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.19+
Flags: blocking1.8.1.18+
Flags: blocking1.9.1? → blocking1.9.1+
Whiteboard: [needs patch?]
One approach is to change the windows versions of nsLocalFile::Create() to return already-exists for directories like Unix does. This affects all callers of Create, though -- maybe a good thing, but also risky if other callers have made per-platform adjustments and rely on the current behavior
The other approach is to make CreateUnique account for platform differences and otherwise don't change any more than we need to.
Attachment #348316 - Flags: superreview?(benjamin)
Attachment #348316 - Flags: review?(neil)
Attachment #348317 - Flags: superreview?(benjamin)
Attachment #348317 - Flags: review?(neil)
Whiteboard: [needs patch?] → [needs reviews and decision on approach]
Comment on attachment 348316 [details] [diff] [review]
Change nsLocalFileWin Create() to match unix

>+        if (rv == NS_ERROR_FILE_ACCESS_DENIED)
>+        {
>+            // need to return already-exists for directories (bug 452217)
>+            PRBool isdir;
>+            if (NS_SUCCEEDED(IsDirectory(&isdir)) && isdir)
>+                rv = NS_ERROR_FILE_ALREADY_EXISTS;
Currently IsDirectory won't do a new stat, since one was done at the beginning of Create, although bug 456603, if it ever lands, would change that, so if you don't want that to happen, it might be worth checking mFileInfo64 directly.
Attachment #348316 - Flags: review?(neil) → review+
Comment on attachment 348317 [details] [diff] [review]
Change CreateUnique to account for platform differences

Not sure whether this should be #ifdef XP_WIN
Attachment #348317 - Flags: review?(neil) → review+
Attachment #348316 - Flags: superreview?(benjamin) → superreview+
> (From update of attachment 348317 [details] [diff] [review])
> Not sure whether this should be #ifdef XP_WIN

It could be if you think only windows really has that issue.

Benjamin: should I take your sr+ on the first patch and not the second as a vote that that is the right approach? It'd be clearer if you minused the one you don't want us to do.
I definitely think we should take the first patch on trunk. I tend to think we should take it on branches as well: I think the chance that other code relies on the consistent behavior is at least as great as finding Windows-specific code that relies on the inconsistent behavior.
Attachment #348317 - Flags: superreview?(benjamin) → superreview+
Comment on attachment 348317 [details] [diff] [review]
Change CreateUnique to account for platform differences

But if you really want to take this on branch(es) instead, that's ok.
Whiteboard: [needs reviews and decision on approach] → [needs landing]
Comment on attachment 348317 [details] [diff] [review]
Change CreateUnique to account for platform differences

We'll go the first route
Attachment #348317 - Attachment is obsolete: true
Comment on attachment 348316 [details] [diff] [review]
Change nsLocalFileWin Create() to match unix

Approved for 1.8.1.19 and 1.9.0.5. a=ss for release-driveers
Attachment #348316 - Flags: approval1.9.0.5+
Attachment #348316 - Flags: approval1.8.1.19+
nsLocalFileWin patch checked into 1.8 and 1.9.0 branches. mozilla-central will have to wait until after b2 is cut.
Priority: -- → P1
Is there a straightforward way for QA to verify this fix in the 1.8 and 1.9.0 builds?
I am totally new at Bugzilla and using Mozilla/Firefox but have issues using the Copy Paste wiht pictures in Care2.com can you assist me on this or am I way off base i do have 8.5 yrs on PC
Poke.  What's the story here?  Does this need to be landed or what?  It's a P1 beta 3 blocker.  Please advise.
Dveditz, are you on top of this for 1.9.1?
http://hg.mozilla.org/mozilla-central/rev/e87319dd2bff
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ce38112af76b
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Whiteboard: [needs landing]
Blocks: 300692
No longer blocks: 300692
You need to log in before you can comment on or make changes to this bug.