Closed Bug 403989 Opened 17 years ago Closed 17 years ago

Cannot save images with SeaMonkey any more

Categories

(Core :: XPCOM, defect)

x86
OS/2
defect
Not set
blocker

Tracking

()

VERIFIED FIXED

People

(Reporter: mozilla, Assigned: mozilla)

References

()

Details

(4 keywords)

Attachments

(1 file, 1 obsolete file)

Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9b2pre) Gecko/2007111218 SeaMonkey/2.0a1pre

This is a regression from bug 402460.

Saving images like http://www.seamonkey-project.org/images/template/header-logo.png fails (Context menu -> Save Image As...) fails because downloads.rdf cannot be created. nsLocalFile::CreateUnique which calls nsLocalFileOS2::Create expects the specific error state NS_ERROR_FILE_ALREADY_EXISTS to go on but translation through NS_ErrorAccordingToNSPR() does not provide this any more.
Yes, because we set the flag OPEN_ACTION_FAIL_IF_EXISTS for DosOpen() in _PR_MD_OPEN in os2io.c we get ERROR_OPEN_FAILED in return instead of ERROR_FILE_EXISTS.

Now, the flags used to call PR_Open in OS/2's nsLocalFile::Create are weird anyway: PR_RDONLY | PR_CREATE_FILE | PR_APPEND | PR_EXCL. Not sure how PR_RDONLY and PR_APPEND go together. So one could argue that 
- either _PR_MD_OPEN should react to PR_APPEND by setting 
  OPEN_ACTION_OPEN_IF_EXISTS and so patch NSPR (with unknown results
  without _lots_ of testing)
- or we just back out the OS/2 part of bug 402460 again. I think this would
  be less risky because it has worked well for a long time.

mkaply, I would like to hear your advice...
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attached patch version 2: NSPR change (obsolete) — Splinter Review
I made the change to attempt to keep parity with Windows because of bug 394713. All other OSes correctly return NS_ERROR_FILE_ALREADY_EXISTS.
Neil, yes, I know you were trying to do us on OS/2 a favor, but next time you do that, could you CC me? That could save me hours of debugging (or at least I could do the debugging session before the patch messes up branch ;-) ).
Given what the NSPR API docs at http://developer.mozilla.org/en/docs/PR_Open say    
   * PR_EXCL. Used with PR_CREATE_FILE; if the file does not exist, the
     file is created. If the file already exists, no action and NULL is
     returned
I think the previous behavior was correct. NULL was returned with PR_CREATE | PR_EXCL and so it could be assumed that the file already existed. Given the OS/2 API I don't think we can do anything else for this combination of PR_ flags than we do already, and from that we will probably never get anything else back than PR_IO_ERROR, even when file permissions/right access is a problem.

After reading the docs I convinced myself even more that PR_APPEND doesn't make sense here, so we shouldn't use it in  _PR_MD_OPEN as I originally suggested.
Attachment #288935 - Attachment is obsolete: true
So is the right fix to backout the change or can we fix NSPR?
Comment on attachment 288934 [details] [diff] [review]
version 1: backout

No, I see no other way than backing it out.
Attachment #288934 - Flags: review?(mozilla)
Actually, this problem also seems to mess up profile handling in other ways (doesn't write stuff to bookmarks or prefs) and hence can cause dataloss, so this is really a blocker.
Either we still get it into CVS for the 1.8.1.10 release or apply the fix by hand for that Gecko release on OS/2.
Severity: major → blocker
Flags: blocking1.8.1.10?
Keywords: dataloss
Comment on attachment 288934 [details] [diff] [review]
version 1: backout

r=mkaply
Attachment #288934 - Flags: review?(mozilla) → review+
OK, thanks. Attachment 288934 [details] [diff] checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment on attachment 288934 [details] [diff] [review]
version 1: backout

I'm not going to check in on the _RELBRANCH without asking this time...

But this is a blocker for OS/2.
Attachment #288934 - Flags: approval1.8.1.10?
Sorry to put you to all this trouble. All the other platforms seem be able to produce reasonable return codes and I had no idea that OS/2 couldn't.

Are OS/2 users likely to be unable to write to the installation directory? We may need to relnote that SeaMonkey 1.1.x will never support this under OS/2.
There are some solutions to implement access restrictions on OS/2 but in the core it's a single user system. I never saw anybody complain about this as a problem for Mozilla apps.
Comment on attachment 288934 [details] [diff] [review]
version 1: backout

approved for 1.8.1.10, a=dveditz

It's OK to land this OS/2-only backout on the RELBRANCH (and also on the 1.8 branch, of course).
Attachment #288934 - Flags: approval1.8.1.10? → approval1.8.1.10+
Great, thanks. Checked in attachment 288934 [details] [diff] [review] on both MOZILLA_1_8_BRANCH and GECKO181_20071115_RELBRANCH.
Hmm, I did not reset the release tags like SEAMONKEY_1_1_7_RELEASE or FIREFOX_2_0_0_10_RELEASE. I had somehow assumed that the release team would do that.
Should I still do that? Any other tags would I have to set?
OK, I used bonsai's graph feature to find out that only those two tags were set (I don't care about the RC1 tag) and reset them from revision 1.83.2.5 to the new revision 1.83.2.5.2.1 for this one file.

In the meantime Mike Luther verified that his branch problems are gone with the SeaMonkey 1.1.7 candidate version that I built (and that now becomes the release) and the same is true for the 1.1.8pre nightly. I have since last week verified that trunk now works again as it should.
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.10?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: