403989 - Cannot save images with SeaMonkey any more

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Peter Weilbacher]

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.

Comment 1

[Peter Weilbacher]

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

Comment 2

[Peter Weilbacher]

Attachment: version 1: backout

Comment 3

[Peter Weilbacher]

Attachment: version 2: NSPR change

Comment 4

[[e-mail]]

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.

Comment 5

[Peter Weilbacher]

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

Comment 6

[Peter Weilbacher]

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.

Comment 7

[Mike Kaply [:mkaply]]

So is the right fix to backout the change or can we fix NSPR?

Comment 8

[Peter Weilbacher]

Comment on attachment 288934 [details] [diff] [review]
version 1: backout

No, I see no other way than backing it out.

Comment 9

[Peter Weilbacher]

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.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 288934 [details] [diff] [review]
version 1: backout

r=mkaply

Comment 11

[Peter Weilbacher]

OK, thanks. Attachment 288934 [details] [diff] checked in to trunk.

Comment 12

[Peter Weilbacher]

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.

Comment 13

[[e-mail]]

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.

Comment 14

[Peter Weilbacher]

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 15

[Daniel Veditz [:dveditz]]

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

Comment 16

[Peter Weilbacher]

Great, thanks. Checked in attachment 288934 [details] [diff] [review] on both MOZILLA_1_8_BRANCH and GECKO181_20071115_RELBRANCH.

Comment 17

[Peter Weilbacher]

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?

Comment 18

[Peter Weilbacher]

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.