This is to be a meta bug entry for the following issues. I depend on TB for the workflow at the office and my personal correspondence on several PCs (under linux and Windows). So its correct operation (and smooth operation hopefully) as a cross-platform mailer is very important for me. So I files this bugzilla entry. Problem: There is a confusion as to which routine should be responsible for closing the file stream associated with a variable m_outFileStream in nsPop3Sink.cpp. Because of the confusion (?), there are multiple instances of unnecessary extra bogus Close calls across a few files. Also, Pop3 code is full of unchecked |Close|, |Flush|, and |Write| calls. (Imap code, too. That will be needed to be taken care of eventually. Sorry I am not using imap right now and not feel motivated to tackle that group of files immediately until nsPops3Sink.cpp is fixed. But someone can take the lead after seeing this.) If the underlying file system experiences a glitch, then it is quite likely that thunderbird treats failed download as success and delete the message on the POP3 server happily. Totally unacceptable behavior. [ The glitch of the file system: almost filled-up file system, transient network problems for remotely mounted file system, incorrect file system permission caused by administrative error an an NFS server, USB memory stick where file is to be stored fell of the PC (!) etc. I have personally experienced and confirmed the first three error scenarios with TB before in different context and experienced data loss. I know some people seemed to have suffered from the last one according to bugzilla entries. ] Background of discovery: I tried to enable output buffering in nsPop3Sink.cpp for performance reasons. [ Bug 1116055 - Performance issue: Failure to use buffered write (comm-central thunderbird) ] But When I enabled output buffering in nsPop3Sink.cpp in addition to the patch in bug 1116055, it caused the failure to incorporate download messages. It looks timing dependent. Anyway, I always found a stream closed prematurely before writing finishes. That prompted me to debug and study the code carefully. Then I found that there are bogus Close() calls to already closed file streams in the code. This bogus |Close()| became apparent as soon as I added error checking of returned value of |Close()| in several places. Such extra bogus |Close| calls are RAMPANT during execution, and I confirm it after static code analysis. But then, when I started to think of how to fix the situation, I noticed there seemed to be a confusion about which function should call |Close()| on the buffer stream variable, m_outFileStream in nsPop3Sink.cpp. So I traced the history of m_outFileStream (where it is set, where it is used as parameter to external functions [which in turn may Close it], when the file associated with it are Opened, Closed, etc.) After the analysis, I came up with a plan to clean up the current incorrect code that invokes bogus |Close| on already closed streams. (Such bogus calls interfere with smooth gdb debugging. There are simply too many such calls that would return NS_BASE_STREAM_CLOSED during execution. Oh yes, come to think of it buffered output routine does not report error situation completely to my taste. (See Bug 1120046 - RFC mozilla/netwerk/base/src/nsBufferedStreams.cpp: better error reporting and maybe adding thread-race lock ) And the source code is not quite right statically, too, even if calling Close on already closed stream should be NOOP in principle. It is too confusing to see an already closed stream closed only several lines down while reading the code. ) We should also check the return value of Close, Flush and Write properly. ======================================== Plan for Improvement ======================================== My Plan to clean up the code is as follows. There are four steps. 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. After the removal of unnecessary calls of |Close|, we go to Step-2. Step-2. Add error value checking of Close() and Flush(); First step. Simply add NS_ERROR(). Better than nothing. At least, we will see the error printed during testing of DEBUG version of TB. (For better error handling, we will wait for Step 4.) The patch for this will be posted as a separate bugzilla entry. After Close() and Flush() are taken care of, we check the error return of Write() and the mismatch of the # of written bytes and requested bytes. Step-3. Add error value checking of Write(); Check the return value of Write and if the requested # of bytes matches the # of really written bytes. First step. Simply add NS_ERROR(). Better than nothing. At least, we will see the error printed during testing of DEBUG version of TB. (For better error handling, we will wait for Step 4.) The patch for this will be posted as a separate bugzilla entry. Step-4. More error processing I understand that some places may need elaborate error handling, better than NS_ERROR(), and error return path. Take care of them in Step-4. The patch for this will be posted as a separate bugzilla entry. I think I will need a few different bugzilla entries since in a few places, the error handling seems to be necessarily complex. Step-5. Enabling buffered output. (This is not a correctness fix. This is a performance improvement.) Additionally, introduce buffering output to the file stream created in these files appropriately. I will post the crude memo/write-up in the next comment.

Here is a write-up. Below is the writeup I created initially as comments to nsPop3Sink.cpp while I checked the usage history of m_outFileStream. It was created initially as comments on pristine nsPop3Sink.cpp, and "hg diff" was used to extract it, and then I manually edied it into a text form. So sorry for irregular formating. But essence of what I found to come up with the two proposals is in the following. // // m_outFileStream starts as nullptr in new nsPop3Sink. // // ======================================== // External Usage of m_outFileStream // ---------------------------------------- // Where is it passed to routines ouside nsPop3Sink.cpp? // (we need to figure out if there are Close operation using the // value to track down strange premature Closing of underlying file stream // when buffering is enabled.): / m_outFileStream is passed to external routine in a few places. Let us take a look at them in turn. // - IncorporateBegin // // rv = m_newMailParser->Init(serverFolder, m_folder, // m_window, newHdr, m_outFileStream); // // Init() copies m_outFileStream to // m_newMailParser->m_outFileStream // // So the question becomes WHERE/WHEN m_outFilestream is Closed in // mailnews/local/src/nsParseMailbox.cpp // // Short Answer: m_outFilestream->Close() will be done inside // the two calles in DiscardNewMessage(). // [ After the analysis below I came up with the FINAL PROPOSAL: DiscardNewMessage should not close the file stream passed as the 1st argument. It is caller's responsibility. ] // Long Answer: // - mailnews/local/src/nsParseMailbox.cpp has two calls to DiscardNewMessages. // (a) nsCOMPtr<nsIMsgPluggableStore> msgStore; // nsresult rv = // m_downloadFolder->GetMsgStore(getter_AddRefs(msgStore)); // rv = msgStore->DiscardNewMessage(m_outputStream, m_newMsgHdr); // // (b) nsCOMPtr<nsIMsgPluggableStore> store; // rv = m_downloadFolder->GetMsgStore(getter_AddRefs(store)); // store->DiscardNewMessage(m_outputStream, mailHdr); // There are two implemenations of DiscardNewMessages. // mailnews/local/src/nsMsgBrkMBoxStore.cpp // as member of class nsMsgBrkMBoxStore -- nsMsgBrkMBoxStore::DiscardNewMessage(nsIOutputStream *aOutputStream, // m_outputStream-> Close is called since // aOutputStream->Close() is called. // // mailnews/local/src/nsMsgMaildirStore.cpp // as member of class nsMsgMaildirStore -- nsMsgMaildirStore::DiscardNewMessage(nsIOutputStream *aOutputStream, // Same as in nsMsgBrkMBoxStore.cpp // m_outputStream-> Close is called since // aOutputStream->Close() is called. // So DiscardNewMessage calls Close always now. [ In the end, I am proposing that the aOutputStream->Close() is removed from DiscardNewMessage(), and it is caller's responsibility to close the stream.] m_outFileStream is passed to external routine in: // - IncorporateComplete // case - (a) if (m_downloadingToTempFile) path // // This has the following code snipet. // *AFTER* |Close|, it is referenced. // // m_outFileStream->Close(); // ... // nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream); // rv = MsgReopenFileStream(m_tmpDownloadFile, inboxInputStream); // .... // // DONE: here is what I found out what happens in MsgReopenFileStream. // // mailnews/base/util/nsMsgUtils.cpp // nsresult MsgReopenFileStream(nsIFile *file, nsIInputStream *fileStream) // InitWithFile is called() // nsMsgFileStream *msgFileStream = static_cast<nsMsgFileStream *>(fileStream); // if (msgFileStream) // return msgFileStream->InitWithFile(file); // // *THEN* what about InitWithFile. // // Short ANSER: Opened for R/w (and mFileSesc is updated.) // XXX: TODO/FIXME? Shouldnt we re-buffer this stream. There is a performance issue. We may want to perform buffered output. // // mailnews/base/util/nsMsgFileStream.cpp // member of class nsMsgFileStream -- nsresult nsMsgFileStream::InitWithFile(nsIFile *file) // RW file is opened. // file->OpenNSPRFileDesc(PR_RDWR|PR_CREATE_FILE, 0664, &mFileDesc // The following three |InitWithFile()|s seem to be unrelated to File stream. // mozilla/netwerk/ipc/RemoteOpenFileChild.cpp (View Hg log or Hg annotations) // member of class RemoteOpenFileChild -- RemoteOpenFileChild::InitWithFile(nsIFile *aFile) // NOT_IMPLEMENTED // mozilla/xpcom/io/nsLocalFileWin.cpp // member of class nsLocalFile -- nsLocalFile::InitWithFile(nsIFile* aFile) // This one seems to set pathname to a field only. no opening??? // mozilla/xpcom/io/nsLocalFileCommon.cpp (View Hg log or Hg annotations) // member of class nsLocalFile -- nsLocalFile::InitWithFile(nsIFile* aFile) // This one seems to set pathname to a field only. no opening??? // (after character code conversion possibly.) an nsIInputStream related to m_outFileStream: // // NOTE: There is a filestream related to m_outFileStream. // nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream); // This is created and used inside // nsPop3Sink::IncorporateComplete(nsIMsgWindow *aMsgWindow, int32_t aSize) // // The name is inboxInputStream, and it is passed to // |AppendMsgFromStream|. // // rv = m_newMailParser->AppendMsgFromStream(inboxInputStream, hdr, // msgSize, m_folder); // // I wonder what happens in AppendMsgFromStream to inboxInputStream // and what happens to m_outFileStream as a result. // // DONE: investigated. // tidied up AppendMsgFromStream listing from mxr // mailnews/local/src/nsParseMailbox.h (View Hg log or Hg annotations) nsresult AppendMsgFromStream(nsIInputStream *fileStream, nsIMsgDBHdr *aHdr, Referenced (in 2 files total) in: mailnews/local/src/nsParseMailbox.cpp (View Hg log or Hg annotations) line 2367 -- nsresult nsParseNewMailState::AppendMsgFromStream(nsIInputStream *fileStream, This is the definition of AppendMsgFromStream. // fileStream is used for *READING* // No explicit close of it (?). // output for append is done by creating a filestream internally to // AppendMsgFromStream and is closed there (or in // store->FinishNewMessage.) PERFORMANCE ISSUE TODO/FIXME: make this outupt stream perform buffered output. // mailnews/local/src/nsParseMailbox.cpp (View Hg log or Hg annotations) line 2507 -- rv = AppendMsgFromStream(inputStream, newHdr, messageLength, destIFolder); Caller does not seem to close inputStream explicitly (directly). mailnews/local/src/nsPop3Sink.cpp (View Hg log or Hg annotations) line 815 -- rv = m_newMailParser->AppendMsgFromStream(inboxInputStream, hdr, This is the reference within nsPop3Sink.cpp m_outFileStream is passed to external routine (cont'ed): 2nd path in IncpoprateComplete // - IncorporateComplete // case- (b) if (m_downloadingToTempFile) ... ELSE path // m_msgStore->FinishNewMessage(m_outFileStream, hdr); // Will it be |Closed| in FinishNewMessage? Below is the problem noticed immediately. INCONSISTENT HANDLING of the 1st file stream argument inside FinishNewMessage defined in nsMsgBrkMBoxStore.cpp and nsMsgMaildirStore.cpp (the latter closes it) while the former does not touch it. // ../../../mailnews/local/src/nsMsgBrkMBoxStore.cpp:711:nsMsgBrkMBoxStore::FinishNewMessage(nsIOutputStream *aOutputStream, // // NOOP: nothing happens to m_outFileStream. // ../../..//mailnews/local/src/nsMsgMaildirStore.cpp:684:nsMsgMaildirStore::FinishNewMessage(nsIOutputStream *aOutputStream, // // Closed! // m_outFileStream->Close() called. // (since aOuptutStream->Close() is called. ) // [ After the analysis like this I came up with the FINAL PROPOSAL: FinishNewMessage should not close the file stream passed as the first argument. It is caller's responsibility to close it.] In order to verify that this proposal works, I checked what the callers of FinishNewMessage expect and do today? (See the analysis at the end.) In a nustshell, though, most of the callers assume that the caller needs to close the stream. m_outFileStream is passed to externa routine in: (con'ted) // - IncorporateAbort // // *AFTER* |Close|, it is passed to DiscardNewMessage // // nsresult rv = m_outFileStream->Close(); // less some error checking // m_msgStore->DiscardNewMessage(m_outFileStream, // m_newMailParser->m_newMsgHdr); // // What happens to m_outFileStream in DiscardNewMessage? // // DONE: Here is what I found out. // XXX TODO/FIXME : // BAD: although it is already Closed, DiscardNewMessage again // Close() it inside. // ======================================== Where is a file stream opened related to m_outFileStream? ======================================== // // |Open| // // Where does m_outFileStream is created [opened]? // - IncorporateBegin: // Created either from a temp file: // rv = MsgGetFileStream(m_tmpDownloadFile, getter_AddRefs(m_outFileStream)); // or from (less error checking) // rv = server->GetMsgStore(getter_AddRefs(m_msgStore)); // m_msgStore->GetNewMsgOutputStream(m_folder, getter_AddRefs(newHdr), // &reusable, getter_AddRefs(m_outFileStream)); ======================================== Where is a file stream related to m_outFileStream Closed? ======================================== I found out numerous bogus calls to already closed stream. This is bad. // // |Close| in nsPop3Sink.cpp // // Where does m_outFileStream is closed? // - EndMailDelivery: closes it and set m_outFileStream to nullptr. // - AbortMailDelivery: ditto. // - IncorporateComplete: closes it. Maybe we should set m_outFileStream to nullptr? // - IncorporateAbort // But after the close, m_outFileStream is passed to // m_msgStore->DiscardNewMessage(m_outFileStream, // m_newMailParser->m_newMsgHdr); // which again calls ->Close inside. This looks wrong/BAD (XXX): Double Close. // // // When a seekable file stream created from // an NSIFile is closed, what happens to the original file stream? // (its underlying low-level file stream, that is ) // // XXX TODO: find out // // seekableOutStream is created: // - WriteLineToMailbox // nsCOMPtr <nsISeekableStream> seekableOutStream = do_QueryInterface(m_outFileStream); // seekableOutStream->Seek(nsISeekableStream::NS_SEEK_END, 0); // to write to the end of the file. // // The following was investigated alreay elsewhere // What happens to m_outFileStream in // - nsPop3Sink::IncorporateComplete // m_outFileStream->Close(); // m_newMailParser->FinishHeader(); // ... // nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream); // rv = MsgReopenFileStream(m_tmpDownloadFile, inboxInputStream); // // a few extra fixes. // ??? XXX: Following is no longer used? Strange??? TODO/FIXME: check out why nsCOMPtr<nsISeekableStream> seekableOutStream = do_QueryInterface(m_outFileStream); Fix multiple close in nsPop3Sink::IncorporateComplete(nsIMsgWindow *aMsgWindow, int32_t aSize) Removes extra Flush() ======================================== ======================================== What do callers of FinishNewMessage expect? ---------------------------------------- |FinishNewMessage| tidied up listing of FinishNewMessage mailnews/base/public/nsIMsgPluggableStore.idl (View Hg log or Hg annotations) line 157 -- void finishNewMessage(in nsIOutputStream aOutputStream, 149 /** 150 * Must be called by code that calls getNewMsgOutputStream to finish 151 * the process of storing a new message, if the new msg has not been 152 * discarded. Could/should this be combined with discardNewMessage? 153 * 154 * @param aOutputStream stream we were writing the message to. 155 * @param aNewHdr header of message finished. 156 */ 157 void finishNewMessage(in nsIOutputStream aOutputStream, 158 in nsIMsgDBHdr aNewHdr); 159 The spec above is not clear about what it does on the file stream argument. mailnews/base/util/nsMsgDBFolder.cpp (View Hg log or Hg annotations) line 1788 -- msgStore->FinishNewMessage(m_tempMessageStream, m_offlineHeader); OK: Close the stream on the caller side. the code below clearly shows that the caller side closes it. XXX/BAD: no check on the return value of Close(); 1787 if (msgStore) 1788 msgStore->FinishNewMessage(m_tempMessageStream, m_offlineHeader); 1789 1790 m_offlineHeader = nullptr; 1791 if (m_tempMessageStream) 1792 { 1793 m_tempMessageStream->Close(); 1794 m_tempMessageStream = nullptr; 1795 } 1796 return NS_OK; mailnews/import/applemail/src/nsAppleMailImport.cpp (View Hg log or Hg annotations) line 564 -- msgStore->FinishNewMessage(outStream, msgHdr); OK: Close the stream on the caller side. XXX: No check on the return value of Close(); XXX/BAD BAD BAD: Double Close() // add the data to the mbox stream 562 if (NS_SUCCEEDED(nsEmlxHelperUtils::AddEmlxMessageToStream(currentEntry, outStream))) { 563 mProgress++; 564 msgStore->FinishNewMessage(outStream, msgHdr); 565 } 566 else { 567 msgStore->DiscardNewMessage(outStream, msgHdr); Since DiscardNewMessage() closes outStream, we would call Close() extra times below. 568 break; 569 } 570 if (!reusable) 571 outStream->Close(); set outStream to nullptr to avoid double Close() more more time again below :-( 572 } 573 if (outStream) 574 outStream->Close(); [ After the analysis like this, I came up with the FINAL PROPOSAL: DiscardNewMessage should not close the file stream passed as its first argument. It is caller's responsibility to close it.] XXX: possibility for a fix. Do not let DiscardNewMessage close the stream. We need to check the usage of DiscardNewMessage to do this (TODO/FIXME). What do calles of DiscardNewMessage expect? See the analysis at the end. The analysis showed my final proposal is OK. mailnews/import/eudora/src/nsEudoraMailbox.cpp (View Hg log or Hg annotations) line 307 -- msgStore->FinishNewMessage(outputStream, msgHdr); OK Caller side close XXX: no check on Close XXX: multiple Closes line 423 -- msgStore->FinishNewMessage(outputStream, msgHdr); OK Caller side close XXX: no check on Close XXX: multiple Closes mailnews/import/oexpress/nsOE5File.cpp (View Hg log or Hg annotations) line 471 -- msgStore->FinishNewMessage(outputStream, msgHdr); OK Caller side close XXX: no check on Close XXX: multiple Closes mailnews/import/oexpress/nsOEMailbox.cpp (View Hg log or Hg annotations) line 330 -- m_msgStore->FinishNewMessage(m_dstOutputStream, msgHdr); NEED CAUTION NG: No Caller side close it returns without closing a file stream. !? XXX: TODO/FIXME: probably should close. mailnews/import/outlook/src/nsOutlookMail.cpp (View Hg log or Hg annotations) line 427 -- msgStore->FinishNewMessage(outputStream, msgHdr); OK Caller side close XXX: no check on Close XXX: multiple Closes Referenced (in 8 files total) in: mailnews/imap/src/nsImapMailFolder.cpp (View Hg log or Hg annotations) line 7280 -- offlineStore->FinishNewMessage(outputStream, newMailHdr); OK caller side close. (outputStream is created in the caller function.) XXX: no check on close line 8385 -- msgStore->FinishNewMessage(offlineStore, fakeHdr); OK caller side close (offlineStore is created in the caller function.) XXX: no check on close mailnews/imap/src/nsImapService.cpp (View Hg log or Hg annotations) line 2096 -- msgStore->FinishNewMessage(offlineStore, fakeHdr); OK caller side close XXX: no check on close mailnews/local/src/nsLocalMailFolder.cpp (View Hg log or Hg annotations) line 2258 -- rv = mCopyState->m_msgStore->FinishNewMessage(mCopyState->m_fileStream, OK Caller side close. XXX: no check on the return value of Close XXX: TODO/FIXME: BUG if FinishNewMessage closes the stream (the 1st argument) we may have a serious problem!? from the code below. (I have no idea what happens if !multipleCopiesFinished on line 2262. 2258 rv = mCopyState->m_msgStore->FinishNewMessage(mCopyState->m_fileStream, 2259 mCopyState->m_newHdr); 2260 if (NS_SUCCEEDED(rv) && mCopyState->m_newHdr) 2261 mCopyState->m_newHdr->GetMessageKey(&mCopyState->m_curDstKey); 2262 if (multipleCopiesFinished) 2263 mCopyState->m_fileStream->Close(); 2264 else 2265 mCopyState->m_fileStream->Flush(); line 2582 -- rv = mCopyState->m_msgStore->FinishNewMessage(mCopyState->m_fileStream, OK Caller side Close. XXX: no check on the return value of Close. line 3595 -- msgStore->FinishNewMessage(outFileStream, newHdr); OK Caller side Close XXX: no check on the return value of Close. mailnews/local/src/nsMovemailService.cpp (View Hg log or Hg annotations) line 499 -- rv = msgStore->FinishNewMessage(outputStream, newHdr); OK Caller Close XXX: no check on the return value of Close Possible Serious Issue. XXX/TODO FIXME: somewhat warped code structure. I am not sure if outputStream may be assigned different file streams during While loop and if that is the case, then we fail to close all of them except the last one (we close it outside the while loop). I wonder while loop is executed only once. But I have no idea. Maybe we can use the Close immediately after use inside the loop. : that is, // Finish previous header and message, if any. if (newHdr) { ... at the end. } In any case, better comment would be a good thing to have here. line 544 -- rv = msgStore->FinishNewMessage(outputStream, newHdr); OK Caller side Close XXX: no check on the return value of Close mailnews/local/src/nsParseMailbox.cpp (View Hg log or Hg annotations) line 2417 -- return store->FinishNewMessage(destOutputStream, aHdr); NG a rare *EXCEPTION*... *CAUTION*: The caller expects FinishNewMessage to close the stream! XXX: no check on close. XXX: double Close when the stream is a reusable one? *WAIT!* A serious *BUG*!!! We should NOT close the stream before usge in FinishNewMessage. Well actualy as of now, FinishNewMEssage only refers to the first argument to invoke Close() once. But still, in this case, DOUBLE Close()!. 2414 // non-reusable streams will get closed by the store. 2415 if (reusable) 2416 destOutputStream->Close(); 2417 return store->FinishNewMessage(destOutputStream, aHdr); 2418 } Change the order of call to FinishNewMessage and Close and always close on the caller side. rvfinal = store->FinishNewMessage(destOutputStream, aHdr); rv = destOutputStream->Close(); if (NS_FAILED(rv)) { ... maybe we should fail... } return rvfinal; mailnews/local/src/nsMsgMaildirStore.cpp (View Hg log or Hg annotations) line 704 -- NS_ERROR("FinishNewMessage - no storeToken in msg hdr!!\n"); line 738 -- NS_ERROR("FinishNewMessage - oops! file does not exist!"); line 777 -- NS_ERROR("FinishNewMessage - no storeToken in msg hdr!!\n"); line 792 -- NS_ERROR("FinishNewMessage - oops! file does not exist!"); mailnews/local/src/nsPop3Sink.cpp (View Hg log or Hg annotations) line 831 -- m_msgStore->FinishNewMessage(m_outFileStream, hdr); No close on caller side. CAUTION: we should close it on caller side, and not let FinishMessage close the stream. mailnews/import/test/unit/resources/TestMailImporter.js (View Hg log or Hg annotations) line 114 -- msgStore.finishNewMessage(outputStream, newMsgHdr); Caller side clode. XXX: No check on the return value of close Huh, everybody seems to have a perfect and infinte storage, and network never experiences transient errors. Sigh... ======================================== Re: DisardNewMessage: ---------------------------------------- Current Status : What the Callers of DiscardNewMessage do regarding the file stream passed as the argument to DiscardNewMessage(). tidied up listing of discardNewMessage from mxr Defined as a function prototype in: mailnews/base/public/nsIMsgPluggableStore.idl (View Hg log or Hg annotations) line 146 -- void discardNewMessage(in nsIOutputStream aOutputStream, Spec is not clear about the closing of aOutputStream. Probably it should not close within discardNewMessage considering the safetey practice of setting stream variable to nullptr when stream is closed. Closing should be done outside. mailnews/base/util/nsMsgDBFolder.cpp (View Hg log or Hg annotations) line 1766 -- msgStore->DiscardNewMessage(m_tempMessageStream, m_offlineHeader); OK: caller side takes care of close. XXX: no check on ->Close... mailnews/import/applemail/src/nsAppleMailImport.cpp (View Hg log or Hg annotations) line 567 -- msgStore->DiscardNewMessage(outStream, msgHdr); OK: Caller side close XXX: no check on -> Close XXX/BAD BAD BAD: multiple Closes. mailnews/import/eudora/src/nsEudoraMailbox.cpp (View Hg log or Hg annotations) line 309 -- msgStore->DiscardNewMessage(outputStream, msgHdr); OK: Caller side close XXX: no check on -> Close XXX/BAD BAD BAD: multiple Closes if (NS_SUCCEEDED(rv)) 307 msgStore->FinishNewMessage(outputStream, msgHdr); 308 else { 309 msgStore->DiscardNewMessage(outputStream, msgHdr); 310 IMPORT_LOG0( "*** Error importing message\n"); 311 } 312 313 if (!reusable) 314 outputStream->Close(); should set outputStream to nullptr to avoid double Close() below. 315 316 if (!readBuffer.m_bytesInBuf && (state.offset >= state.size)) 317 break; 318 } 319 if (outputStream) 320 outputStream->Close(); should set outputStream to nullptr ? line 425 -- msgStore->DiscardNewMessage(outputStream, msgHdr); Similar to the code above. OK Caller side close XXX: no check on -> Close XXX/BAD: multiple Closes mailnews/import/oexpress/nsOE5File.cpp (View Hg log or Hg annotations) line 467 -- msgStore->DiscardNewMessage(outputStream, msgHdr); OK Caller side close XXX: no check on -> Close XXX/BAD: multiple Closes mailnews/import/oexpress/nsOEMailbox.cpp (View Hg log or Hg annotations) line 332 -- m_msgStore->DiscardNewMessage(m_dstOutputStream, msgHdr); OK Caller side close XXX: no check on -> Close The function that invokes DiscardNewMessage() bool CMbxScanner::WriteMailItem may return without closing when the m_dstOuptuStream is reusable. BUG. Inconsistent? File Descriptor Leak? XXX: TODO/FIXME should we not close it? bool reusable; 317 318 rv = m_msgStore->GetNewMsgOutputStream(m_dstFolder, getter_AddRefs(msgHdr), &reusable, 319 getter_AddRefs(m_dstOutputStream)); 320 if (NS_FAILED(rv)) 321 { 322 IMPORT_LOG1( "Mbx getting outputstream error: 0x%lx\n", rv); 323 return false; 324 } 325 326 // everything looks kosher... 327 // the actual message text follows and is values[3] bytes long... 328 bool copyOK = CopyMbxFileBytes(flags, values[3]); 329 if (copyOK) 330 m_msgStore->FinishNewMessage(m_dstOutputStream, msgHdr); 331 else { 332 m_msgStore->DiscardNewMessage(m_dstOutputStream, msgHdr); 333 IMPORT_LOG0( "Mbx CopyMbxFileBytes failed\n"); 334 } 335 if (!reusable) 336 { 337 m_dstOutputStream->Close(); 338 m_dstOutputStream = nullptr; 339 } 340 return copyOK; It seems that the code "assumed" that FinishNewMessage and DiscardNewMessage will not close the m_dstOutputStream, which is unfortunately untrue. We should close it anyway on line 335-338 no matter what reusable value is and set m_dstOutputStream to nullptr. mailnews/import/outlook/src/nsOutlookMail.cpp (View Hg log or Hg annotations) line 431 -- msgStore->DiscardNewMessage(outputStream, msgHdr); OK Caller Side Close XXX: No check on Close XXX: double close Referenced (in 3 files total) in: mailnews/local/src/nsLocalMailFolder.cpp (View Hg log or Hg annotations) line 2208 -- mCopyState->m_msgStore->DiscardNewMessage(mCopyState->m_fileStream, OK Caller Side Close XXX: no check on Close(); (The following file needs change.) mailnews/local/src/nsParseMailbox.cpp (View Hg log or Hg annotations) line 1823 -- rv = msgStore->DiscardNewMessage(m_outputStream, m_newMsgHdr); this is within 1782 int32_t nsParseNewMailState::PublishMsgHeader(nsIMsgWindow *msgWindow) Need CAUTION Caller side NO Close? XXX: Maybe we can close it on the caller, but I am not sure whether PublishMsgHeader can close it or not... Again what the callers of PublishMsgHeader expect (although as of now, DiscardNewMessage closes m_outputStream anyway.) Move the close on caller side by not having DiscardNewMessage invoke Close. line 2562 -- store->DiscardNewMessage(m_outputStream, mailHdr); This is within 2424 nsresult nsParseNewMailState::MoveIncorporatedMessage(nsIMsgDBHdr *mailHdr, Caller side NO Close? The same comment as above. XXX: Move the close on caller side by not having DiscardNewMessage invoke Close. mailnews/local/src/nsPop3Sink.cpp (View Hg log or Hg annotations) line 862 -- m_msgStore->DiscardNewMessage(m_outFileStream, Within IncorporateAbort Need CAUTION Caller side no close XXX: may close on the caller side. XXX: TODO/FIXME ===> Do not let DiscardNewMessage close the stream (its 1st argumen) and let the caller side to handle the close. [AGAIN, after the analysis above, I came up with the proposal that DiscardNewMessage should NOT close the first file argument, and the caller should close it if necessary. ======================================== A several places in code that may need extra changes. NS_IMPL_ISUPPORTS(nsPop3Sink, nsIPop3Sink) nsPop3Sink::nsPop3Sink() { m_authed = false; m_downloadingToTempFile = false; m_biffState = 0; m_numNewMessages = 0; @@ -501,16 +697,17 @@ nsPop3Sink::IncorporateBegin(const char* &reusable, getter_AddRefs(m_outFileStream)); } // The following (!m_outFileStream etc) was added to make sure that we don't // write somewhere where for some reason or another we can't write to and // lose the messages. See bug 62480 if (!m_outFileStream) return NS_ERROR_OUT_OF_MEMORY; + // ??? XXX: Following is no longer used? Strange??? TODO/FIXME: check out why nsCOMPtr<nsISeekableStream> seekableOutStream = do_QueryInterface(m_outFileStream); // create a new mail parser if (!m_newMailParser) m_newMailParser = new nsParseNewMailState; NS_ENSURE_TRUE(m_newMailParser, NS_ERROR_OUT_OF_MEMORY); if (m_uidlDownload) m_newMailParser->DisableFilters(); @@ -676,16 +873,19 @@ nsresult nsPop3Sink::WriteLineToMailbox( // The following (!m_outFileStream etc) was added to make sure that we don't write somewhere // where for some reason or another we can't write to and lose the messages // See bug 62480 NS_ENSURE_TRUE(m_outFileStream, NS_ERROR_OUT_OF_MEMORY); // seek to the end in case someone else has seeked elsewhere in our stream. nsCOMPtr <nsISeekableStream> seekableOutStream = do_QueryInterface(m_outFileStream); seekableOutStream->Seek(nsISeekableStream::NS_SEEK_END, 0); + + // XXX: what happens if the thread is interrupted here? + uint32_t bytesWritten; m_outFileStream->Write(buffer.BeginReading(), bufferLen, &bytesWritten); NS_ENSURE_TRUE(bytesWritten == bufferLen, NS_ERROR_FAILURE); } return NS_OK; } nsresult nsPop3Sink::HandleTempDownloadFailed(nsIMsgWindow *msgWindow) @@ -728,8 +928,17 @@ nsresult nsPop3Sink::HandleTempDownloadF m_newMailParser->m_newMsgHdr = nullptr; return (dlgResult == 0) ? NS_OK : NS_MSG_ERROR_COPYING_FROM_TMP_DOWNLOAD; } return rv; } +// +// 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); + +// +// NS_IMETHODIMP nsPop3Sink::IncorporateComplete(nsIMsgWindow *aMsgWindow, int32_t aSize) { if (m_buildMessageUri && !m_baseMessageUri.IsEmpty() && m_newMailParser && m_newMailParser->m_newMsgHdr) { nsMsgKey msgKey; m_newMailParser->m_newMsgHdr->GetMessageKey(&msgKey); @@ -780,28 +1019,30 @@ nsPop3Sink::IncorporateComplete(nsIMsgWi rv = GetMsgDBHdrFromURI(m_origMessageUri.get(), getter_AddRefs(oldMsgHdr)); if (NS_SUCCEEDED(rv) && oldMsgHdr) localFolder->UpdateNewMsgHdr(oldMsgHdr, hdr); } if (m_downloadingToTempFile) { // close file to give virus checkers a chance to do their thing... - m_outFileStream->Flush(); + m_outFileStream->Flush(); // don't need this, do we? m_outFileStream->Close(); m_newMailParser->FinishHeader(); // need to re-open the inbox file stream. bool exists; m_tmpDownloadFile->Exists(&exists); if (!exists) return HandleTempDownloadFailed(aMsgWindow); nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream); rv = MsgReopenFileStream(m_tmpDownloadFile, inboxInputStream); NS_ENSURE_SUCCESS(rv, HandleTempDownloadFailed(aMsgWindow)); + + // Should this reference be inboxInputStream??? if (m_outFileStream) { int64_t tmpDownloadFileSize; uint32_t msgSize; hdr->GetMessageSize(&msgSize); // we need to clone because nsLocalFileUnix caches its stat result, // so it doesn't realize the file has changed size. nsCOMPtr <nsIFile> tmpClone; @@ -812,16 +1053,22 @@ nsPop3Sink::IncorporateComplete(nsIMsgWi if (msgSize > tmpDownloadFileSize) rv = NS_MSG_ERROR_WRITING_MAIL_FOLDER; else rv = m_newMailParser->AppendMsgFromStream(inboxInputStream, hdr, msgSize, m_folder); if (NS_FAILED(rv)) return HandleTempDownloadFailed(aMsgWindow); + // 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); } else { return HandleTempDownloadFailed(aMsgWindow); // need to give an error here. } @@ -846,21 +1093,30 @@ nsPop3Sink::IncorporateComplete(nsIMsgWi printf("Incorporate message complete.\n"); #endif nsCOMPtr<nsIPop3Service> pop3Service(do_GetService(NS_POP3SERVICE_CONTRACTID1, &rv)); NS_ENSURE_SUCCESS(rv, rv); pop3Service->NotifyDownloadProgress(m_folder, ++m_numMsgsDownloaded, m_numNewMessages); return NS_OK; } +// Incorporate Abort +// +// m_outFileStream is closed always +// NS_IMETHODIMP nsPop3Sink::IncorporateAbort(bool uidlDownload) { // In IncorporateComplete we call m_FileStream->Close. // Maybe the following m_outFileStream->Close() // should come AFTER *the* if statement? // nsresult rv = m_outFileStream->Close(); NS_ENSURE_SUCCESS(rv,rv); + if (!m_downloadingToTempFile && m_msgStore && m_newMailParser && m_newMailParser->m_newMsgHdr) { m_msgStore->DiscardNewMessage(m_outFileStream, m_newMailParser->m_newMsgHdr); } #ifdef DEBUG printf("Incorporate message abort.\n"); ----

Step 1 is addressed by Bug 1122698. Step 2 is addressed by Bug 1134527. Step 3 is addressed by Bug 1134529.

(In reply to ISHIKAWA, Chiaki from comment #2) > Step 1 is addressed by Bug 1122698. > Step 2 is addressed by Bug 1134527. > Step 3 is addressed by Bug 1134529. step-5 is Bug 1116055 - Performance issue: Failure to use buffered write (comm-central thunderbird) So actually step-5 will come before step-4.

(In reply to ISHIKAWA, Chiaki from comment #3) > (In reply to ISHIKAWA, Chiaki from comment #2) > > Step 1 is addressed by Bug 1122698. > > Step 2 is addressed by Bug 1134527. > > Step 3 is addressed by Bug 1134529. > > step-5 is > Bug 1116055 - Performance issue: Failure to use buffered write (comm-central > thunderbird) > > So actually step-5 will come before step-4. I re-arranged a few things and entered a few bugzilla entries. This is my plan. ======================================== 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 * 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 1174500 [was mistyped as 1117450 in a few bugzilla entries. :-( ] 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. Please feel free to take up the review or move it to somebody else based on the workload of the potential reviewers. TIA

The design of not closing the passed stream inside FinishNewMessage and DiscardNewMessage had to be changed a little. The version of FinishNewMessage and DiscardNewMessage for Maildir storage format (there are ones for imap and pop3 local storage, too) need to close the stream to cope with the issue of the failure of renaming/removing a file when an open file descriptor exists for that file UNDER WINDOWS. Here is a short write up I wrote to explain the background of this change. I have finally resolved the issues with my patch set, and Windows mozmill and xpcshell tests on try-comm-central run without errors! https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ba9d63b23951 History of the final bugs that kept me from asking for formal reviews: The errors occurred only with Maildir usage. It was rather difficult to diagnose the issues that happen only under Windows since I rely try-comm-central build and test for debugging. After checking the return code of various calls leading up to the errors, and dumping file pathnames involved in errors, I finally figured that most of the errors seemed to be failures of copying a message file from a directory to the other for Maildir operation. I could not figure out whether the file to be copied was finally there or not. So I finally bit the bullet and inserted code to dump directory listing under windows to see if the message files are there as they are supposed to be. (I basically called "dir filepath", "dir directorypath" to obtain the listing.) With the new dump listing, I found that the message files WERE there. And so I was motivated to learn further WHERE the error to copy/move occurred. The further detailed error check showed that CopySingleFile() failed to move the message file from tmp to cur directory or something like that. Coupled with some strange errors about NS_ERROR_FILE_IS_LOCKED which I saw earlier, I could only think that there are open file descriptors to the message files, which prevented TB from copying/moving the message files UNDER WINDOWS ONLY. (Under linux and OSX, and other OSes that have UNIX UFS-like filesystem semantics, we can move/copy/rename files even if there are open file descriptors to the file. But Windows does not allow us to do that.) So I re-read my patch set carefully and considering that bisecting only indicated that very fundamental (i.e., early) part of my patch set was to blame, I finally figured that my NOT closing file stream in FinishNewMessage(), especially the Maildir version in nsMsgMaildirStore.cpp, was to blame. FinishNewMessage() for Maildir operation tries to move the freshly downloaded/inserted message to a new location at the end. And it seemed that the closing of the passed file stream is essential for Maildir. So I tweaked the code to close the stream ONLY IN the version of FinishNewMessage and DiscardNewMessage() in nsMsgMaildirStore.cpp. I need to close the file stream in DiscardNewMessage() since it tries to REMOVE the message file. OTOH, I want to handle the Close() of the file stream to the newly downloaded/copied/moved message file (or mbox, etc.) on the caller's side so that yet-to-be-devised better error-handling could be done in the logically higher-level of routines (those that call FinishNewMessage or DiscardNewMessage). So how to fuse the dichotomy of different versions? I decided to add a third argument to the pair of functions to return the indicator whether these functions closed the passed file stream (as the 1st argument) before returning. By looking at the indicator, the callers can decide whether to close the file stream and check the error code on their own. With the change, Windows DEBUG build on try-comm-central ran without a hitch! I can tell. I will post a summary of the new patch and revised roadmap to bug 1116055. TIA

This bug is now being worked on bug 1242030 after a few incarnations.