1122698 - C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (Step-1)

Status: RESOLVED DUPLICATE of bug 1242030

(MailNews Core :: Networking: POP, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Step-1: Keeping track of |Close| by nullifying stream variables.

This is a branch of meta Bug 1121842 which depends on this
entry.

This is the step-1
(from bug 112184)
--- quote ---
Step 1. Remove Extra ->Close() calls (and Flush() before Close().).

Let us remove extra / unnecessary / bogus |Close| calls.

The removal is based on the two proposals below.

PROPOSAL-1: DiscardNewMessage should not close the file stream passed
as the 1st argument. It is caller's responsibility to close it..

PROPOSAL-2: FinishNewMessage should not close the
file stream passed as the first argument.
It is caller's responsibility to close it.

The reasoning is given in a crude write-up I created from checking the
code at the end. (This will be posted as the next comment.)

I checked the usage of two functions above, and with a few patches,
the proposals ought to work.

This patch will be filed as a different bugzilla on which this meta
bugzilla entry depends.

--- end quote

Actually, not many |Close| are removed statically, but
their calls during execution will be decreased by 
- setting stream value to nullptr after |Close|, and
- the checking of stream value before calling |Close|.

(Well, that is the theory. We may find out more
bogus calls once we check the return value of Close().
But this is a start.)

Local |make mozmill| test passed, and |make xpcshell-tests| did not introduce any new bugs with the attached patch.
I will still mark this as Work-in-Progress in the series so that
people in the know can figure out where any modifications should be made, etc. by looking at the whole picture.
 
(Hmm. I wonder why there is component for Mail Transfer. This is a mail client.)

Comment 1

[ISHIKAWA, Chiaki]

> (Hmm. I wonder why there is component for Mail Transfer. This is a mail client.)
I meant to say, it is strange that there is NO component for Mail Transfer in bugzilla.

I wonder if rkent can comment on the approach here.

TIA

PS: I have a suspicion that the correct handling of file stream and Close seems
to make the buffering output in nsPop3Sink.cpp work correctly
based on a preliminary testing. If so, all the better, but again, 
I will leave the introduction of buffered output and its test 
until later steps.

Comment 2

[ISHIKAWA, Chiaki]

A typo in comment 0.

> This is the step-1
> (from bug 112184)
> --- quote ---

The quote was taken from Bug 1121842, of course.

Comment 3

[ISHIKAWA, Chiaki]

Attachment: Step-1: Keeping track of |Close| by nullifying stream variables. (Take 2) please see comment.

I recreated the patch to take care of the following issue that was unfortunately missing from the initial patch.

|IncorporateComplete| in nsPop3Sink.cpp
has a seemingly buggy code as noticed during code review.

Quote from the crude write-up in 
https://bugzilla.mozilla.org/show_bug.cgi?id=1121842#c1

--- begin quote
+//
+// nsPop3Sink::IncorporateComplete(nsIMsgWindow *aMsgWindow, int32_t aSize)
+//
+// m_outFileStream should still be open if !m_downloadingToTempFile
+// it is closed if (m_downloadingToTempFile.)
+//
+
+// XXX: something is wrong(?): m_outFileStream->Close() is called
+// twice in a path.

+// Essentially (less error check) the code looks like this.

+//
+//      m_outFileStream->Close();
+//      m_newMailParser->FinishHeader();
+//      ...
+//      nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream);
+//      rv = MsgReopenFileStream(m_tmpDownloadFile, inboxInputStream);
+//      // Should this reference be inboxInputStream???
+//      if (m_outFileStream)
+//      {
+//        int64_t tmpDownloadFileSize;
+//        uint32_t msgSize;
+//        hdr->GetMessageSize(&msgSize);
+//        nsCOMPtr <nsIFile> tmpClone;
+//        rv = m_tmpDownloadFile->Clone(getter_AddRefs(tmpClone));
+//        tmpClone->GetFileSize(&tmpDownloadFileSize);
+//        ...
+//        rv = m_newMailParser->AppendMsgFromStream(inboxInputStream, hdr,
+//                                                    msgSize, m_folder);
+//        // XXX: DANGER, something is wrong???
+//        // We have ALREADY closed m_outFileStream  about 32 lines above!!!
+//
+//        // But we have duplicated some info into inboxInputStream
+//        // Maybe we should be closing inboxInputStream here???
+//
+//        m_outFileStream->Close(); // close so we can truncate.
+//        m_tmpDownloadFile->SetFileSize(0);
+
+//
--- end quote

In the end I decided to Close inboxInputStream instead of m_outFileStream near the end of the section mentioned above.

The patch passed |make mozmill| and |make xpcshell-tests| locally.

It could be even that
> if (m_outFileStream)
check and surrounding "{", "}"
can be taken out, but I have not done that yet.

I wonder if the general outline of the patch, and the proposals not to close the streams in the two leaf functions are OK.

TIA

Comment 4

[ISHIKAWA, Chiaki]

Attachment: Attachments Step-1: Keeping track of |Close| by nullifying stream variables. (Take 2) please see comment. [take 2, update]

Update. This patch applies to current tree.

Comment 5

[ISHIKAWA, Chiaki]

Attachment: Attachments Step-1: Keeping track of |Close| by nullifying stream variables. (Take 3) Update to match current tree

This is an update for the current C-C tree
and a patch for checking the file pointer position in  Bug 1116055 - Performance issue: Failure to use buffered write
(comm-central thunderbird)

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Attachments Step-1: Keeping track of |Close| by nullifying stream variables. (Take 4) Update to match current tree

This is going to be in a series of patches.

========================================
PLANNED ORDER
========================================

1 A check_fileptr_change.patch: Bug 1116055: Check the unexpected change of file seek position and ask the user to report it

   bug 1116055 

   This was considered a necessary step so that we can know that if
   any user experiences a strange seek position change.

2 A fix-close-step-1.patch: Bug 1122698 Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends (part 1 of  a series)

   bug 1122698
   (This ENTRY)

3 A fix-close-part-2.patch: Bug 1134527 Add error value checking of Close() and Flush() (part-2 of a series)

   bug 1134527

 4 A fix-close-part-3-write-error-check.patch: Bug 1134529: Check Write failures (part-3 of a series)

   bug 1134529

 5 A removing-a-Seek.patch: C-C Thunderbird - Cleaning of incorrect Close, unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (step 4)

   bug 1117450

 6 A 1116055-acelist.patch: One liner patch posted in Bug 1116055 to return a buffered stream from mbox::getNewOutputStream by aceman

   Bug 1176857 - C-C TB Enabling buffering for pop3 download (and possibly part of imap file operation)

   I have created a separate bug for this since it will be necessary
   to apply this patch only after other patches landed. (This is
   because enabling the buffering requires proper error checking of
   |Close()| and |Flush()|. These are only taken care of AFTER 1-5
   patches above are applied.)

   Maybe about a month after the first patch (bug 1116055) above is
   applied, and other patches 2-5 have been applied, I can enable this
   patch. (Assuming there is NO ONE who reports strange seek position
   change on their computer.)

 7 A add-short-read-handling.patch: Bug 1170606 - C-C T-B fixes to take care of short read

   Bug 1170606

   This is theoretically independent of 1-6, but my local patch
   assumes the changes in 1-6 to be in place. (The patch may not apply
   cleanly without 1-6 in advance.)

  There are some additional shor-read issues UNDER M-C portion of the
  source tree.  They can be applied indepdent of this series.

Comment 7

[ISHIKAWA, Chiaki]

As a serious user of TB, I appreciate that TB has been released to the
public for free.
At the same time, I noticed serious I/O issues, and thus
am posting these series of patches.

I hope TB will be more robust, and does not eat my message even in the
face of I/O errors :-) [Well, it did before :-( ]

Summary of the patch in this bugzilla.

- I propose |discardNewMessage| and |finishNewMessage| do not close
  |aOutputStream| argument. See "Why" section below.
  This is important to eradicate a few serious bugs that occur during
  error recovery processing.

- Setting stream pointer variables to nullptr after the stream is
  closed.
  This will help us avoiding

  (1) |Close()| of already closed streams and,

  (2) sometimes seen double-free memory error in the low-level code
    (and crash in C-C TB) due to trying to Close() already streams.

- There was a mix-up of |m_outFileStream| and |inboxInputStream| near
  line 922. This was fixed.  After the fix, C-C TB passes |make
  mozmill| and |make xpcshell-tests|.

  Then I notice that this path on line 922 is taken only when we save
  to temporary file FIRST during pop3 download and THEN copy the
  temporary file to Inbox.  (This has to be set by preference change.)
  I manually tested the operation, and it works. Verified.

Why we do not close |aOutputStream| in the two functions any more:

The function parameter |aOutputStream| is passed |m_tempMessageStream|
or |mCopyState->m_fileStream| during runtime.

These pointer variables need to be set to nullptr after the streams
associated with them are closed so that a second extra close can be
avoided a la following code:

     if (m_tempMessageStream) {
         |m_tempMesageStream->Close();
         m_tempMessageStream = nullptr;
     }

WHAT was wrong:

The two functions originally COPIED the |aOutputStream| value to a
LOCAL VARIABLE and then set that LOCAL variable to nullptr when the
stream was closed.

But this left the original |m_tempMessageStream| or
|mCopyState->m_fileStream| with a dangling pointer now. Stream is
already closed.  Later, when the time comes, the pointer variable is
checked and, since it is not a nullptr, a |Close| is performed.

If we are lucky, we will receive NS_ERROR_BASE_STREAM_CLOSED from
|Close()| which was completely ignored by the code(*2) before the
patch series.

If we are not lucky, we sometimes get double free error in the
lower-level I/O code after an error recovery (*1) due to I/O error is
attempted during testing. C-C TB crashed as a result.  The double-free
does not seem to come from |m_tempMesageStream| or
|mCopyState->m_fileStream| alone.
(There seemed to be OTHER stream pointer
variables that needed to be set to nullptr after stream is closed during
error recovery/processing.)

This double-free does not happen any more after the whole series of
the patches is applied (and especially after stream pointers are set
properly to nullptr almost everywhere where I touched the code for
Pop3/Imap robustness.)

This patch now fixes the first issue by moving the |Close| and setting
nullptr of |aOutputStream| (i.e., |m_tempMessageStream| or
|mCopyState->m_fileStream|) outside the two functions
(|DiscardNewMessage()|, |FinishNewMessage()|.)

The addition of |Close()| calls after the calls to
|DiscardNewMessage()|, |FinishNewMessage()|, and setting the stream
variables to nullptr after the stream associated with them are closed
are main purpose of this patch.

TIA

Note *1: Error recovery is more frequent after the whole series of the
patches is applied since I put in the proper error checks of low-level
I/O function return values, which had been missing for a long time,
and in a few places invoke error return, etc.  [Error recover attempt
is NOT PERFECT YET, but *IS* MUCH BETTER IMHO.]
Because of the added error checks, I experienced more error recovery
attempts during simulated network error testing, and then I
experienced double-free error from memory management routines before
proper |Close| invocations of the stream and setting the stream
pointer variables with wider scope/visibility to nullptr.

Note *2:

The checking of |Close| and other low-level functions come in the
later parches in the series.

PS: I have noticed that return values of |Seek()| and |Tell()| are not
checked very well yet. I will add the check in a planned patch to pick
up these left-over failures to check low-level I/O function return
values (mainly Seek and Tell) after the patch

[7] add-short-read-handling.patch: Bug 1170606 - C-C T-B fixes to take
care of short read

so that existing patches do not have to be modified heavily.

PPS: It might be a good idea to merge the patches of

*- bug 1122698 Cleaning of incorrect Close, unchecked Flush, Write
   in nsPop3Sink.cpp and friends (part 1 of  a series) [This bugzilla]
 - bug 1134527 Add error value checking of Close() and Flush() (part-2
   of a series)
 - bug 1134529 Check Write failures (part-3 of a series)
 - bug 1174500 C-C Thunderbird - Cleaning of incorrect Close,
   unchecked Flush, Write etc. in nsPop3Sink.cpp and friends. (step 4)

into one big patch.
(Yet, still keeping the following patches separate.

- bug 1116055 Performance issue: Failure to use buffered write
  (comm-central thunderbird)
- bug 1176857 - C-C TB Enabling buffering for pop3 download (and
  possibly part of imap file operation)

This may make it easier to check what I/O error is being checked, what
action is taken, etc. in one pass.

Comment 8

[ISHIKAWA, Chiaki]

In bug 1242030, I have uploaded merged patches from the bug 1122698, bug 1134527, bug 1134529, bug  1174500 
The reason for consolidation is that the patch as a whole is much easier to 
read when consolidated.
However, at the same time, I have split the resulting gigantic patch into a few smaller chunks that address files under only certain directories each.

I will upload the latest road map that explains the patches and application order shortly.

TIA

Comment 9

[ISHIKAWA, Chiaki]

As per comment 8, the patch discussed here is now merged and split per directories in bug 1242030.
So I am duping this bugzilla entry to the new bug.

Comment 10

[Wayne Mery (:wsmwk)]

(cancelling NI and review request)