452217 - nsLocalFileWin::OpenFile propagates inconsistent Windows error codes

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[[e-mail]]

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

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[[e-mail]]

(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?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Samuel Sidler (old account; do not CC)]

dveditz: What do you want to do here? We need to find an owner one way or another...

Comment 5

[Daniel Veditz [:dveditz]]

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

Comment 6

[Daniel Veditz [:dveditz]]

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

Comment 7

[[e-mail]]

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.

Comment 8

[[e-mail]]

Comment on attachment 348317 [details] [diff] [review]
Change CreateUnique to account for platform differences

Not sure whether this should be #ifdef XP_WIN

Comment 9

[Daniel Veditz [:dveditz]]

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

Comment 10

[Benjamin Smedberg]

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.

Comment 11

[Benjamin Smedberg]

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.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 348317 [details] [diff] [review]
Change CreateUnique to account for platform differences

We'll go the first route

Comment 13

[Samuel Sidler (old account; do not CC)]

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

Comment 14

[Daniel Veditz [:dveditz]]

nsLocalFileWin patch checked into 1.8 and 1.9.0 branches. mozilla-central will have to wait until after b2 is cut.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Is there a straightforward way for QA to verify this fix in the 1.8 and 1.9.0 builds?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Should this be checkin-needed? Can attachment #348316 [details] [diff] [review] land as-is?

Comment 17

[Glenda Jasper]

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

Comment 18

[Damon Sicore (:damons)]

Poke.  What's the story here?  Does this need to be landed or what?  It's a P1 beta 3 blocker.  Please advise.

Comment 19

[Damon Sicore (:damons)]

Dveditz, are you on top of this for 1.9.1?

Comment 20

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/mozilla-central/rev/e87319dd2bff
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ce38112af76b