Bug 1170606 part-5: add short read handling to files under mailnews/local/src draft
authorISHIKAWA, Chiaki <ishikawa@yk.rim.or.jp>
Wed, 01 Jul 2020 13:26:15 +0900
changeset 89800 76ef617f9a95af7c20ef219079664765e59cbb87
parent 89799 4115c68473dc1abec18bd6b7aaee6f82461724c6
child 89801 d9ffe5e92613921c6dc29569ecc38a2b988525bc
push id11337
push userishikawa@yk.rim.or.jp
push dateWed, 01 Jul 2020 04:26:34 +0000
treeherdertry-comm-central@5b0c8a7b2471 [default view] [failures only]
bugs1170606
Bug 1170606 part-5: add short read handling to files under mailnews/local/src
mailnews/local/src/nsLocalMailFolder.cpp
mailnews/local/src/nsMsgLocalStoreUtils.cpp
mailnews/local/src/nsParseMailbox.cpp
--- a/mailnews/local/src/nsLocalMailFolder.cpp
+++ b/mailnews/local/src/nsLocalMailFolder.cpp
@@ -1756,24 +1756,24 @@ nsMsgLocalMailFolder::CopyFileMessage(ns
 
     // All or none for adding a message file to the store
     if (NS_SUCCEEDED(rv) && fileSize > PR_INT32_MAX)
       rv = NS_ERROR_ILLEGAL_VALUE;  // may need error code for max msg size
 
     if (NS_SUCCEEDED(rv) && inputStream) {
       char buffer[5];
       uint32_t readCount;
-      rv = inputStream->Read(buffer, 5, &readCount);
+      rv = FullyReadStream(inputStream, buffer, 5, &readCount);
       if (NS_SUCCEEDED(rv)) {
         if (strncmp(buffer, "From ", 5))
           mCopyState->m_dummyEnvelopeNeeded = true;
         nsCOMPtr<nsISeekableStream> seekableStream =
             do_QueryInterface(inputStream, &rv);
         if (NS_SUCCEEDED(rv))
-          seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, 0);
+          rv = seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, 0);
       }
     }
 
     mCopyState->m_wholeMsgInStream = true;
     if (NS_SUCCEEDED(rv)) rv = BeginCopy(nullptr);
 
     if (NS_SUCCEEDED(rv)) rv = CopyData(inputStream, (int32_t)fileSize);
 
@@ -2165,25 +2165,25 @@ NS_IMETHODIMP nsMsgLocalMailFolder::Copy
   nsCOMPtr<nsISeekableStream> seekableStream =
       do_QueryInterface(mCopyState->m_fileStream, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
   seekableStream->Seek(nsISeekableStream::NS_SEEK_END, 0);
 
   // BTW, under Linux, and for POP3 case, Read is handled by recvfrom in
   // lower-level code with 32KB buffer: verified from strace data
 
-  rv = aIStream->Read(mCopyState->m_dataBuffer + mCopyState->m_leftOver + 1,
-                      aLength, &readCount);
-  NS_ENSURE_SUCCESS(rv, rv);
+  rv = FullyReadStream(aIStream,
+                       mCopyState->m_dataBuffer + mCopyState->m_leftOver + 1,
+                       aLength, &readCount);
   if (NS_FAILED(rv)) {
 #ifdef DEBUG
     fprintf(stderr,
             "(debug) nsMsgLocalMailFolder::CopyData() "
-            "aIStream->Read(mCopyState->m_dataBuffer + mCopyState->m_leftOver "
-            "+ 1, aLength, &readCount); failed. "
+            "FullyReadStream(aIStream, mCopyState->m_dataBuffer + "
+            "mCopyState->m_leftOver + 1, aLength, &readCount); failed. "
             "rv=0x%" PRIx32 "\n",
             static_cast<uint32_t>(rv));
 #endif
     return rv;
   }
   mCopyState->m_leftOver += readCount;
   mCopyState->m_dataBuffer[mCopyState->m_leftOver + 1] = '\0';
   char* start = mCopyState->m_dataBuffer + 1;
--- a/mailnews/local/src/nsMsgLocalStoreUtils.cpp
+++ b/mailnews/local/src/nsMsgLocalStoreUtils.cpp
@@ -254,18 +254,35 @@ nsresult nsMsgLocalStoreUtils::UpdateFol
   rv = seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, statusPos);
   NS_ENSURE_SUCCESS(rv, rv);
 
   char buf[50];
   buf[0] = '\0';
   nsCOMPtr<nsIInputStream> inputStream = do_QueryInterface(fileStream, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
   uint32_t bytesRead;
-  if (NS_SUCCEEDED(
-          inputStream->Read(buf, X_MOZILLA_STATUS_LEN + 6, &bytesRead))) {
+
+  // XXX We need to handle short read
+  rv = FullyReadStream(inputStream, buf, X_MOZILLA_STATUS_LEN + 6, &bytesRead);
+
+  if (NS_SUCCEEDED(rv) && (X_MOZILLA_STATUS_LEN + 6) == bytesRead)
+    ;  // OK
+  else {
+#ifdef DEBUG
+    // to investigate strange Windows error on tryserver
+    fflush(stdout);
+    fprintf(stderr,
+            "UpdateFolderFlag: failed read 0x%08x, bytesRead = %d "
+            "(X_MOZILLA_STATUS_LEN + 6=%d)\n",
+            (unsigned int)rv, bytesRead, X_MOZILLA_STATUS_LEN + 6);
+#endif
+    return rv = NS_ERROR_FAILURE;
+  }
+
+  if (1 /*dummy*/) {  // Keep the same indentation for comparison purposes
     buf[bytesRead] = '\0';
     if (strncmp(buf, X_MOZILLA_STATUS, X_MOZILLA_STATUS_LEN) == 0 &&
         strncmp(buf + X_MOZILLA_STATUS_LEN, ": ", 2) == 0 &&
         strlen(buf) >= X_MOZILLA_STATUS_LEN + 6) {
       uint32_t flags;
       uint32_t bytesWritten;
       (void)mailHdr->GetFlags(&flags);
       if (!(flags & nsMsgMessageFlags::Expunged)) {
@@ -292,47 +309,94 @@ nsresult nsMsgLocalStoreUtils::UpdateFol
       // can be done.
 #if 0
       // Not done currently done.
       nsresult rv1 =  fileStream->Flush();
       NS_WARN_IF_FALSE(NS_SUCCEEDED(rv1), "fileStream->Flush() failed.");
 #endif
 
       // XXX we ought to check for Seek failure, too...
-      seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, statusPos);
+      // This may happen if a remote file system failure occurs when Maildir is
+      // on such a remote file system. An error after a timeout will return!
+
+      nsresult rv5 =
+          seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, statusPos);
+      if (NS_FAILED(rv5)) {
+        MOZ_ASSERT(NS_SUCCEEDED(rv5),
+                   "seek: statusPos ought to have succeeded ");
+        // XXX better error recovery
+        return rv5;
+      }
       // We are filing out x-mozilla-status flags here
       PR_snprintf(buf, sizeof(buf), X_MOZILLA_STATUS_FORMAT,
                   flags & 0x0000FFFF);
       int32_t lineLen = PL_strlen(buf);
       uint64_t status2Pos = statusPos + lineLen;
       nsresult rv4 = fileStream->Write(buf, lineLen, &bytesWritten);
-      if (NS_FAILED(rv4) || lineLen != (int)bytesWritten) {
-        MOZ_ASSERT(NS_SUCCEEDED(rv4) && lineLen == (int)bytesWritten,
+      if (NS_FAILED(rv4) || lineLen != (int32_t)bytesWritten) {
+        MOZ_ASSERT(NS_SUCCEEDED(rv4) && lineLen == (int32_t)bytesWritten,
                    "fileStream->Write(buf, lineLen, &bytesWritten);");
         // XXX Better error recovery.
+#ifdef DEBUG
+        fprintf(stderr,
+                "UpdateFolderFlag: failed Write 0x%" PRIx32
+                ", lineLen=%d, bytesWritten=%d\n",
+                static_cast<uint32_t>(rv4), lineLen, bytesWritten);
+#endif
+        return rv4;
       }
 
       if (flag & 0xFFFF0000) {
         // The following seek is for read.
         // XXX TODO We ought to check for Seek failure, too...
 
         // Time to update x-mozilla-status2,
         // first find it by finding end of previous line, see bug 234935.
-        seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, status2Pos);
+        rv5 = seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, status2Pos);
+        if (NS_FAILED(rv5)) {
+          MOZ_ASSERT(NS_SUCCEEDED(rv5),
+                     "seek: status2Pos ought to have succeeded");
+          return rv5;
+        }
         do {
-          rv = inputStream->Read(buf, 1, &bytesRead);
+          rv = FullyReadStream(inputStream, buf, 1, &bytesRead);
           status2Pos++;
         } while (NS_SUCCEEDED(rv) && (*buf == '\n' || *buf == '\r'));
         status2Pos--;
 
         // This seek is for read.
         // XXX TODO We ought to check for Seek failure, too...
-        seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, status2Pos);
-        if (NS_SUCCEEDED(inputStream->Read(buf, X_MOZILLA_STATUS2_LEN + 10,
-                                           &bytesRead))) {
+        // This may happen if a remote file system failure occurs when Maildir
+        // is on such a remote file system. An error after a timeout will
+        // return!
+        rv5 = seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, status2Pos);
+        if (NS_FAILED(rv5)) {
+          MOZ_ASSERT(NS_SUCCEEDED(rv5),
+                     "seek: status2pos ought to have succeeded. part-2");
+          return rv5;
+        }
+
+        rv5 = FullyReadStream(inputStream, buf, X_MOZILLA_STATUS2_LEN + 10,
+                              &bytesRead);
+
+        if (NS_SUCCEEDED(rv5) && (X_MOZILLA_STATUS2_LEN + 10) == bytesRead)
+          ;  // OK
+        else {
+#ifdef DEBUG
+          fflush(stdout);
+          fprintf(stderr,
+                  "UpdateFolderFlag: failed read 0x%" PRIx32
+                  ", bytesRead = %d (X_MOZILLA_STATUS2_LEN+10=%d)\n",
+                  static_cast<uint32_t>(rv), bytesRead,
+                  X_MOZILLA_STATUS2_LEN + 10);
+#endif
+          return rv = NS_ERROR_FAILURE;
+        }
+
+        if (1) {
           if (strncmp(buf, X_MOZILLA_STATUS2, X_MOZILLA_STATUS2_LEN) == 0 &&
               strncmp(buf + X_MOZILLA_STATUS2_LEN, ": ", 2) == 0 &&
               strlen(buf) >= X_MOZILLA_STATUS2_LEN + 10) {
             uint32_t dbFlags;
             (void)mailHdr->GetFlags(&dbFlags);
             dbFlags &= 0xFFFF0000;
 
             // The following seek is for write.
@@ -341,40 +405,60 @@ nsresult nsMsgLocalStoreUtils::UpdateFol
             // breakdown can be done.
 #if 0
             // Currently not done.
             nsresult rv1 =  fileStream->Flush();
             NS_WARN_IF_FALSE(NS_SUCCEEDED(rv1), "fileStream->Flush() failed.");
 #endif
 
             // XXX we ought to check for Seek failure, too...
-            seekableStream->Seek(nsISeekableStream::NS_SEEK_SET, status2Pos);
+            rv5 = seekableStream->Seek(nsISeekableStream::NS_SEEK_SET,
+                                       status2Pos);
+            if (NS_FAILED(rv5)) {
+              MOZ_ASSERT(NS_SUCCEEDED(rv5),
+                         "seek: status2pos ought to have succeeded. part-3");
+              return rv5;
+            }
             PR_snprintf(buf, sizeof(buf), X_MOZILLA_STATUS2_FORMAT, dbFlags);
             nsresult rv4 =
                 fileStream->Write(buf, PL_strlen(buf), &bytesWritten);
             if (NS_FAILED(rv4) || PL_strlen(buf) != bytesWritten) {
               MOZ_ASSERT(NS_SUCCEEDED(rv4) && PL_strlen(buf) == bytesWritten,
                          "fileStream->Write(buf, PL_strlen(buf), "
                          "&bytesWritten) failed");
               // XXX better error recovery
+#ifdef DEBUG
+              fflush(stdout);
+              fprintf(stderr,
+                      "(debug) rv4 = 0x%" PRIx32
+                      ", PL_strlen(buf) = %d, bytesWritten=%d\n",
+                      static_cast<uint32_t>(rv4), PL_strlen(buf), bytesWritten);
+#endif
+              return rv4;
             }
+          } else {
+#ifdef DEBUG
+            fflush(stdout);
+            fprintf(stderr, "(debug) mozilla header line was not found?\n");
+#endif
+            rv = NS_ERROR_FAILURE;
           }
-        }
-      }
-    } else {
+        }  // if (1)
+      }    // if (flag & 0xFFFF0000)
+    }      // series of strncmp, strcmp
+    else {
 #ifdef DEBUG
       printf(
           "Didn't find %s where expected at position %ld\n"
           "instead, found %s.\n",
           X_MOZILLA_STATUS, (long)statusPos, buf);
 #endif
       rv = NS_ERROR_FAILURE;
     }
-  } else
-    rv = NS_ERROR_FAILURE;
+  }
   return rv;
 }
 
 /**
  * Returns true if there is enough space on disk.
  *
  * @param aFile  Any file in the message store that is on a logical
  *               disk volume so that it can be queried for disk space.
--- a/mailnews/local/src/nsParseMailbox.cpp
+++ b/mailnews/local/src/nsParseMailbox.cpp
@@ -296,20 +296,27 @@ nsresult nsMsgMailboxParser::ProcessMail
 
   uint32_t bytesRead = 0;
 
   if (NS_SUCCEEDED(m_inputStream.GrowBuffer(aLength))) {
     // OK, this sucks, but we're going to have to copy into our
     // own byte buffer, and then pass that to the line buffering code,
     // which means a couple buffer copies.
     // XXX TODO we should repeat Read until all the bytes are read...
-    // Would be handled by future patch. CI
+    // XXX Should we repeat Read until all the bytes are read??? I am not sure
+    // if this is desirable here. Is short read acceptable? I think the upper
+    // routine handles the retry, etc. So let it staty as it is.
     ret = aIStream->Read(m_inputStream.GetBuffer(), aLength, &bytesRead);
+
+    // Just |return ret| was what happens when !NS_SUCCEDED(ret) in the
+    // existing code. We return here explicitly.
     if (NS_SUCCEEDED(ret))
       ret = BufferInput(m_inputStream.GetBuffer(), bytesRead);
+    else
+      return ret;
   }
   if (m_graph_progress_total > 0) {
     if (NS_SUCCEEDED(ret)) m_graph_progress_received += bytesRead;
   }
   return (ret);
 }
 
 void nsMsgMailboxParser::DoneParsingFolder(nsresult status) {