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

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

A typo in comment 0. > This is the step-1 > (from bug 112184) > --- quote --- The quote was taken from Bug 1121842, of course.

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

Update. This patch applies to 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)

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.

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.

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

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.

(cancelling NI and review request)