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>
Sun, 20 Oct 2019 05:51:04 +0900
changeset 80327 ee4730e6f42ef71ee825e051c92fc07eb2698a0e
parent 80326 5d42e5d7861b4d1c9415f5468d8299e9b4081bcc
child 80328 9d4be59651880855b94939ef2b82e0823ccded3d
push id9576
push userishikawa@yk.rim.or.jp
push dateSat, 19 Oct 2019 20:51:19 +0000
treeherdertry-comm-central@4e899dfbba0c [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/util/nsMsgDBFolder.cpp
--- a/mailnews/base/public/nsIMsgPluggableStore.idl
+++ b/mailnews/base/public/nsIMsgPluggableStore.idl
@@ -12,17 +12,17 @@ interface nsIMsgDBHdr;
 interface nsIMsgWindow;
 interface nsIOutputStream;
 interface nsIInputStream;
 interface nsIArray;
 interface nsIUrlListener;
 interface nsIMsgDatabase;
 interface nsITransaction;
 
-[scriptable, uuid(F732CE58-E540-4dc4-B803-9456056EBEFC)]
+[scriptable, uuid(046AB3DC-D4C7-11E5-95DB-0800272954B9)]
 
 /**
  * Pluggable message store interface. Each incoming server can have a different
  * message store.
  * All methods are synchronous unless otherwise specified.
  */
 interface nsIMsgPluggableStore : nsISupports {
   /**
@@ -112,51 +112,59 @@ 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.
+//   * @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/util/nsMsgDBFolder.cpp
+++ b/mailnews/base/util/nsMsgDBFolder.cpp
@@ -706,26 +706,46 @@ bool nsMsgDBFolder::VerifyOfflineMessage
 
 NS_IMETHODIMP nsMsgDBFolder::GetOfflineFileStream(
     nsMsgKey msgKey, int64_t *offset, uint32_t *size,
     nsIInputStream **aFileStream) {
   NS_ENSURE_ARG(aFileStream);
 
   *offset = *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, NS_OK);
   nsCOMPtr<nsIMsgDBHdr> hdr;
   rv = mDatabase->GetMsgHdrForKey(msgKey, getter_AddRefs(hdr));
   NS_ENSURE_TRUE(hdr, NS_OK);
   NS_ENSURE_SUCCESS(rv, rv);
   if (hdr) 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;
+  }
+
   // check if offline store really has the correct offset into the offline
   // store by reading the first few bytes. If it doesn't, clear the offline
   // flag on the msg and return false, which will fall back to reading the
   // message from the server. We'll also advance the offset past the envelope
   // header and X-Mozilla-Status lines.
   nsCOMPtr<nsISeekableStream> seekableStream = do_QueryInterface(*aFileStream);
   if (seekableStream) {
     seekableStream->Tell(offset);
@@ -760,16 +780,22 @@ NS_IMETHODIMP nsMsgDBFolder::GetOfflineF
       // Check that the first line is a header line, i.e., with a ':' in it.
       // Or, the line starts with "From " - I've seen IMAP servers return
       // a bogus "From " line without a ':'.
       if (findPos != -1 &&
           (startOfMsg[findPos] == ':' || !(strncmp(startOfMsg, "From ", 5)))) {
         *offset += msgOffset;
         *size -= msgOffset;
       } else {
+#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
         rv = NS_ERROR_FAILURE;
       }
     }
     if (NS_SUCCEEDED(rv))
       seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, *offset);
     else if (mDatabase)
       mDatabase->MarkOffline(msgKey, false, nullptr);
   }
@@ -781,19 +807,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);
@@ -803,16 +827,24 @@ nsMsgDBFolder::GetMsgInputStream(nsIMsgD
   nsresult rv = GetMsgStore(getter_AddRefs(msgStore));
   NS_ENSURE_SUCCESS(rv, rv);
   nsCString storeToken;
   rv = aMsgHdr->GetStringProperty("storeToken", getter_Copies(storeToken));
   NS_ENSURE_SUCCESS(rv, rv);
   int64_t offset;
   rv = msgStore->GetMsgInputStream(this, storeToken, &offset, aMsgHdr,
                                    aReusable, aInputStream);
+#ifdef DEBUG
+  if (NS_FAILED(rv)) {
+    fprintf(stderr,
+            "(debug) nsMsgDBFolder::GetMsgInputStream: msgStore->"
+            "GetMsgInputStream(this, ...) returned error rv=%" PRIx32 "\n",
+            static_cast<uint32_t>(rv));
+  }
+#endif
   NS_ENSURE_SUCCESS(rv, rv);
   nsCOMPtr<nsISeekableStream> seekableStream(do_QueryInterface(*aInputStream));
   if (seekableStream) rv = seekableStream->Seek(PR_SEEK_SET, offset);
   NS_WARNING_ASSERTION(seekableStream || !offset,
                        "non-zero offset w/ non-seekable stream");
   return rv;
 }
 
@@ -897,16 +929,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
@@ -1524,16 +1563,19 @@ nsresult nsMsgDBFolder::WriteStartOfNewL
   ct[24] = 0;
   result = "From - ";
   result += ct;
   result += MSG_LINEBREAK;
   m_bytesAddedToLocalMsg = result.Length();
 
   nsCOMPtr<nsISeekableStream> seekable;
 
+  MOZ_ASSERT(m_tempMessageStream,
+             "Temporary message stream must not be nullptr");
+
   if (m_offlineHeader) seekable = do_QueryInterface(m_tempMessageStream);
 
   m_tempMessageStream->Write(result.get(), result.Length(), &writeCount);
 
   NS_NAMED_LITERAL_CSTRING(MozillaStatus,
                            "X-Mozilla-Status: 0001" MSG_LINEBREAK);
   m_tempMessageStream->Write(MozillaStatus.get(), MozillaStatus.Length(),
                              &writeCount);
@@ -1568,82 +1610,101 @@ 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));
 
   if (seekable) {
     mDatabase->MarkOffline(messageKey, true, nullptr);
+
+    MOZ_ASSERT(m_tempMessageStream,
+               "Temporary message stream must not be nullptr");
+
     m_tempMessageStream->Flush();
     int64_t tellPos;
     seekable->Tell(&tellPos);
     curStorePos = tellPos;
 
     // N.B. This only works if we've set the offline flag for the message,
     // so be careful about moving the call to MarkOffline above.
     m_offlineHeader->GetMessageOffset(&messageOffset);
     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=%d "
+              "curStorePos=%" PRId64 " numOfflineMsgLines=%d bytesAdded=%d\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;
 }
 
 nsresult nsMsgDBFolder::CompactOfflineStore(nsIMsgWindow *inWindow,
                                             nsIUrlListener *aListener) {
   nsresult rv;
   nsCOMPtr<nsIMsgFolderCompactor> folderCompactor =
       do_CreateInstance(NS_MSGOFFLINESTORECOMPACTOR_CONTRACTID, &rv);