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

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect)

Description

[Jeff Walden [:Waldo]]

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.

Comment 1

[:Cykesiopka]

Are you still working on this Waldo?

Comment 2

[Jeff Walden [:Waldo]]

Seven years later, I'm gonna say no.  :-)

Comment 3

[:Cykesiopka]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Seven years later, I'm gonna say no.  :-)

:-) Ok, thanks!

Comment 4

[:Cykesiopka]

Attachment: Proposed Patch

Comment 5

[(dormant account)]

Comment on attachment 789272 [details] [diff] [review]
Proposed Patch

I think this should be ok, but I'll let glandium decide.

Comment 6

[Mike Hommey [:glandium]]

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.

Comment 7

[:Cykesiopka]

(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?

Comment 8

[Mike Hommey [:glandium]]

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

Comment 9

[:Cykesiopka]

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

Comment 10

[:Cykesiopka]

Attachment: Remove the comment

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce9839537560

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce9839537560