69862 - Close the stream in the destructor of the base classes nsInputStream and nsOutputStream

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Comment 1

[Navin Gupta]

Attachment: proposed fix

Comment 2

[Navin Gupta]

CC [e-mail] for review/super-review

Comment 3

[Navin Gupta]

cc bienvenu for review. This will help us avoid closing all the 
streams individually.

Comment 4

[Scott Collins]

darin, gordon: what do you think?  naving is going to run smoketests and what
not to make sure this doesn't cause any problems.  Is |close| idempotent?

Comment 5

[Scott Collins]

the problem here is that we don't know that the |Close| of any given input
stream or output stream actually is idempotent.  Maybe they all must be.  But,
if so, it should say so in the IDL files, and it doesn't.  Need network expertise.

I understand that this fix is spurred by bad behavior in mail where after
downloading messages with POP, the mailbox can't be compacted because, though
deleted, the stream has not been closed.

Comment 6

[Navin Gupta]

Attachment: Another possible fix

Comment 7

[Navin Gupta]

The compaction problem that we are trying to solve here concerns filestreams
because they do not relinquish control after they are deleted, this fix
would be a safer fix and will take care of idempotent problem.

Comment 8

[Scott Collins]

This second patch addresses my concerns.  sr=scc

Comment 9

[Darin Fisher]

A |close| method should not, in general, be relied upon to be idempotent.  Just
take a look at the close() system call man page on your linux box.  EBADF is
what you get if you try to close a file descriptor twice.  It seems like bad
code if you end up calling close() more than once.

Comment 10

[David :Bienvenu]

r=bienvenu, darin, the second patch addresses your concerns as well, doesn't it?

Comment 11

[Navin Gupta]

fix checked in. 

Comment 12

[Darin Fisher]

bienvenu: yes

Comment 13

[Simon Fraser [no longer active]]

Wasn't this backed out?

Comment 14

[Navin Gupta]

The bookmarks problem described in #70312 is now fixed. 

Comment 15

[kinmoz]

To answer Simon's question ... Navin's patch/fix was disabled by commenting out
the close() calls in the destructors he added:

  nsInputFileStream::~nsInputFileStream
  nsOutputFileStream::~nsOutputFileStream()

That is what fixed the bookmarks truncation problem I believe.

I just checked with LXR and it seems that the close() calls are still commented
out in todays source.

Comment 16

[Navin Gupta]

#70312 was just the temporary fix. In preparation for this fix to land again
I checked in a more permanent fix (See #70817)

Comment 17

[Simon Fraser [no longer active]]

Navin: please update us on the state of this bug.

Comment 18

[Navin Gupta]

Simon, the fix for this bug was backed out because bookmarks were being deleted 
on exit (#70312). I investigated the bookmarks code and checked in the fix
yesterday (#70817) so that when this fix lands again bookmarks don't get deleted.
There was also another issue about cookies (#70316), but I think this fix does
not affect it. 

I 'll do some more testing before I land this again

Comment 19

[Simon Fraser [no longer active]]

Cookies code also does stream passing by value. I'm filing another bug on that: 
bug 71063

Comment 20

[Navin Gupta]

I have done somketest for browser, mail and editor for all three platforms
(mac, linux and win) and everything seems to work fine. Also the bookmarks
issue #70312 (resolved in #70817) and cookies issue #70316 (resolved in #71063)
have been resolved. So I will checkin this patch again. In the second patch,
I will add flush() before I close the stream in ~nsOutputFileStream().

Comment 21

[Navin Gupta]

fix checked in again.

Comment 22

[Eric Pollmann]

Backing this change out as it causes bug 71834.

Comment 23

[Navin Gupta]

Marking this bug invalid as this does not apply to all cases.