nsLocalFileWin::OpenFile propagates inconsistent Windows error codes

RESOLVED FIXED

Status

()

Core
XPCOM
P1
normal
RESOLVED FIXED
9 years ago
6 years ago

People

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

Tracking

(4 keywords)

Trunk
x86
Windows XP
fixed1.8.1.19, fixed1.9.0.5, fixed1.9.1, regression
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.5 +
blocking1.8.1.19 +
wanted1.8.1.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.14 KB, patch
neil@parkwaycc.co.uk
: review+
bsmedberg
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.8.1.19+
Samuel Sidler (old account; do not CC)
: approval1.9.0.5+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.

Comment 1

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

Comment 2

9 years ago
(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?

Comment 3

9 years ago
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?]
Created attachment 348316 [details] [diff] [review]
Change nsLocalFileWin Create() to match unix

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
Created attachment 348317 [details] [diff] [review]
Change CreateUnique to account for platform differences

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]
(Reporter)

Comment 7

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

Comment 8

9 years ago
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.
Keywords: fixed1.8.1.19, fixed1.9.0.5
Priority: -- → P1
Is there a straightforward way for QA to verify this fix in the 1.8 and 1.9.0 builds?
Should this be checkin-needed? Can attachment #348316 [details] [diff] [review] land as-is?

Comment 17

9 years ago
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
Last Resolved: 9 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.