Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Cannot save images with SeaMonkey any more

VERIFIED FIXED

Status

()

Core
XPCOM
--
blocker
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Peter Weilbacher, Assigned: Peter Weilbacher)

Tracking

(4 keywords)

Trunk
x86
OS/2
dataloss, regression, verified1.8.1.10, verified1.8.1.12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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)

Comment 2

10 years ago
Created attachment 288934 [details] [diff] [review]
version 1: backout
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 3

10 years ago
Created attachment 288935 [details] [diff] [review]
version 2: NSPR change

Comment 4

10 years ago
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.
(Assignee)

Comment 5

10 years ago
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 ;-) ).
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Updated

10 years ago
Attachment #288935 - Attachment is obsolete: true

Comment 7

10 years ago
So is the right fix to backout the change or can we fix NSPR?
(Assignee)

Comment 8

10 years ago
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)
(Assignee)

Comment 9

10 years ago
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 10

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

r=mkaply
Attachment #288934 - Flags: review?(mozilla) → review+
(Assignee)

Comment 11

10 years ago
OK, thanks. Attachment 288934 [details] [diff] checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

10 years ago
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?

Comment 13

10 years ago
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.
(Assignee)

Comment 14

10 years ago
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+
(Assignee)

Comment 16

10 years ago
Great, thanks. Checked in attachment 288934 [details] [diff] [review] on both MOZILLA_1_8_BRANCH and GECKO181_20071115_RELBRANCH.
Keywords: fixed1.8.1.10, fixed1.8.1.11
(Assignee)

Comment 17

10 years ago
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?
(Assignee)

Comment 18

10 years ago
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
Keywords: fixed1.8.1.10, fixed1.8.1.11 → verified1.8.1.10, verified1.8.1.11
(Assignee)

Updated

10 years ago
Flags: blocking1.8.1.10?
You need to log in before you can comment on or make changes to this bug.