bug 1242030: DONT-USE-REUSABLE new part-5 new splitting of (consolidation of 1122698, 1134527, 1134529, 1174500) draft
authorISHIKAWA, Chiaki <ishikawa@yk.rim.or.jp>
Sat, 09 Nov 2019 11:11:57 +0900
changeset 81102 a1fa995a885e034c98b434ec943077d204ce27d6
parent 81101 380ec5a67f05dbc96d31b262be2dbf01737f3df4
child 81103 9dd140559a2c2e692c787cbdba37e364b90034f2
push id9745
push userishikawa@yk.rim.or.jp
push dateSat, 09 Nov 2019 02:12:14 +0000
treeherdertry-comm-central@f62e5475decc [default view] [failures only]
bugs1242030, 1122698, 1134527, 1134529, 1174500
bug 1242030: DONT-USE-REUSABLE new part-5 new splitting of (consolidation of 1122698, 1134527, 1134529, 1174500)
mailnews/local/src/nsLocalMailFolder.cpp
mailnews/local/src/nsPop3Protocol.cpp
mailnews/local/src/nsPop3Service.cpp
mailnews/local/src/nsPop3Sink.cpp
mailnews/local/src/nsPop3Sink.h
--- a/mailnews/local/src/nsLocalMailFolder.cpp
+++ b/mailnews/local/src/nsLocalMailFolder.cpp
@@ -2207,16 +2207,23 @@ NS_IMETHODIMP nsMsgLocalMailFolder::EndC
       }
       // XXX TODO better error processing.
       // XXX TODO ??? do we have to call mCopyState->m_fileStream->Close() here?
       // It seems so.
       if (mCopyState->m_fileStream) {
         rv3 = mCopyState->m_fileStream->Close();
         if (NS_FAILED(rv3))
           NS_WARNING("mCopyState->m_fileStream->Close() failed");
+        mCopyState->m_fileStream = nullptr;
+#ifdef DEBUG
+        fprintf(
+            stderr,
+            "{debug} Possibly premature mCopyState->m_fileStream->Close() \n"
+            "          and setting of mCopyState->m_fileStream to nullptr\n");
+#endif
       }
     }
     // mCopyState->m_fileStream is nullptr here and so Close() is unnecesary.
 
     if (!mCopyState->m_isMove) {
       // passing true because the messages that have been successfully
       // copied have their corresponding hdrs in place. The message that has
       // failed has been truncated so the msf file and berkeley mailbox
--- a/mailnews/local/src/nsPop3Protocol.cpp
+++ b/mailnews/local/src/nsPop3Protocol.cpp
@@ -143,17 +143,33 @@ static Pop3UidlHost *net_pop3_load_state
   popState->AppendNative(NS_LITERAL_CSTRING("popstate.dat"));
 
   nsCOMPtr<nsIInputStream> fileStream;
   nsresult rv =
       NS_NewLocalFileInputStream(getter_AddRefs(fileStream), popState);
   // It is OK if the file doesn't exist. No state is stored yet.
   // Return empty list without warning.
   if (rv == NS_ERROR_FILE_NOT_FOUND) return result;
-  // Warn for other errors.
+    // Warn for other errors.
+#ifdef DEBUG
+  if (NS_FAILED(rv)) {
+    nsAutoCString nativePath;
+    nsresult rv2 = popState->GetNativeTarget(nativePath);
+    fprintf(stderr,
+            "(debug) net_pop3_load_state: NS_NewLocalFileInputStream() for "
+            "popState failed.\n");
+    if (NS_FAILED(rv2)) {
+      fprintf(stderr,
+              "(debug) popState->GetNativeTarget failed. rv2=0x%" PRIx32 "\n",
+              static_cast<uint32_t>(rv2));
+    } else
+      fprintf(stderr, "(debug) popState->nativePath = <<%s>>\n",
+              nativePath.Data());
+  }
+#endif
   NS_ENSURE_SUCCESS(rv, result);
 
   nsCOMPtr<nsILineInputStream> lineInputStream(
       do_QueryInterface(fileStream, &rv));
   NS_ENSURE_SUCCESS(rv, result);
 
   bool more = true;
   nsCString line;
@@ -219,17 +235,19 @@ static Pop3UidlHost *net_pop3_load_state
             put_hash(current->hash, uidl->get(), flag, dateReceived);
           } else {
             NS_ASSERTION(false, "invalid flag in popstate.dat");
           }
         }
       }
     }
   }
-  fileStream->Close();
+  nsresult rv3 = fileStream->Close();
+  if (NS_FAILED(rv3)) NS_WARNING("fileStream->Close() failed;");
+  fileStream = nullptr;  // code uniformity
 
   return result;
 }
 
 static int hash_clear_mapper(PLHashEntry *he, int msgindex, void *arg) {
   Pop3UidlEntry *uidlEntry = (Pop3UidlEntry *)he->value;
   PR_Free(uidlEntry->uidl);
   PR_Free(uidlEntry);
@@ -256,17 +274,23 @@ static int net_pop3_write_mapper(PLHashE
       (uidlEntry->status == KEEP) || (uidlEntry->status == DELETE_CHAR) ||
           (uidlEntry->status == FETCH_BODY) || (uidlEntry->status == TOO_BIG),
       "invalid status");
   char *tmpBuffer =
       PR_smprintf("%c %s %d" MSG_LINEBREAK, uidlEntry->status,
                   (char *)uidlEntry->uidl, uidlEntry->dateReceived);
   PR_ASSERT(tmpBuffer);
   uint32_t numBytesWritten;
-  file->Write(tmpBuffer, strlen(tmpBuffer), &numBytesWritten);
+  nsresult rv4 = file->Write(tmpBuffer, strlen(tmpBuffer), &numBytesWritten);
+  if (NS_FAILED(rv4) || strlen(tmpBuffer) != numBytesWritten) {
+    MOZ_ASSERT(
+        NS_SUCCEEDED(rv4) && strlen(tmpBuffer) == numBytesWritten,
+        "file->Write(tmpBuffer, strlen(tmpBuffer), &numBytesWritten) failed.");
+    // XXX Better error recovery
+  }
   PR_Free(tmpBuffer);
   return HT_ENUMERATE_NEXT;
 }
 
 static int net_pop3_delete_old_msgs_mapper(PLHashEntry *he, int msgindex,
                                            void *arg) {
   PRTime cutOffDate = (PRTime)arg;
   Pop3UidlEntry *uidlEntry = (Pop3UidlEntry *)he->value;
@@ -288,17 +312,33 @@ static void net_pop3_write_state(Pop3Uid
       getter_AddRefs(fileOutputStream), popState, -1, 00600);
   if (NS_FAILED(rv)) return;
 
   const char tmpBuffer[] =
       "# POP3 State File" MSG_LINEBREAK
       "# This is a generated file!  Do not edit." MSG_LINEBREAK MSG_LINEBREAK;
 
   uint32_t numBytesWritten;
-  fileOutputStream->Write(tmpBuffer, strlen(tmpBuffer), &numBytesWritten);
+  nsresult rv4 =
+      fileOutputStream->Write(tmpBuffer, strlen(tmpBuffer), &numBytesWritten);
+  // XXX TODO: this is supposed to be a safe output stream.
+  // Feel free to remove the error check if you really feel safe.
+  // Given the histor of I/O error mishandling, I do NOT AT ALL.
+  // I wonder why not all the output in relation to local storage
+  // is done using safe output stream?
+  if (NS_FAILED(rv4) || strlen(tmpBuffer) != numBytesWritten) {
+    MOZ_ASSERT(NS_SUCCEEDED(rv4) && strlen(tmpBuffer) == numBytesWritten,
+               "fileOutputStream->Write(tmpBuffer, strlen(tmpBuffer), "
+               "&numBytesWritten) failed.");
+  }
+
+  // XXX
+  // Note that fileOutputStream is supposed to be a safe output
+  // stream, but given the history of I/O error mishandling, I feel
+  // tempted to check write error below as well as above..
 
   for (; host && (len >= 0); host = host->next) {
     if (!hash_empty(host->hash)) {
       fileOutputStream->Write("*", 1, &numBytesWritten);
       fileOutputStream->Write(host->host, strlen(host->host), &numBytesWritten);
       fileOutputStream->Write(" ", 1, &numBytesWritten);
       fileOutputStream->Write(host->user, strlen(host->user), &numBytesWritten);
       fileOutputStream->Write(MSG_LINEBREAK, MSG_LINEBREAK_LEN,
--- a/mailnews/local/src/nsPop3Service.cpp
+++ b/mailnews/local/src/nsPop3Service.cpp
@@ -97,16 +97,19 @@ nsresult nsPop3Service::GetMail(bool dow
 
   rv = server->GetUsername(popUser);
   NS_ENSURE_SUCCESS(rv, rv);
   if (popUser.IsEmpty()) return NS_MSG_SERVER_USERNAME_MISSING;
 
   nsCString escapedUsername;
   MsgEscapeString(popUser, nsINetUtil::ESCAPE_XALPHAS, escapedUsername);
 
+  // XXX TODO Since |rv| has already been checked with
+  // NS_ENSURE_SUCCESS above, We can probably remove the
+  // NS_SUCCEEDED(rv) check on the next line.
   if (NS_SUCCEEDED(rv) && aPopServer) {
     // now construct a pop3 url...
     // we need to escape the username because it may contain
     // characters like / % or @
     char *urlSpec =
         (downloadNewMail)
             ? PR_smprintf("pop3://%s@%s:%d", escapedUsername.get(),
                           popHost.get(), popPort)
@@ -115,16 +118,22 @@ nsresult nsPop3Service::GetMail(bool dow
     rv = BuildPop3Url(urlSpec, aInbox, aPopServer, aUrlListener,
                       getter_AddRefs(url), aMsgWindow);
     PR_smprintf_free(urlSpec);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   NS_ENSURE_TRUE(url, rv);
 
+  // XXX TODO I *think* since |rv| has been checked with NS_ENSURE_SUCCESS
+  // above, We can remove the NS_SUCCEEDED(rv) check on the next line. Or
+  // rather, I think the original intention was to check that url is not nil?
+  // But it is now checked by NS_ENSURE_TRUE(url, rv) above.
+  // So I think NS_SUCCEEDED(rv) check is unnecessary.
+  // Someone more familiar with the code here should check.
   if (NS_SUCCEEDED(rv)) rv = RunPopUrl(server, url);
 
   if (aURL)  // we already have a ref count on pop3url...
     url.forget(aURL);
 
   return rv;
 }
 
@@ -163,16 +172,17 @@ NS_IMETHODIMP nsPop3Service::VerifyLogon
   NS_ENSURE_TRUE(urlSpec, NS_ERROR_OUT_OF_MEMORY);
 
   nsCOMPtr<nsIURI> url;
   rv = BuildPop3Url(urlSpec, nullptr, popServer, aUrlListener,
                     getter_AddRefs(url), aMsgWindow);
   PR_smprintf_free(urlSpec);
 
   if (NS_SUCCEEDED(rv) && url) {
+    // RunPopUrl is defined later in this file.
     rv = RunPopUrl(aServer, url);
     if (NS_SUCCEEDED(rv) && aURL) url.forget(aURL);
   }
 
   return rv;
 }
 
 nsresult nsPop3Service::BuildPop3Url(const char *urlSpec, nsIMsgFolder *inbox,
@@ -234,45 +244,67 @@ nsresult nsPop3Service::RunPopUrl(nsIMsg
       // escape it.
       protocol->SetUsername(userName.get());
       rv = protocol->LoadUrl(aUrlToRun);
       if (NS_FAILED(rv)) aServer->SetServerBusy(false);
     }
   } else {
     nsCOMPtr<nsIMsgMailNewsUrl> url = do_QueryInterface(aUrlToRun);
     if (url) AlertServerBusy(url);
+    // XXX TODO Can't we be more imaginative with the error code
+    // rather than using the bland NS_ERROR_FAILURE?
     rv = NS_ERROR_FAILURE;
+    NS_WARNING("(debug) server is busy in nsPop3Service::RunPopUrl()\n");
+
+    // Since NS_WARNING() is a no-op in a publish build.
+    // I want an error indication by output somehow.
+#ifdef DEBUG
+    fprintf(stderr, "(debug) server is busy in nsPop3Service::RunPopUrl()\n");
+#endif
   }
   return rv;
 }
 
 NS_IMETHODIMP nsPop3Service::GetScheme(nsACString &aScheme) {
   aScheme.AssignLiteral("pop3");
   return NS_OK;
 }
 
 NS_IMETHODIMP nsPop3Service::GetDefaultPort(int32_t *aDefaultPort) {
   NS_ENSURE_ARG_POINTER(aDefaultPort);
   *aDefaultPort = nsIPop3URL::DEFAULT_POP3_PORT;
   return NS_OK;
 }
 
+//
+// CI Q: what is AllowPort supposed to do?
+//
 NS_IMETHODIMP nsPop3Service::AllowPort(int32_t port, const char *scheme,
                                        bool *_retval) {
   *_retval = true;  // allow pop on any port
   return NS_OK;
 }
 
 NS_IMETHODIMP nsPop3Service::GetDefaultDoBiff(bool *aDoBiff) {
   NS_ENSURE_ARG_POINTER(aDoBiff);
   // by default, do biff for POP3 servers
   *aDoBiff = true;
   return NS_OK;
 }
 
+//
+// XXX TODO we better document the used flag values here for handy reference.
+//
+// URI_NO_RELATIVE
+// URI_DANGEROUS_TO_LOAD
+// ALLOWS_PROXY
+// URI_FORBIDS_COOKIE_ACCESS
+//
+// XXX URI_FORBIDS_COOKIE_ACCESS does not seem to be relevant for POP3.
+//
 NS_IMETHODIMP nsPop3Service::GetProtocolFlags(uint32_t *result) {
   NS_ENSURE_ARG_POINTER(result);
   *result = URI_NORELATIVE | URI_DANGEROUS_TO_LOAD | ALLOWS_PROXY |
             URI_FORBIDS_COOKIE_ACCESS;
   return NS_OK;
 }
 
 nsresult nsPop3Service::NewURI(const nsACString &aSpec,
--- a/mailnews/local/src/nsPop3Sink.cpp
+++ b/mailnews/local/src/nsPop3Sink.cpp
@@ -31,16 +31,17 @@
 #include "nsEmbedCID.h"
 #include "nsMsgUtils.h"
 #include "nsMsgBaseCID.h"
 #include "nsServiceManagerUtils.h"
 #include "nsIPop3Service.h"
 #include "nsMsgLocalCID.h"
 #include "mozilla/Services.h"
 #include "mozilla/Logging.h"
+#include "nsNetUtil.h"
 
 /* for logging to Error Console */
 #include "nsIScriptError.h"
 
 extern mozilla::LazyLogModule POP3LOGMODULE;  // defined in nsPop3Protocol.cpp
 #define POP3LOG(str) "sink: [this=%p] " str, this
 
 NS_IMPL_ISUPPORTS(nsPop3Sink, nsIPop3Sink)
@@ -139,18 +140,29 @@ nsresult nsPop3Sink::FindPartialMessages
           partialMsg->m_uidl = folderScanState.m_uidl;
           partialMsg->m_msgDBHdr = msgDBHdr;
           m_partialMsgsArray.AppendElement(partialMsg);
         }
       }
     }
     messages->HasMoreElements(&hasMore);
   }
-  if (isOpen && folderScanState.m_inputStream)
-    folderScanState.m_inputStream->Close();
+  if (isOpen && folderScanState.m_inputStream) {
+    nsresult rv3 = folderScanState.m_inputStream->Close();
+#ifdef DEBUG
+    fprintf(
+        stderr,
+        "{debug} AbortMailDelivery: m_outFileStream(%p)->Close() = 0x%" PRIx32
+        "\n",
+        (void *)m_outFileStream, static_cast<uint32_t>(rv3));
+#endif
+    if (NS_FAILED(rv3))
+      NS_WARNING("folderScanState.m_inputStream->Close() failed.");
+    folderScanState.m_inputStream = nullptr;  // code uniformity
+  }
   return rv;
 }
 
 // For all the partial messages saved by FindPartialMessages,
 // ask the protocol handler if they still exist on the server.
 // Any messages that don't exist any more are deleted from the
 // msgDB.
 void nsPop3Sink::CheckPartialMessages(nsIPop3Protocol *protocol) {
@@ -217,34 +229,53 @@ nsresult nsPop3Sink::BeginMailDelivery(b
   nsCOMPtr<nsIPop3Service> pop3Service(
       do_GetService(NS_POP3SERVICE_CONTRACTID1, &rv));
   NS_ENSURE_SUCCESS(rv, rv);
   pop3Service->NotifyDownloadStarted(m_folder);
   if (aBool) *aBool = true;
   return NS_OK;
 }
 
+//
+// XXX  TODO add better and proper I/O error processing, especially
+// with regard to output error (file system filling up, etc.)
+//
 nsresult nsPop3Sink::EndMailDelivery(nsIPop3Protocol *protocol) {
   CheckPartialMessages(protocol);
 
   if (m_newMailParser) {
-    if (m_outFileStream) m_outFileStream->Flush();  // try this.
+    if (m_outFileStream) {
+      nsresult rv3 = m_outFileStream->Flush();  // try this.
+      if (NS_FAILED(rv3)) NS_WARNING("m_outFileStream->Flush() failed;");
+    }
     m_newMailParser->OnStopRequest(nullptr, NS_OK);
     m_newMailParser->EndMsgDownload();
   }
   if (m_outFileStream) {
-    m_outFileStream->Close();
+    nsresult rv3 = m_outFileStream->Close();
+#ifdef DEBUG
+    fprintf(stderr, "m_outFileStream(%p)->Close() = 0x%" PRIx32 "\n",
+            (void *)m_outFileStream, static_cast<uint32_t>(rv3));
+#endif
+    if (NS_FAILED(rv3)) NS_WARNING("m_outFileStream->Close() failed;");
     m_outFileStream = nullptr;
   }
 
-  if (m_downloadingToTempFile) m_tmpDownloadFile->Remove(false);
+  if (m_downloadingToTempFile) {
+    nsresult rv1 = m_tmpDownloadFile->Remove(false);
+    if (NS_FAILED(rv1)) {
+      NS_WARNING("m_tmpDownloadFile->Remove(false) failed.");
+    }
+  }
 
   // tell the parser to mark the db valid *after* closing the mailbox.
   if (m_newMailParser) m_newMailParser->UpdateDBFolderInfo();
 
+  // XXX TODO Investigate if this is the correct place to release the lock.
+  // Shouldn't we release it after filter plugins are run below?
   MOZ_LOG(POP3LOGMODULE, mozilla::LogLevel::Debug,
           (POP3LOG("Calling ReleaseFolderLock from EndMailDelivery")));
   nsresult rv = ReleaseFolderLock();
   NS_ASSERTION(NS_SUCCEEDED(rv), "folder lock not released successfully");
 
   bool filtersRun;
   m_folder->CallFilterPlugins(nullptr,
                               &filtersRun);  // ??? do we need msgWindow?
@@ -259,16 +290,19 @@ nsresult nsPop3Sink::EndMailDelivery(nsI
       m_numNewMessages);  // we'll adjust this for spam later
   if (!filtersRun && m_numNewMessages > 0) {
     nsCOMPtr<nsIMsgIncomingServer> server;
     m_folder->GetServer(getter_AddRefs(server));
     if (server) {
       server->SetPerformingBiff(true);
       m_folder->SetBiffState(m_biffState);
       server->SetPerformingBiff(false);
+    } else {
+      // XXX TODO What do we do if server was null? Shouldn't we warn with
+      // a debug print?
     }
   }
   // note that size on disk has possibly changed.
   nsCOMPtr<nsIMsgLocalMailFolder> localFolder = do_QueryInterface(m_folder);
   if (localFolder) (void)localFolder->RefreshSizeOnDisk();
   nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_popServer);
   if (server) {
     nsCOMPtr<nsIMsgFilterList> filterList;
@@ -341,22 +375,40 @@ nsresult nsPop3Sink::ReleaseFolderLock()
 }
 
 nsresult nsPop3Sink::AbortMailDelivery(nsIPop3Protocol *protocol) {
   CheckPartialMessages(protocol);
 
   // ### PS TODO - discard any new message?
 
   if (m_outFileStream) {
-    m_outFileStream->Close();
+    nsresult rv3 = m_outFileStream->Close();
+#ifdef DEBUG
+    fprintf(
+        stderr,
+        "{debug} AbortMailDelivery: m_outFileStream(%p)->Close() = 0x%" PRIx32
+        "\n",
+        (void *)m_outFileStream, static_cast<uint32_t>(rv3));
+#endif
+    if (NS_FAILED(rv3)) NS_WARNING("m_outFileStream->Close() failed;");
     m_outFileStream = nullptr;
   }
 
-  if (m_downloadingToTempFile && m_tmpDownloadFile)
-    m_tmpDownloadFile->Remove(false);
+  // In the following,  We print a warning if Remove failed...
+  // The following message was indeed printed during a test.
+  // Funny, though, in Linux strace capture, unlink("/tmp/newmsg") succeeded.
+  // Here, we are going to remove a temporary download file.
+  // Under windows, remove fails if the file is still open. That is why.
+  // The file close timing was changed and close is called before
+  // control reaches here. Thus we now avoid such an error.
+  if (m_downloadingToTempFile && m_tmpDownloadFile) {
+    nsresult rv3 = m_tmpDownloadFile->Remove(false);
+    if (NS_FAILED(rv3))
+      NS_WARNING("m_outFileStream->Remove() for temporary file failed;");
+  }
 
   /* tell the parser to mark the db valid *after* closing the mailbox.
   we have truncated the inbox, so berkeley mailbox and msf file are in sync*/
   if (m_newMailParser) m_newMailParser->UpdateDBFolderInfo();
   MOZ_LOG(POP3LOGMODULE, mozilla::LogLevel::Debug,
           (POP3LOG("Calling ReleaseFolderLock from AbortMailDelivery")));
 
   nsresult rv = ReleaseFolderLock();
@@ -401,16 +453,18 @@ nsPop3Sink::IncorporateBegin(const char 
   }
 
   nsCOMPtr<nsIMsgDBHdr> newHdr;
 
   nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_popServer);
   if (!server) return NS_ERROR_UNEXPECTED;
 
   if (m_downloadingToTempFile) {
+    NS_WARNING("(debug): IncorporateBegin: m_downloadingToTempFile path");
+
     // need to create an nsIOFileStream from a temp file...
     nsCOMPtr<nsIFile> tmpDownloadFile;
     rv = GetSpecialDirectoryWithFileName(NS_OS_TEMP_DIR, "newmsg",
                                          getter_AddRefs(tmpDownloadFile));
 
     NS_ASSERTION(NS_SUCCEEDED(rv),
                  "writing tmp pop3 download file: failed to append filename");
     if (NS_FAILED(rv)) return rv;
@@ -420,36 +474,146 @@ nsPop3Sink::IncorporateBegin(const char 
       rv = tmpDownloadFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 00600);
       NS_ENSURE_SUCCESS(rv, rv);
 
       m_tmpDownloadFile = tmpDownloadFile;
     }
     rv = MsgGetFileStream(m_tmpDownloadFile, getter_AddRefs(m_outFileStream));
     NS_ENSURE_SUCCESS(rv, rv);
   } else {
+    NS_WARNING("(debug): IncorporateBegin: !m_downloadingToTempFile path");
+
     rv = server->GetMsgStore(getter_AddRefs(m_msgStore));
-    bool reusable;
+    // bool reusable;
     NS_ENSURE_SUCCESS(rv, rv);
     m_msgStore->GetNewMsgOutputStream(m_folder, getter_AddRefs(newHdr),
-                                      &reusable,
                                       getter_AddRefs(m_outFileStream));
+#ifdef DEBUG
+    if (NS_FAILED(rv)) {
+      fflush(stdout);
+      fprintf(stderr,
+              "(debug) IncorporateBegin: "
+              "m_msgStore->GetNewMsgOutputStream(....) failed. "
+              "rv = 0x%" PRIx32 "\n",
+              static_cast<uint32_t>(rv));
+    }
+#endif
+    // Note: The failure of the above call ought to be noticable by
+    // |!m_outFileStream| check below. But we should be explicit about the
+    // error. So NS_ENSURE_SUCCESS() is added now.
+    NS_ENSURE_SUCCESS(rv, rv);
   }
   // 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
+  // XXX TODO/FIXME
+  // the error code ought to be something along the line of
+  // "Could not create a writable stream/file", etc.
+  // NS_ERROR_OUT_OF_MEMORY seems out of context.
   if (!m_outFileStream) return NS_ERROR_OUT_OF_MEMORY;
 
+  // Problem: when |m_outFileStream| has become a buffered output stream
+  // the following code failed.
+  // nsCOMPtr <nsIInputStream> inboxInputStream =
+  // do_QueryInterface(m_outFileStream, &rv); So we execute this BEFORE
+  // |m_outFileStream| becomes buffered, and save the resulting
+  // |inboxInputStream| in  |m_inboxInputStream|. See Bug 1159777 for details.
+
+  nsCOMPtr<nsIInputStream> inboxInputStream =
+      do_QueryInterface(m_outFileStream, &rv);
+  if (NS_FAILED(rv)) {
+#ifdef DEBUG
+    fprintf(stderr,
+            "(debug) nsCOMPtr <nsIInputStream> inboxInputStream = "
+            "do_QueryInterface(m_outFileStream, &rv);"
+            "failed with rv=0x%" PRIx32 "\n",
+            static_cast<uint32_t>(rv));
+#endif
+    m_inboxInputStream = nullptr;
+  } else {
+#ifdef DEBUG
+    fprintf(stderr,
+            "(debug) nsCOMPtr <nsIInputStream> inboxInputStream = "
+            "do_QueryInterface(m_outFileStream, &rv);"
+            "succeeded\n");
+#endif
+    m_inboxInputStream = inboxInputStream;
+  }
+
+  // XXX Does this buffering code conflict with aceman's patch?
+  // Well, not quite, but the buffering interferes with
+  // nsCOMPtr <nsIInputStream> inboxInputStream =
+  // do_QueryInterface(m_outFileStream, &rv); A buffered output stream can't be
+  // used for producing non-nullptr inboxInputStream by do_QueryInterface(). The
+  // next line is executed when downloading to temporary file is done to switch
+  // from writing the temporary file to reading from that file.
+  // nsCOMPtr <nsIInputStream> inboxInputStream =
+  // do_QueryInterface(m_outFileStream); But this fails when the m_outFileStream
+  // is a buffered stream. So I moved the generation of inboxInputStream before
+  // buffering is enabled for
+  // m_outFileStream and save the value for later use in |IncorporateComplete()|
+  // See Bug 1159777 for details.
+
+  // WATCH OUT: Enabling buffering caused the failure to incorporate
+  // messages into Inbox in Jan 2015 especially when artificial
+  // network errors were introduced, etc., but but after cleaning up
+  // error handling in message download code, I have not seen
+  // corruption since (April, 2015).
+
+#ifdef DEBUG
+  NS_WARNING(
+      "(debug) We are enabling buffering for m_outFileStream in "
+      "nsPop3Sink::IncorporateBegin in nsPop3Sink.cpp.");
+#endif
+  // Enable Buffering.
+  nsresult rv1;
+  rv1 = m_outFileStream->Flush();  // Should be NOOP immediately after creation
+                                   // but just in case.
+#ifdef DEBUG
+  fflush(stdout);
+  fprintf(stderr,
+          "(debug) %d: m_outFileStream->Flush() returned "
+          "0x%" PRIx32 "\n",
+          __LINE__, static_cast<uint32_t>(rv1));
+#endif
+  if (NS_FAILED(rv1)) {
+    NS_WARNING("m_outFileStream->Flush() returned error.");
+  }
+
+  // Finally buffering! But we have to remove offending |Seek|.
+  // CI: Is forget() the correct call? It seems so.
+
+  //
+  // clang and GCC has a different order of argument evaluation.
+  // We have to save m_outFileSream because
+  // geter_AddRefs(m_outFileStream) may trash it depending on the
+  // order of evaluation of passed arguments!
+  //
+  nsCOMPtr<nsIOutputStream> tmp_p = m_outFileStream;
+  rv1 = NS_NewBufferedOutputStream(getter_AddRefs(m_outFileStream),
+                                   tmp_p.forget(), 64 * 1024);
+  NS_ASSERTION(NS_SUCCEEDED(rv1),
+               "buffering by NS_NewBufferedOutputStream failed.");
+
   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);
+
+  // We are disabling filters |if (m_uidlDownload)| to deal with the
+  // option to fetch only headers or the first part of a
+  // message. There is then UI to download the rest of the message,
+  // but the message would already have been filtered, so there's no
+  // need to filter it again.
+
   if (m_uidlDownload) m_newMailParser->DisableFilters();
 
+  // GetServerFolder is defined in this file later.
   nsCOMPtr<nsIMsgFolder> serverFolder;
   rv = GetServerFolder(getter_AddRefs(serverFolder));
   if (NS_FAILED(rv)) return rv;
 
   rv = m_newMailParser->Init(serverFolder, m_folder, m_window, newHdr,
                              m_outFileStream);
   // If we failed to initialize the parser, then just don't use it!!!
   // We can still continue without one.
@@ -462,18 +626,38 @@ nsPop3Sink::IncorporateBegin(const char 
   if (closure) *closure = (void *)this;
 
 #ifdef DEBUG
   // Debugging, see bug 1116055.
   int64_t first_pre_seek_pos;
   nsresult rv3 = seekableOutStream->Tell(&first_pre_seek_pos);
 #endif
 
+  // Bug 1116055: We need to seek to the end of the m_outputstream
+  // here ONCE per message.  This is because function(s) to enable
+  // buffering rewinds the file seek position to the beginning.
+  // Thus we need to call |Seek()| once per message, but we don't
+  // need to call it per output line in |WriteLineToMailBox|.  The
+  // call in |Seek()| in |WriteLineToMailBox| negated the effect of
+  // buffering and resulted in loss of output peformance.
+
   // XXX Handle error such as network error for remote file system.
-  seekableOutStream->Seek(nsISeekableStream::NS_SEEK_END, 0);
+  rv = seekableOutStream->Seek(nsISeekableStream::NS_SEEK_END, 0);
+  if (NS_FAILED(rv)) {
+    // XXX Better error recovery.
+    // What do we want to do after seek error?
+    // Probably abort downloading subsequent messages.
+#ifdef DEBUG
+    fflush(stdout);
+    fprintf(stderr,
+            "(debug) Seek in nsPop3Sink.cpp failed: "
+            "rv=0x%" PRIx32 "\n",
+            static_cast<uint32_t>(rv));
+#endif
+  }
 
 #ifdef DEBUG
   // Debugging, see bug 1116055.
   int64_t first_post_seek_pos;
   nsresult rv4 = seekableOutStream->Tell(&first_post_seek_pos);
   if (NS_SUCCEEDED(rv3) && NS_SUCCEEDED(rv4)) {
     if (first_pre_seek_pos != first_post_seek_pos) {
       nsString folderName;
@@ -512,29 +696,33 @@ nsPop3Sink::IncorporateBegin(const char 
   if (uidlString) {
     outputString.AssignLiteral("X-UIDL: ");
     outputString.Append(uidlString);
     outputString.AppendLiteral(MSG_LINEBREAK);
     rv = WriteLineToMailbox(outputString);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
+  // Explanation: actually 8000 part will be overwritten to
+  // reflect the value of |flags|.
   // WriteLineToMailbox("X-Mozilla-Status: 8000" MSG_LINEBREAK);
   char *statusLine = PR_smprintf(X_MOZILLA_STATUS_FORMAT MSG_LINEBREAK, flags);
   outputString.Assign(statusLine);
   rv = WriteLineToMailbox(outputString);
   PR_smprintf_free(statusLine);
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = WriteLineToMailbox(
       NS_LITERAL_CSTRING("X-Mozilla-Status2: 00000000" MSG_LINEBREAK));
   NS_ENSURE_SUCCESS(rv, rv);
 
   // leave space for 60 bytes worth of keys/tags
   rv = WriteLineToMailbox(NS_LITERAL_CSTRING(X_MOZILLA_KEYWORDS));
+  NS_ENSURE_SUCCESS(rv, rv);
+
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsPop3Sink::SetPopServer(nsIPop3IncomingServer *server) {
   m_popServer = server;
   return NS_OK;
 }
@@ -566,16 +754,18 @@ nsresult nsPop3Sink::GetServerFolder(nsI
     nsCOMPtr<nsIMsgIncomingServer> incomingServer =
         do_QueryInterface(m_popServer);
     if (incomingServer) return incomingServer->GetRootFolder(aFolder);
   }
   *aFolder = nullptr;
   return NS_ERROR_NULL_POINTER;
 }
 
+// set # of messages to download for progress notifications
+// (downloading 1 of N).
 NS_IMETHODIMP nsPop3Sink::SetMsgsToDownload(uint32_t aNumMessages) {
   m_numNewMessages = aNumMessages;
   return NS_OK;
 }
 
 char *nsPop3Sink::GetDummyEnvelope(void) {
   static char result[75];
   char *ct;
@@ -589,16 +779,23 @@ char *nsPop3Sink::GetDummyEnvelope(void)
   /* This value must be in ctime() format, with English abbreviations.
    strftime("... %c ...") is no good, because it is localized. */
   PL_strcpy(result, "From - ");
   PL_strcpy(result + 7, ct);
   PL_strcpy(result + 7 + 24, MSG_LINEBREAK);
   return result;
 }
 
+//
+// Append a block into internal buffer (m_outputBuffer)
+// with "From" escaping, and then write it as a single line. using
+// WriteLineToMailBox
+// This function is called by nsPop3Protocol::HandleLine(char *line, uint32_t
+// line_length) in nsPop3Protocol.cpp
+//
 nsresult nsPop3Sink::IncorporateWrite(const char *block, int32_t length) {
   m_outputBuffer.Truncate();
   if (!strncmp(block, "From ", 5)) m_outputBuffer.Assign('>');
 
   m_outputBuffer.Append(block);
 
   return WriteLineToMailbox(m_outputBuffer);
 }
@@ -622,17 +819,26 @@ nsresult nsPop3Sink::WriteLineToMailbox(
         do_QueryInterface(m_outFileStream);
 
     int64_t before_seek_pos;
     nsresult rv2 = seekableOutStream->Tell(&before_seek_pos);
     MOZ_ASSERT(NS_SUCCEEDED(rv2),
                "seekableOutStream->Tell(&before_seek_pos) failed");
 
     // XXX Handle error such as network error for remote file system.
-    seekableOutStream->Seek(nsISeekableStream::NS_SEEK_END, 0);
+    nsresult rv = seekableOutStream->Seek(nsISeekableStream::NS_SEEK_END, 0);
+    if (NS_FAILED(rv)) {
+#  ifdef DEBUG
+      fflush(stdout);
+      fprintf(stderr,
+              "(seekdebug) Seek() failed in nsPop3Sink.cpp: "
+              "rv = 0x%" PRIx32 "\n",
+              static_cast<uint32_t>(rv));
+#  endif
+    }
 
     int64_t after_seek_pos;
     nsresult rv3 = seekableOutStream->Tell(&after_seek_pos);
     MOZ_ASSERT(NS_SUCCEEDED(rv3),
                "seekableOutStream->Tell(&after_seek_pos) failed");
 
     if (NS_SUCCEEDED(rv2) && NS_SUCCEEDED(rv3)) {
       if (before_seek_pos != after_seek_pos) {
@@ -669,23 +875,60 @@ nsresult nsPop3Sink::WriteLineToMailbox(
             "(seekdebug) before_seek_pos=0x%016llx, after_seek_pos=0x%016llx\n",
             (long long unsigned int)before_seek_pos,
             (long long unsigned int)after_seek_pos);
 #  endif
       }
     }
 #endif
 
+    // We do not need to call |Seek()| here, but call it once per
+    // message in |IncorporateBegin()|.
+
     uint32_t bytesWritten;
-    m_outFileStream->Write(buffer.BeginReading(), bufferLen, &bytesWritten);
-    NS_ENSURE_TRUE(bytesWritten == bufferLen, NS_ERROR_FAILURE);
+    nsresult rv4 =
+        m_outFileStream->Write(buffer.BeginReading(), bufferLen, &bytesWritten);
+#ifdef DEBUG
+    // During test, this Write failed.
+    if (NS_FAILED(rv4)) {
+      fprintf(stderr,
+              "(debug) m_outFileStream->Write(buffer.BeginReading(), "
+              "bufferLen, &bytesWritten); failed. "
+              "rv4 = 0x%" PRIx32 "\n",
+              static_cast<uint32_t>(rv4));
+    } else if (bytesWritten != bufferLen) {
+      fprintf(
+          stderr,
+          "(debug) written length mismatch: bytesWritten=%d != bufferLen=%d\n",
+          bytesWritten, bufferLen);
+    }
+#endif
+    // XXX TODO Better network error handling, etc.
+    NS_ENSURE_TRUE(NS_SUCCEEDED(rv4) && bytesWritten == bufferLen,
+                   NS_ERROR_FAILURE);
   }
   return NS_OK;
 }
 
+// Handle the case when download to temporary file failed.
+// It shows a dialog: never realized that since I don't use temporary file
+// download. We must set undefined-by-default mailnews.downloadToTempFile
+// profile to true to test this function.
+//
+
+// CI: I really don't like mozilla code's tendency to fail to
+// show/display something meaningful to the user when
+// the stringbundle processing fails.
+// Why can't we show something as simple as "temporary download
+// fails"?
+// Yes, it may be as cryptic as "SYSTEM ERROR: 12345" to an ordinary
+// user in a non-English-speaking country, but at least, a developer
+// can get a clue when something fails.
+// XXX FIXME: Today's behavior of not showing anything at all is unacceptable
+// IMHO.
 nsresult nsPop3Sink::HandleTempDownloadFailed(nsIMsgWindow *msgWindow) {
   nsresult rv;
   nsCOMPtr<nsIStringBundleService> bundleService =
       mozilla::services::GetStringBundleService();
   NS_ENSURE_TRUE(bundleService, NS_ERROR_UNEXPECTED);
   nsCOMPtr<nsIStringBundle> bundle;
   rv = bundleService->CreateBundle(
       "chrome://messenger/locale/localMsgs.properties", getter_AddRefs(bundle));
@@ -715,33 +958,62 @@ nsresult nsPop3Sink::HandleTempDownloadF
 
     return (dlgResult == 0) ? NS_OK : NS_MSG_ERROR_COPYING_FROM_TMP_DOWNLOAD;
   }
   return rv;
 }
 
 NS_IMETHODIMP
 nsPop3Sink::IncorporateComplete(nsIMsgWindow *aMsgWindow, int32_t aSize) {
+  // Given that m_newMailPaser can't be null due to the assertion in
+  // the middle of this function
+  // we might as well check this here, and simplify a few things.
+  NS_ASSERTION(m_newMailParser, "could not get m_newMailParser");
+
+  // We are extracting a msgKey from m_newMsgHdr and create a local message URI.
+  // A message URI is what the front end uses to actually view a
+  // message. This is because it has to go though all the gubbins used
+  // for displaying web pages.
+
   if (m_buildMessageUri && !m_baseMessageUri.IsEmpty() && m_newMailParser &&
       m_newMailParser->m_newMsgHdr) {
     nsMsgKey msgKey;
     m_newMailParser->m_newMsgHdr->GetMessageKey(&msgKey);
     m_messageUri.Truncate();
     nsBuildLocalMessageURI(m_baseMessageUri.get(), msgKey, m_messageUri);
   }
 
   nsresult rv = WriteLineToMailbox(NS_LITERAL_CSTRING(MSG_LINEBREAK));
   NS_ENSURE_SUCCESS(rv, rv);
   bool leaveOnServer = false;
   m_popServer->GetLeaveMessagesOnServer(&leaveOnServer);
   // We need to flush the output stream, in case mail filters move
   // the new message, which relies on all the data being flushed.
   rv =
       m_outFileStream->Flush();  // Make sure the message is written to the disk
+#ifdef DEBUG
+  // During testing using Linux CIFS server as mail store from a TB
+  // running on another Linux, we get error on this flush.
+  if (NS_FAILED(rv)) {
+    fprintf(stderr,
+            "(debug) m_outFileStream->Flush() returned "
+            "0x%" PRIx32 "\n",
+            static_cast<uint32_t>(rv));
+  }
+#endif
+  // For now, we have no good error recovery strategy, and so just
+  // return.
   NS_ENSURE_SUCCESS(rv, rv);
+
+  // XXX TODO Devise a better error recovery strategy here.
+  // For example, when flush failed, would we want to return by
+  // NS_ENSURE_SUCCESS above without closing the m_outFileStream here?
+  // Or is it closed somewhere?
+  // We DO need a good error recovery strategy here.
+
   NS_ASSERTION(m_newMailParser, "could not get m_newMailParser");
   if (m_newMailParser) {
     // PublishMsgHdr clears m_newMsgHdr, so we need a comptr to
     // hold onto it.
     nsCOMPtr<nsIMsgDBHdr> hdr = m_newMailParser->m_newMsgHdr;
     NS_ASSERTION(hdr, "m_newMailParser->m_newMsgHdr wasn't set");
     if (!hdr) return NS_ERROR_FAILURE;
 
@@ -760,100 +1032,280 @@ nsPop3Sink::IncorporateComplete(nsIMsgWi
       nsCOMPtr<nsIMsgDBHdr> oldMsgHdr;
       rv =
           GetMsgDBHdrFromURI(m_origMessageUri.get(), getter_AddRefs(oldMsgHdr));
       if (NS_SUCCEEDED(rv) && oldMsgHdr)
         localFolder->UpdateNewMsgHdr(oldMsgHdr, hdr);
     }
 
     if (m_downloadingToTempFile) {
+      NS_WARNING(
+          "(debug) m_downloadingToTempFile path in IncorporateComplete ");
       // close file to give virus checkers a chance to do their thing...
-      m_outFileStream->Flush();
-      m_outFileStream->Close();
-      m_newMailParser->FinishHeader();
+
+      nsresult rv2 = m_outFileStream->Close();
+      if (NS_FAILED(rv2)) NS_WARNING("m_outFileStream->Close() failed;");
+      // We can't null this yet: m_outFileStream = nullptr;
+
+      // Check I/O error.
+      nsresult rv3 = m_newMailParser->FinishHeader();
+      if (NS_FAILED(rv3)) {
+#ifdef DEBUG
+        fprintf(stderr,
+                "(debug) %d: m_newMailParser->FinishHeader() returned 0x%08x\n",
+                __LINE__, (unsigned)rv3);
+#endif
+        MOZ_ASSERT(NS_SUCCEEDED(rv3),
+                   "m_newMailparser->FinishHeader() failed.");
+      }
+
+      // Should we not set
+      //    m_outFileStream = nullptr
+      // irrespective of whether Close() failed or not?
+      // No, not here yet because we may use this variable below.
+
+      // XXX Add a proper error check. We now return with an error dialog.
+      if (NS_FAILED(rv2) || NS_FAILED(rv3)) {
+        NS_WARNING("(debug): show error dialog.");
+#ifdef DEBUG
+        fflush(stdout);
+        fprintf(stderr, "(debug) Error handling... case 1\n");
+#endif
+        return HandleTempDownloadFailed(aMsgWindow);
+      }
+
       // need to re-open the inbox file stream.
+
+      // Note that a virus checker might have removed the temporary file!
+
       bool exists;
       m_tmpDownloadFile->Exists(&exists);
-      if (!exists) return HandleTempDownloadFailed(aMsgWindow);
+      if (!exists) {
+#ifdef DEBUG
+        fflush(stdout);
+        fprintf(stderr, "(debug) Error handling... case 2\n");
+#endif
+        return HandleTempDownloadFailed(aMsgWindow);
+      }
+
+      // Note the mixture of (inboxInputStream|m_outFilestream) below.
+      // This is because we are going to READ the temporary
+      // file into which we WROTE the downloaded message.
 
-      nsCOMPtr<nsIInputStream> inboxInputStream =
-          do_QueryInterface(m_outFileStream);
+      // Problem: when m_outFileStream has become a buffered output stream
+      // the following code failed.
+      // clang-format off
+      // nsCOMPtr <nsIInputStream> inboxInputStream = do_QueryInterface(m_outFileStream, &rv);
+      // So it seems that we need to either
+      // (i) disable buffering,
+      // (ii) execute the code BEFORE m_outFileStream becomes buffered, or
+      // (iii) open a named file explicitly.
+      // clang-format on
+      // I thought of trying (ii) but |m_outFileStrem| becomes buffered in
+      // ANOTHER function. Well, finally I decided to define a struct member,
+      // |m_inboxInputStream| for the whole purpose of passing this value. See
+      // Bug 1159777 for details.
+
+      // nsCOMPtr<nsIInputStream> inboxInputStream =
+      //  do_QueryInterface(m_outFileStream);
+
+      // Use the saved value.
+      nsCOMPtr<nsIInputStream> inboxInputStream = m_inboxInputStream;
       rv = MsgReopenFileStream(m_tmpDownloadFile, inboxInputStream);
+#ifdef DEBUG
+      if (NS_FAILED(rv)) {
+        fflush(stdout);
+        fprintf(stderr, "(debug) m_outFileStream=%p, inboxInputStream=%p\n",
+                (void *)m_outFileStream, (void *)inboxInputStream);
+        fprintf(stderr, "(debug) Error handling... case 3\n");
+      }
+#endif
       NS_ENSURE_SUCCESS(rv, HandleTempDownloadFailed(aMsgWindow));
-      if (m_outFileStream) {
+      m_outFileStream = nullptr;  // finally. avoid accessing closed stream
+
+      // Recall we are in if (m_downloadingToTempFile) branch.
+      // XXX In the old code, the following |if| started as
+      //
+      // if (m_outFileStream)
+      //
+      // I think it was a typo.
+      if (inboxInputStream) {
         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;
         rv = m_tmpDownloadFile->Clone(getter_AddRefs(tmpClone));
         NS_ENSURE_SUCCESS(rv, rv);
         tmpClone->GetFileSize(&tmpDownloadFileSize);
 
+        // We are inside a big if (m_newMailParser) so m_newMailParser
+        // is not null .
+
+        // So finally we are going to append the content of the
+        // temporary file into mail store (could be a single file or
+        // real append to an existing mbox file).
+
+        // It is hidden behind AppendMsgFromStream().
+
         if (msgSize > tmpDownloadFileSize)
           rv = NS_MSG_ERROR_WRITING_MAIL_FOLDER;
-        else
+        else {
+#ifdef DEBUG
+          NS_WARNING("(debug) AppendMsgFropmStream called.");
+#endif
           rv = m_newMailParser->AppendMsgFromStream(inboxInputStream, hdr,
                                                     msgSize, m_folder);
-        if (NS_FAILED(rv)) return HandleTempDownloadFailed(aMsgWindow);
+        }
+        if (NS_FAILED(rv)) {
+#ifdef DEBUG
+          fflush(stdout);
+          fprintf(stderr, "(debug) Error handling... case 4\n");
+#endif
+          return HandleTempDownloadFailed(aMsgWindow);
+        }
+        // Here inboxInputStream is NOT nullptr
+        nsresult rv3 = inboxInputStream->Close();  // close so we can truncate.
+        if (NS_FAILED(rv3)) {
+          NS_WARNING("inboxInputStream->Close() failed;");
+        }
+        inboxInputStream = nullptr;
 
-        m_outFileStream->Close();  // close so we can truncate.
-        m_tmpDownloadFile->SetFileSize(0);
-      } else {
+        nsresult rv4 = m_tmpDownloadFile->SetFileSize(0);
+        if (NS_FAILED(rv4)) {
+          NS_WARNING("m_tmpDownloadFile->SetFileSize(0) failed.");
+        }
+        if (NS_FAILED(rv3) || NS_FAILED(rv4)) {
+          // XXX need a good error recovery code but printing error is
+          // better than nothing.
+          NS_WARNING(
+              "(debug): Error status was not checked before!?"
+              "one of rv3, rv4 were not NS_OK");
+#ifdef DEBUG
+          fflush(stdout);
+          fprintf(stderr, "(debug) Error handling... case 5\n");
+#endif
+          return HandleTempDownloadFailed(aMsgWindow);
+        }
+      } else { // ! inboxInputStream
+        // need to give an error indication here.
+        NS_WARNING("inboxInputStream was nullptr!");
+#ifdef DEBUG
+          fflush(stdout);
+          fprintf(stderr, "(debug) Error handling... case 6\n");
+#endif
         return HandleTempDownloadFailed(aMsgWindow);
-        // need to give an error here.
       }
-    } else {
-      m_msgStore->FinishNewMessage(m_outFileStream, hdr);
+    } else { // !m_downloadingToTempFile
+      // XXX TODO I wonder if I should check the error code of FinishNewMessage.
+      // In some other places, yes I do.
+      // Am I being paranoid?
+      nsresult rv2 = m_msgStore->FinishNewMessage(m_outFileStream, hdr);
+      // FinishNewMessage used to close stream (before Bug 1121842, 1122698)
+      // And it does so now again.
+      m_outFileStream = nullptr;  // avoid accessing closed stream
+      // delayed processing of FinishNewMessage return value.
+      if (NS_FAILED(rv2)) NS_WARNING("m_msgStore->FinishNewMessage failed;");
     }
+    // PublishMsgHeader updates UI.
     m_newMailParser->PublishMsgHeader(aMsgWindow);
+
     // run any reply/forward filter after we've finished with the
     // temp quarantine file, and/or moved the message to another folder.
+#ifdef DEBUG
+    NS_WARNING("(debug): ApplyForwardAndReplyFilter getting called.");
+#endif
     m_newMailParser->ApplyForwardAndReplyFilter(aMsgWindow);
     if (aSize) hdr->SetUint32Property("onlineSize", aSize);
 
+    // |doSelect| is not set if |aSize| is and thus we use |else if.|
     // if DeleteDownloadMsg requested it, select the new message
     else if (doSelect)
       (void)localFolder->SelectDownloadMsg();
   }
 
 #ifdef DEBUG
   printf("Incorporate message complete.\n");
 #endif
   nsCOMPtr<nsIPop3Service> pop3Service(
       do_GetService(NS_POP3SERVICE_CONTRACTID1, &rv));
+#ifdef DEBUG
+  if (NS_FAILED(rv)) {
+    printf(
+        "pop3Service could not be obtained: we can not notify download "
+        "progress info.");
+  }
+#endif
   NS_ENSURE_SUCCESS(rv, rv);
   pop3Service->NotifyDownloadProgress(m_folder, ++m_numMsgsDownloaded,
                                       m_numNewMessages);
   return NS_OK;
 }
 
+//
+// CAUTION: Inside |IncorporateAbort|, |m_outFileStream| can be
+// nullptr due to the way error handling/recovery works.
+// When the stream associated with |m_outFileStream| is closed
+// during error processing, |m_outFileStream| is set to nullptr.
+//
 NS_IMETHODIMP
 nsPop3Sink::IncorporateAbort(bool uidlDownload) {
-  nsresult rv = m_outFileStream->Close();
+  nsresult rv = NS_OK;
+  // Note m_outFileStream can be nullptr.
+  if (m_outFileStream) {
+    rv = m_outFileStream->Close();
+#ifdef DEBUG
+    fprintf(stderr,
+            "{debug} IncorporateAbort: "
+            "m_outFileStream(%p)->Close() = "
+            "0x%" PRIx32 "\n",
+            (void *)m_outFileStream, static_cast<uint32_t>(rv));
+    if (NS_FAILED(rv)) {
+      fflush(stdout);
+      fprintf(stderr, "(debug) m_outFileStream->Close() failed. rv=0x%08x\n",
+              (unsigned)rv);
+      fflush(stderr);
+    }
+#endif
+    m_outFileStream = nullptr;
+  } else {
+#ifdef DEBUG
+    fprintf(
+        stderr,
+        "(debug) m_outFileStream was nullptr on entry to IncorporateAbort.\n");
+#endif
+  }
   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);
+    rv = m_msgStore->DiscardNewMessage(m_outFileStream,
+                                       m_newMailParser->m_newMsgHdr);
+    // Lucky us: DiscardNewMessage now tries to close file stream always unless
+    // stream is nullptr. We already set |m_outFileStream| to  nullptr at the
+    // beginning of this function! But it is hard to tell/read and so setting to
+    // nullptr again.
+    m_outFileStream = nullptr;
   }
 #ifdef DEBUG
   printf("Incorporate message abort.\n");
 #endif
   return rv;
 }
 
 nsresult nsPop3Sink::BiffGetNewMail() {
 #ifdef DEBUG
   printf("Biff get new mail.\n");
 #endif
   return NS_OK;
 }
 
+//
+// "FE" stands for Front End. i.e. User Interface (UI).
+//
 nsresult nsPop3Sink::SetBiffStateAndUpdateFE(uint32_t aBiffState,
                                              int32_t numNewMessages,
                                              bool notify) {
   m_biffState = aBiffState;
   if (m_newMailParser) numNewMessages -= m_newMailParser->m_numNotNewMessages;
 
   if (notify && m_folder && numNewMessages > 0 &&
       numNewMessages != m_numNewMessages &&
@@ -861,47 +1313,61 @@ nsresult nsPop3Sink::SetBiffStateAndUpda
     m_folder->SetNumNewMessages(numNewMessages);
     m_folder->SetBiffState(aBiffState);
   }
   m_numNewMessages = numNewMessages;
 
   return NS_OK;
 }
 
+//
+// m_buildMessageUri is set by SetBuildMessageUri() defined immediately below.
+// It is set to true by a call in nsPop3Service::NewURI() in nsPop3Service.cpp
+//
 NS_IMETHODIMP
 nsPop3Sink::GetBuildMessageUri(bool *bVal) {
   NS_ENSURE_ARG_POINTER(bVal);
   *bVal = m_buildMessageUri;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsPop3Sink::SetBuildMessageUri(bool bVal) {
   m_buildMessageUri = bVal;
   return NS_OK;
 }
 
+//
+// XXX TODO Better comment/documentation
+// A series of URIs are handled: what are the differences?
+// Also the way they are set and retrieved are different.
+// MessageUri        set from char *
+// BaseMessageUri    set from char *
+// OrigMessageUri    set from nsACString& aOrigMessageUri
+//
 NS_IMETHODIMP
 nsPop3Sink::GetMessageUri(char **messageUri) {
   NS_ENSURE_ARG_POINTER(messageUri);
-  NS_ENSURE_TRUE(!m_messageUri.IsEmpty(), NS_ERROR_FAILURE);
+  NS_ENSURE_TRUE(!m_messageUri.IsEmpty(), NS_ERROR_NOT_INITIALIZED);
   *messageUri = ToNewCString(m_messageUri);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsPop3Sink::SetMessageUri(const char *messageUri) {
   NS_ENSURE_ARG_POINTER(messageUri);
   m_messageUri = messageUri;
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsPop3Sink::GetBaseMessageUri(char **baseMessageUri) {
   NS_ENSURE_ARG_POINTER(baseMessageUri);
+  // XXX Can't we be more imaginative by using other error code
+  // than  NS_ERROR_FAILURE?
   NS_ENSURE_TRUE(!m_baseMessageUri.IsEmpty(), NS_ERROR_FAILURE);
   *baseMessageUri = ToNewCString(m_baseMessageUri);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsPop3Sink::SetBaseMessageUri(const char *baseMessageUri) {
   NS_ENSURE_ARG_POINTER(baseMessageUri);
--- a/mailnews/local/src/nsPop3Sink.h
+++ b/mailnews/local/src/nsPop3Sink.h
@@ -55,16 +55,22 @@ class nsPop3Sink : public nsIPop3Sink {
   bool m_senderAuthed;
   nsCString m_outputBuffer;
   nsCOMPtr<nsIPop3IncomingServer> m_popServer;
   // Currently the folder we want to update about biff info
   nsCOMPtr<nsIMsgFolder> m_folder;
   RefPtr<nsParseNewMailState> m_newMailParser;
   nsCOMPtr<nsIOutputStream>
       m_outFileStream;  // the file we write to, which may be temporary
+
+  // we used to read from the temporary file above,
+  // i.e. |m_outFileStream|, which was confusing AND buggy when
+  // m_outFileStream became buffered.
+  // Hence we introduced |m_inboxInputStream| for reading;
+  nsCOMPtr<nsIInputStream> m_inboxInputStream;
   nsCOMPtr<nsIMsgPluggableStore> m_msgStore;
   bool m_uidlDownload;
   bool m_buildMessageUri;
   bool m_downloadingToTempFile;
   nsCOMPtr<nsIFile> m_tmpDownloadFile;
   nsCOMPtr<nsIMsgWindow> m_window;
   nsCString m_messageUri;
   nsCString m_baseMessageUri;