Closed Bug 332139 Opened 14 years ago Closed 6 years ago

Remove comment on NS_ERROR_FAILURE case on file removal in nsJAR::Extract

Categories

(Core :: Networking: JAR, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: Waldo, Assigned: Cykesiopka)

References

()

Details

Attachments

(1 file, 1 obsolete file)

I need to check in a patch for bug 309296 before the relevant code is in the tree, at which point I'll update the URL field.  Note also that bug 322183 must be fixed before this bug can be fixed.
Component: Networking → Networking: JAR
QA Contact: networking → networking.jar
Are you still working on this Waldo?
Flags: needinfo?(jwalden+bmo)
Seven years later, I'm gonna say no.  :-)
Assignee: jwalden+bmo → nobody
Flags: needinfo?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Seven years later, I'm gonna say no.  :-)

:-) Ok, thanks!
Attached patch Proposed Patch (obsolete) — Splinter Review
Attachment #789272 - Flags: review?(taras.mozilla)
Comment on attachment 789272 [details] [diff] [review]
Proposed Patch

I think this should be ok, but I'll let glandium decide.
Attachment #789272 - Flags: review?(taras.mozilla) → review?(mh+mozilla)
Comment on attachment 789272 [details] [diff] [review]
Proposed Patch

Review of attachment 789272 [details] [diff] [review]:
-----------------------------------------------------------------

There's at least one case that will return NS_ERROR_FAILURE for a non empty directory: if the directory is a mount point (and anything else that would return EBUSY from rmdir). There's also EINVAL (pathname has . as last component), ELOOP (too many symbolic links), ENAMETOOLONG, ENOMEM, EROFS (read-only file system). Of all those, except EINVAL and EBUSY, the code following the check would fail gracefully. However, for EINVAL and EBUSY, files would be created without an error, while they shouldn't have been created at all.
Note I haven't looked at the Windows side of things. Only Unix.
Attachment #789272 - Flags: review?(mh+mozilla) → review-
(In reply to Mike Hommey [:glandium] from comment #6)
> Comment on attachment 789272 [details] [diff] [review]
> Proposed Patch
> 
> Review of attachment 789272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's at least one case that will return NS_ERROR_FAILURE for a non empty
> directory: if the directory is a mount point (and anything else that would
> return EBUSY from rmdir). There's also EINVAL (pathname has . as last
> component), ELOOP (too many symbolic links), ENAMETOOLONG, ENOMEM, EROFS
> (read-only file system). Of all those, except EINVAL and EBUSY, the code
> following the check would fail gracefully. However, for EINVAL and EBUSY,
> files would be created without an error, while they shouldn't have been
> created at all.
> Note I haven't looked at the Windows side of things. Only Unix.

Thanks for the detailed explanation!

Should the current behaviour just be left unchanged then?
(In reply to Cykesiopka from comment #7)
> Should the current behaviour just be left unchanged then?

I guess so. At this point, we may just want to remove the comment in the code and close this bug.
(In reply to Mike Hommey [:glandium] from comment #8)
> I guess so. At this point, we may just want to remove the comment in the
> code and close this bug.

Ok. I've adjusted the bug summary to show this, but please change it back / to something else if that is more appropriate.
Summary: Remove NS_ERROR_FAILURE case on file removal in nsJAR::Extract → Remove comment on NS_ERROR_FAILURE case on file removal in nsJAR::Extract
Assignee: nobody → cykesiopka.bmo
Attachment #789272 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #792527 - Flags: review?(mh+mozilla)
Attachment #792527 - Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ce9839537560
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.