Last Comment Bug 452217 - nsLocalFileWin::OpenFile propagates inconsistent Windows error codes
: nsLocalFileWin::OpenFile propagates inconsistent Windows error codes
Status: RESOLVED FIXED
: fixed1.8.1.19, fixed1.9.0.5, fixed1.9.1, regression
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows XP
: P1 normal (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
:
Mentors:
Depends on:
Blocks: 402460
  Show dependency treegraph
 
Reported: 2008-08-26 04:54 PDT by neil@parkwaycc.co.uk
Modified: 2011-08-27 08:19 PDT (History)
14 users (show)
benjamin: blocking1.9.1+
dveditz: blocking1.9.0.5+
dveditz: blocking1.8.1.19+
dveditz: wanted1.8.1.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Change nsLocalFileWin Create() to match unix (1.14 KB, patch)
2008-11-14 23:39 PST, Daniel Veditz [:dveditz]
neil: review+
benjamin: superreview+
samuel.sidler+old: approval1.8.1.19+
samuel.sidler+old: approval1.9.0.5+
Details | Diff | Splinter Review
Change CreateUnique to account for platform differences (1018 bytes, patch)
2008-11-14 23:42 PST, Daniel Veditz [:dveditz]
neil: review+
benjamin: superreview+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2008-08-26 04:54:35 PDT
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 [:bz] (TPAC) 2008-08-26 05:41:32 PDT
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 neil@parkwaycc.co.uk 2008-08-26 05:48:33 PDT
(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 [:bz] (TPAC) 2008-08-26 14:36:55 PDT
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) 2008-10-13 09:11:27 PDT
dveditz: What do you want to do here? We need to find an owner one way or another...
Comment 5 Daniel Veditz [:dveditz] 2008-11-14 23:39:45 PST
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
Comment 6 Daniel Veditz [:dveditz] 2008-11-14 23:42:37 PST
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.
Comment 7 neil@parkwaycc.co.uk 2008-11-15 05:05:51 PST
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 neil@parkwaycc.co.uk 2008-11-15 05:07:46 PST
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] 2008-11-17 15:07:58 PST
> (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 [:bsmedberg] 2008-11-18 07:34:33 PST
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 [:bsmedberg] 2008-11-18 07:35:07 PST
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] 2008-11-18 13:17:08 PST
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) 2008-11-18 13:17:08 PST
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] 2008-11-18 22:20:56 PST
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] 2008-12-01 12:11:11 PST
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) 2008-12-16 16:29:05 PST
Should this be checkin-needed? Can attachment #348316 [details] [diff] [review] land as-is?
Comment 17 Glenda Jasper 2009-01-14 15:12:40 PST
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) 2009-01-23 16:40:12 PST
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) 2009-01-26 11:40:50 PST
Dveditz, are you on top of this for 1.9.1?

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