Last Comment Bug 403989 - Cannot save images with SeaMonkey any more
: Cannot save images with SeaMonkey any more
: dataloss, regression, verified1.8.1.10, verified1.8.1.12
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 OS/2
-- blocker (vote)
: ---
Assigned To: Peter Weilbacher
: Nathan Froyd [:froydnj]
Depends on:
Blocks: 402460
  Show dependency treegraph
Reported: 2007-11-15 17:01 PST by Peter Weilbacher
Modified: 2007-12-28 16:09 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

version 1: backout (954 bytes, patch)
2007-11-15 17:29 PST, Peter Weilbacher
mozilla: review+
dveditz: approval1.8.1.10+
Details | Diff | Splinter Review
version 2: NSPR change (1.21 KB, patch)
2007-11-15 17:30 PST, Peter Weilbacher
no flags Details | Diff | Splinter Review

Description User image Peter Weilbacher 2007-11-15 17:01:07 PST
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 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 User image Peter Weilbacher 2007-11-15 17:27:32 PST
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 User image Peter Weilbacher 2007-11-15 17:29:56 PST
Created attachment 288934 [details] [diff] [review]
version 1: backout
Comment 3 User image Peter Weilbacher 2007-11-15 17:30:36 PST
Created attachment 288935 [details] [diff] [review]
version 2: NSPR change
Comment 4 User image 2007-11-16 01:29:43 PST
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 User image Peter Weilbacher 2007-11-16 02:00:46 PST
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 User image Peter Weilbacher 2007-11-16 02:30:03 PST
Given what the NSPR API docs at 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
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 User image Mike Kaply [:mkaply] 2007-11-16 08:28:18 PST
So is the right fix to backout the change or can we fix NSPR?
Comment 8 User image Peter Weilbacher 2007-11-16 12:14:52 PST
Comment on attachment 288934 [details] [diff] [review]
version 1: backout

No, I see no other way than backing it out.
Comment 9 User image Peter Weilbacher 2007-11-16 12:19:06 PST
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 release or apply the fix by hand for that Gecko release on OS/2.
Comment 10 User image Mike Kaply [:mkaply] 2007-11-16 15:48:37 PST
Comment on attachment 288934 [details] [diff] [review]
version 1: backout

Comment 11 User image Peter Weilbacher 2007-11-16 16:17:21 PST
OK, thanks. Attachment 288934 [details] [diff] checked in to trunk.
Comment 12 User image Peter Weilbacher 2007-11-16 16:23:22 PST
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 User image 2007-11-16 17:05:11 PST
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 User image Peter Weilbacher 2007-11-17 02:27:04 PST
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 User image Daniel Veditz [:dveditz] 2007-11-18 23:08:56 PST
Comment on attachment 288934 [details] [diff] [review]
version 1: backout

approved for, 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 User image Peter Weilbacher 2007-11-19 12:48:31 PST
Great, thanks. Checked in attachment 288934 [details] [diff] [review] on both MOZILLA_1_8_BRANCH and GECKO181_20071115_RELBRANCH.
Comment 17 User image Peter Weilbacher 2007-11-24 04:59:15 PST
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 User image Peter Weilbacher 2007-11-25 09:49:35 PST
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 to the new revision 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.

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