bug 1242030: DONT-USE-REUSABLE new part-1 new splitting of (consolidation of 1122698, 1134527, 1134529, 1174500) draft
authorISHIKAWA, Chiaki <ishikawa@yk.rim.or.jp>
Mon, 23 May 2022 03:03:50 +0900
changeset 116814 6a30493af21436eb1e76c71716a4a989cc200ded
parent 116813 cd142979ab21feb373574fad9c17ee586569069a
child 116815 2fb240c10bf3ac417acce2f7c9694eb9c535bf1b
push id15955
push userishikawa@yk.rim.or.jp
push dateSun, 22 May 2022 18:04:18 +0000
treeherdertry-comm-central@beaacca97fbd [default view] [failures only]
bugs1242030, 1122698, 1134527, 1134529, 1174500
bug 1242030: DONT-USE-REUSABLE new part-1 new splitting of (consolidation of 1122698, 1134527, 1134529, 1174500)
mailnews/base/public/nsIMsgPluggableStore.idl
mailnews/base/src/nsMsgDBFolder.cpp
mailnews/local/src/nsLocalMailFolder.cpp
--- a/mailnews/base/public/nsIMsgPluggableStore.idl
+++ b/mailnews/base/public/nsIMsgPluggableStore.idl
@@ -110,51 +110,54 @@ interface nsIMsgPluggableStore : nsISupp
    *                to create a new header for the new message. If the db
    *                is invalid, this can be null. But if the db is valid,
    *                the store should create a message header with the right
    *                message key, or whatever other property it needs to set to
    *                be able to retrieve the message contents later. If the store
    *                needs to base any of this on the contents of the message,
    *                it will need remember the message header and hook into
    *                the output stream somehow to alter the message header.
-   * @param aReusable set to true on output if the caller can reuse the
-   *                  stream for multiple messages, e.g., mbox format.
-   *                  This means the caller will likely get the same stream
-   *                  back on multiple calls to this method, and shouldn't
-   *                  close the stream in between calls if they want reuse.
    *
    * @return The output stream to write to. The output stream will be positioned
    *         for writing (e.g., for berkeley mailbox, it will be at the end).
    */
   nsIOutputStream getNewMsgOutputStream(in nsIMsgFolder aFolder,
-                                        inout nsIMsgDBHdr aNewHdr,
-                                        out boolean aReusable);
+                                        inout nsIMsgDBHdr aNewHdr);
+
 
   /**
    * Called when the current message is discarded, e.g., it is moved
    * to an other folder as a filter action, or is deleted because it's
    * a duplicate. This gives the berkeley mailbox store a chance to simply
    * truncate the Inbox w/o leaving a deleted message in the store.
    *
+   * discardNewMessage closes aOutputStream always unless the passed stream
+   * is nullptr due to error processing..
+   * (Clarification/Rationale in Bug 1121842, 1122698, 1242030)
+   *
    * @param aOutputStream stream we were writing the message to be discarded to
    * @param aNewHdr header of message to discard
    */
   void discardNewMessage(in nsIOutputStream aOutputStream,
                          in nsIMsgDBHdr aNewHdr);
 
   /**
    * Must be called by code that calls getNewMsgOutputStream to finish
    * the process of storing a new message, if the new msg has not been
    * discarded. Could/should this be combined with discardNewMessage?
    *
+   * finishdNewMessage closes aOutputStream always unless the passed stream
+   * is nullptr due to error processing.
+   * (Clarification/Rationale in Bug 1121842, 1122698, 1242030)
+   *
    * @param aOutputStream stream we were writing the message to.
    * @param aNewHdr header of message finished.
    */
   void finishNewMessage(in nsIOutputStream aOutputStream,
-                         in nsIMsgDBHdr aNewHdr);
+                        in nsIMsgDBHdr aNewHdr);
 
   /**
    * Called by pop3 message filters when a newly downloaded message is being
    * moved by an incoming filter. This is called before finishNewMessage, and
    * it allows the store to optimize that case.
    *
    * @param aNewHdr msg hdr of message being moved.
    * @param aDestFolder folder to move message to, in the same store.
--- a/mailnews/base/src/nsMsgDBFolder.cpp
+++ b/mailnews/base/src/nsMsgDBFolder.cpp
@@ -733,25 +733,43 @@ NS_IMETHODIMP nsMsgDBFolder::GetMsgStore
 nsresult nsMsgDBFolder::GetOfflineFileStream(nsMsgKey msgKey, uint64_t* offset,
                                              uint32_t* size,
                                              nsIInputStream** aFileStream) {
   NS_ENSURE_ARG(aFileStream);
 
   *offset = 0;
   *size = 0;
 
+  // Initialise to nullptr since this is checked by some callers for the success
+  // of the function.
+  *aFileStream = nullptr;
+
   nsresult rv = GetDatabase();
   NS_ENSURE_SUCCESS(rv, rv);
   nsCOMPtr<nsIMsgDBHdr> hdr;
   rv = mDatabase->GetMsgHdrForKey(msgKey, getter_AddRefs(hdr));
   NS_ENSURE_SUCCESS(rv, rv);
   hdr->GetOfflineMessageSize(size);
 
   bool reusable;
   rv = GetMsgInputStream(hdr, &reusable, aFileStream);
+  if (NS_FAILED(rv)) {
+#ifdef DEBUG
+    fprintf(stderr,
+            "(debug) nsMsgDBFolder::GetOfflineFileStream: "
+            "GetMsgInputStream(hdr, &reusable, aFileStream); rv=0x%" PRIx32
+            "\n",
+            static_cast<uint32_t>(rv));
+#endif
+    // Return early: If we could not find an offline stream, clear the offline
+    // flag which will fall back to reading the message from the server.
+    if (mDatabase) mDatabase->MarkOffline(msgKey, false, nullptr);
+
+   return rv;
+  }
   NS_ENSURE_SUCCESS(rv, rv);
   // Check if the database has the correct offset into the offline store by
   // reading up to 300 bytes. If it is incorrect, clear the offline flag on the
   // message and return false. This will cause a fall back to reading the
   // message from the server. We will also advance the offset past the envelope
   // header ("From " or "FCC") and "X-Mozilla-Status*" lines so these line are
   // not included when the message is read from the file.
   // Note: This occurs for both mbox and maildir offline store and probably any
@@ -822,16 +840,22 @@ nsresult nsMsgDBFolder::GetOfflineFileSt
           foundNextLine =
               MsgAdvanceToNextLine(startOfMsg, msgOffset, bytesRead - 1);
           if (MOZ_LOG_TEST(DBLog, LogLevel::Info)) {
             char save;
             if (foundNextLine) {
               // Temporarily null terminate the bad header line for logging.
               save = startOfMsg[msgOffset - 1];  // should be \r or \n
               startOfMsg[msgOffset - 1] = 0;
+#ifdef DEBUG
+              // DEBUG: the error happened while checking network outage condition.
+              // XXX TODO We may need to check if dovecot and other imap
+              // servers are returning funny "From " line, etc.
+              fprintf(stderr, "(debug) Strange startOfMsg: |%s|\n", startOfMsg);
+#endif
             }
             MOZ_LOG(DBLog, LogLevel::Info,
                     ("Invalid header line in offline store: %s",
                      startOfMsg + keepMsgOffset));
             if (foundNextLine) startOfMsg[msgOffset - 1] = save;
           }
           line1 = false;
           continue;
@@ -893,19 +917,17 @@ NS_IMETHODIMP
 nsMsgDBFolder::GetOfflineStoreOutputStream(nsIMsgDBHdr* aHdr,
                                            nsIOutputStream** aOutputStream) {
   NS_ENSURE_ARG_POINTER(aOutputStream);
   NS_ENSURE_ARG_POINTER(aHdr);
 
   nsCOMPtr<nsIMsgPluggableStore> offlineStore;
   nsresult rv = GetMsgStore(getter_AddRefs(offlineStore));
   NS_ENSURE_SUCCESS(rv, rv);
-  bool reusable;
-  rv = offlineStore->GetNewMsgOutputStream(this, &aHdr, &reusable,
-                                           aOutputStream);
+  rv = offlineStore->GetNewMsgOutputStream(this, &aHdr, aOutputStream);
   NS_ENSURE_SUCCESS(rv, rv);
   return rv;
 }
 
 NS_IMETHODIMP
 nsMsgDBFolder::GetMsgInputStream(nsIMsgDBHdr* aMsgHdr, bool* aReusable,
                                  nsIInputStream** aInputStream) {
   NS_ENSURE_ARG_POINTER(aMsgHdr);
@@ -932,16 +954,24 @@ nsMsgDBFolder::GetMsgInputStream(nsIMsgD
     uint64_t offset;
     aMsgHdr->GetMessageOffset(&offset);
     storeToken = nsPrintfCString("%" PRIu64, offset);
     rv = aMsgHdr->SetStringProperty("storeToken", storeToken.get());
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   rv = msgStore->GetMsgInputStream(this, storeToken, aReusable, aInputStream);
+#ifdef DEBUG
+  if (NS_FAILED(rv)) {
+    fprintf(stderr,
+            "(debug) nsMsgDBFolder::GetMsgInputStream: msgStore->"
+            "GetMsgInputStream(this, ...) returned error rv=0x%" PRIx32 "\n",
+            static_cast<uint32_t>(rv));
+  }
+#endif
   NS_ENSURE_SUCCESS(rv, rv);
   return NS_OK;
 }
 
 // path coming in is the root path without the leaf name,
 // on the way out, it's the whole path.
 nsresult nsMsgDBFolder::CreateFileForDB(const nsAString& userLeafName,
                                         nsIFile* path, nsIFile** dbFile) {
@@ -1025,16 +1055,23 @@ nsMsgDBFolder::GetDatabaseOpen(bool* aOp
   *aOpen = (mDatabase != nullptr);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsMsgDBFolder::GetBackupMsgDatabase(nsIMsgDatabase** aMsgDatabase) {
   NS_ENSURE_ARG_POINTER(aMsgDatabase);
   nsresult rv = OpenBackupMsgDatabase();
+#ifdef DEBUG
+  if (NS_FAILED(rv)) {
+    fprintf(stderr,
+            "(debug) OpenBackupMsgDatabase(); returns error=0x%" PRIx32 "\n",
+            static_cast<uint32_t>(rv));
+  }
+#endif
   NS_ENSURE_SUCCESS(rv, rv);
   if (!mBackupDatabase) return NS_ERROR_FAILURE;
 
   NS_ADDREF(*aMsgDatabase = mBackupDatabase);
   return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -1591,16 +1628,20 @@ nsresult nsMsgDBFolder::WriteStartOfNewL
   uint32_t writeCount;
   time_t now = time((time_t*)0);
   char* ct = ctime(&now);
   ct[24] = 0;
   result = "From - ";
   result += ct;
   result += MSG_LINEBREAK;
   m_bytesAddedToLocalMsg = result.Length();
+
+  MOZ_ASSERT(m_tempMessageStream,
+             "Temporary message stream must not be nullptr");
+
   m_tempMessageStream->Write(result.get(), result.Length(), &writeCount);
 
   constexpr auto MozillaStatus = "X-Mozilla-Status: 0001"_ns MSG_LINEBREAK;
   m_tempMessageStream->Write(MozillaStatus.get(), MozillaStatus.Length(),
                              &writeCount);
   m_bytesAddedToLocalMsg += writeCount;
   constexpr auto MozillaStatus2 =
       "X-Mozilla-Status2: 00000000"_ns MSG_LINEBREAK;
@@ -1632,27 +1673,33 @@ nsresult nsMsgDBFolder::StartNewOfflineM
 
 nsresult nsMsgDBFolder::EndNewOfflineMessage() {
   nsCOMPtr<nsISeekableStream> seekable;
   int64_t curStorePos;
   uint64_t messageOffset;
   uint32_t messageSize;
 
   nsMsgKey messageKey;
+  nsresult rv1, rv2;
 
   nsresult rv = GetDatabase();
   NS_ENSURE_SUCCESS(rv, rv);
 
   m_offlineHeader->GetMessageKey(&messageKey);
   if (m_tempMessageStream) seekable = do_QueryInterface(m_tempMessageStream);
 
   nsCOMPtr<nsIMsgPluggableStore> msgStore;
   GetMsgStore(getter_AddRefs(msgStore));
 
   mDatabase->MarkOffline(messageKey, true, nullptr);
+
+  // No longer true?
+  //  MOZ_ASSERT(m_tempMessageStream,
+  //           "Temporary message stream must not be nullptr");
+
   if (m_tempMessageStream) {
     m_tempMessageStream->Flush();
   }
   if (seekable) {
     int64_t tellPos;
     seekable->Tell(&tellPos);
     curStorePos = tellPos;
 
@@ -1662,54 +1709,70 @@ nsresult nsMsgDBFolder::EndNewOfflineMes
     curStorePos -= messageOffset;
     m_offlineHeader->SetOfflineMessageSize(curStorePos);
     m_offlineHeader->GetMessageSize(&messageSize);
     messageSize += m_bytesAddedToLocalMsg;
     // unix/mac has a one byte line ending, but the imap server returns
     // crlf terminated lines.
     if (MSG_LINEBREAK_LEN == 1) messageSize -= m_numOfflineMsgLines;
 
+    rv1 = rv2 = NS_OK;
     // We clear the offline flag on the message if the size
     // looks wrong. Check if we're off by more than one byte per line.
     if (messageSize > (uint32_t)curStorePos &&
         (messageSize - (uint32_t)curStorePos) >
             (uint32_t)m_numOfflineMsgLines) {
       mDatabase->MarkOffline(messageKey, false, nullptr);
       // we should truncate the offline store at messageOffset
       ReleaseSemaphore(static_cast<nsIMsgFolder*>(this));
-      if (msgStore)
-        // this closes the stream
-        msgStore->DiscardNewMessage(m_tempMessageStream, m_offlineHeader);
-      else
-        m_tempMessageStream->Close();
-      m_tempMessageStream = nullptr;
-#ifdef _DEBUG
-      nsAutoCString message("Offline message too small: messageSize=");
-      message.AppendInt(messageSize);
-      message.AppendLiteral(" curStorePos=");
-      message.AppendInt(curStorePos);
-      message.AppendLiteral(" numOfflineMsgLines=");
-      message.AppendInt(m_numOfflineMsgLines);
-      message.AppendLiteral(" bytesAdded=");
-      message.AppendInt(m_bytesAddedToLocalMsg);
-      NS_ERROR(message.get());
+      if (msgStore) {
+        // DiscardNewMessage may not close the stream all the time.
+        rv1 = msgStore->DiscardNewMessage(m_tempMessageStream, m_offlineHeader);
+        m_tempMessageStream = nullptr;  // avoid accessing closed stream
+      }
+      // XXX We should check for errors of rv1 and rv2.
+      if (NS_FAILED(rv1)) NS_WARNING("DiscardNewMessage returned error");
+      if (NS_FAILED(rv2))  // not called actually in the new code.
+        NS_WARNING("m_tempMessageStream->close returned error");
+#ifdef DEBUG
+      fprintf(stderr,
+              "(debug) Offline message too small: messageSize=%" PRIu32
+              "curStorePos=%" PRId64 " numOfflineMsgLines=%" PRId32
+              " bytesAdded=%" PRId32 "\n",
+              messageSize, curStorePos, m_numOfflineMsgLines,
+              m_bytesAddedToLocalMsg);
 #endif
+
       m_offlineHeader = nullptr;
       return NS_ERROR_FAILURE;
     } else
       m_offlineHeader->SetLineCount(m_numOfflineMsgLines);
+  }  // if (seekable)
+
+  rv1 = rv2 = NS_OK;
+  if (msgStore) {
+    // FinishNewMessage now closes the stream ALL THE TIME.
+    rv1 = msgStore->FinishNewMessage(m_tempMessageStream, m_offlineHeader);
+    m_tempMessageStream = nullptr;
   }
-  if (msgStore)
-    msgStore->FinishNewMessage(m_tempMessageStream, m_offlineHeader);
+#ifdef DEBUG
+  // We can not let this happe: I think the code assumes this.
+  NS_ASSERTION(msgStore, "msgStore is nullptr");
+#endif
 
   m_offlineHeader = nullptr;
-  if (m_tempMessageStream) {
-    m_tempMessageStream->Close();
-    m_tempMessageStream = nullptr;
-  }
+  // Notify users of the errors for now, just use NS_WARNING.
+  if (NS_FAILED(rv1)) NS_WARNING("FinishNewMessage returned error");
+  if (NS_FAILED(rv2)) NS_WARNING("m_tempMessageStream->Close() returned error");
+
+  m_tempMessageStream = nullptr;
+
+  if (NS_FAILED(rv1)) return rv1;
+  if (NS_FAILED(rv2)) return rv2;
+
   return NS_OK;
 }
 
 class AutoCompactEvent : public mozilla::Runnable {
  public:
   AutoCompactEvent(nsIMsgWindow* aMsgWindow, nsMsgDBFolder* aFolder)
       : mozilla::Runnable("AutoCompactEvent"),
         mMsgWindow(aMsgWindow),
--- a/mailnews/local/src/nsLocalMailFolder.cpp
+++ b/mailnews/local/src/nsLocalMailFolder.cpp
@@ -1912,19 +1912,18 @@ nsresult nsMsgLocalMailFolder::WriteStar
 
   mCopyState->m_curCopyIndex++;
   return NS_OK;
 }
 
 nsresult nsMsgLocalMailFolder::InitCopyMsgHdrAndFileStream() {
   nsresult rv = GetMsgStore(getter_AddRefs(mCopyState->m_msgStore));
   NS_ENSURE_SUCCESS(rv, rv);
-  bool reusable;
   rv = mCopyState->m_msgStore->GetNewMsgOutputStream(
-      this, getter_AddRefs(mCopyState->m_newHdr), &reusable,
+      this, getter_AddRefs(mCopyState->m_newHdr),
       getter_AddRefs(mCopyState->m_fileStream));
   NS_ENSURE_SUCCESS(rv, rv);
   if (mCopyState->m_parseMsgState)
     mCopyState->m_parseMsgState->SetNewMsgHdr(mCopyState->m_newHdr);
   return rv;
 }
 
 // nsICopyMessageListener
@@ -3393,19 +3392,17 @@ nsMsgLocalMailFolder::AddMessageBatch(
   AcquireSemaphore(static_cast<nsIMsgLocalMailFolder*>(this));
 
   if (NS_SUCCEEDED(rv)) {
     NS_ENSURE_SUCCESS(rv, rv);
     for (uint32_t i = 0; i < aMessages.Length(); i++) {
       RefPtr<nsParseNewMailState> newMailParser = new nsParseNewMailState;
       NS_ENSURE_TRUE(newMailParser, NS_ERROR_OUT_OF_MEMORY);
       if (!mGettingNewMessages) newMailParser->DisableFilters();
-      bool reusable;
       rv = msgStore->GetNewMsgOutputStream(this, getter_AddRefs(newHdr),
-                                           &reusable,
                                            getter_AddRefs(outFileStream));
       NS_ENSURE_SUCCESS(rv, rv);
 
       // Get a msgWindow. Proceed without one, but filter actions to imap
       // folders will silently fail if not signed in and no window for a prompt.
       nsCOMPtr<nsIMsgWindow> msgWindow;
       nsCOMPtr<nsIMsgMailSession> mailSession =
           do_GetService(NS_MSGMAILSESSION_CONTRACTID, &rv);