Bug 1494764 - Removed MOZ_ASSERT but now still process line where it would occur.

MOZ_ASSERT changed to NS_WARNING. Also correctly handle case where last chunk ends with \r. Tested to make sure that regression identified in Bug 1494764 comment 10 remains fixed and that non-chunked and chunked messages are handled correctly including when \r\n is split between chunks.

Tag #1273

1 files changed, 45 insertions, 22 deletions

diff --git a/mailnews/imap/src/nsImapServerResponseParser.cpp b/mailnews/imap/src/nsImapServerResponseParser.cpp
index 68e929fe6..138ee596f 100644
--- a/mailnews/imap/src/nsImapServerResponseParser.cpp
+++ b/mailnews/imap/src/nsImapServerResponseParser.cpp
@@ -3116,10 +3116,19 @@ void nsImapServerResponseParser::ResetCapabilityFlag()
 }
 
 /*
- literal         ::= "{" number "}" CRLF *CHAR8
-                              ;; Number represents the number of CHAR8 octets
-*/
-// returns true if this is the last chunk and we should close the stream
+   literal ::= "{" number "}" CRLF *CHAR8
+   Number represents the number of CHAR8 octets
+ */
+
+// Processes a message body, header or message part fetch response. Typically
+// the full message, header or part are proccessed in one call (effectively, one
+// chunk), and parameter `chunk` is false and `origin` (offset into the
+// response) is 0. But under some conditions and larger messages, multiple calls
+// will occur to process the message in multiple chunks and parameter `chunk`
+// will be true and parameter `origin` will increase by the chunk size from
+// initially 0 with each call. This function returns true if this is the last or
+// only chunk. This signals the caller that the stream should be closed since
+// the message response has been processed.
 bool nsImapServerResponseParser::msg_fetch_literal(bool chunk, int32_t origin)
 {
   numberOfCharsInThisChunk = atoi(fNextToken + 1);
@@ -3128,13 +3137,11 @@ bool nsImapServerResponseParser::msg_fetch_literal(bool chunk, int32_t origin)
   bool lastChunk = (!chunk ||
                     (numberOfCharsInThisChunk != fServerConnection.GetCurFetchSize()));
 
-#ifdef DEBUG
   if (lastChunk)
     MOZ_LOG(IMAP, mozilla::LogLevel::Debug,
-                  ("PARSER: fetch_literal chunk = %d, requested %d, receiving %d",
-                    chunk, fServerConnection.GetCurFetchSize(),
-                    numberOfCharsInThisChunk));
-#endif
+            ("PARSER: msg_fetch_literal() chunking=%s, requested=%d, receiving=%d",
+             chunk ? "true":"false", fServerConnection.GetCurFetchSize(),
+             numberOfCharsInThisChunk));
 
   charsReadSoFar = 0;
 
@@ -3244,20 +3251,37 @@ bool nsImapServerResponseParser::msg_fetch_literal(bool chunk, int32_t origin)
       else
       {
         // Not the last line of a chunk.
-        if (!fNextChunkStartsWithNewline)
-        {
-          // Process unmodified fCurrentLine string.
+        bool processTheLine = true;
+        if (fNextChunkStartsWithNewline && origin > 0) {
+          // A split of the \r\n between chunks was detected. Ignore orphan \n
+          // on line by itself which can occur on the first line of a 2nd or
+          // later chunk. Line length should be 1 and the only character should
+          // be \n. Note: If previous message ended with just \r, don't expect
+          // the first chunk of a message (origin == 0) to begin with \n.
+          // (Typically, there is only one chunk required for a message or
+          // header response unless its size exceeds the chunking threshold.)
+          if (strlen(fCurrentLine) > 1 || fCurrentLine[0] != '\n') {
+            // In case expected orphan \n is not really there, go ahead and
+            // process the line. This should theoretically not occur but rarely,
+            // and for yet to be determined reasons, it does. Logging may help.
+            NS_WARNING(
+                "'\\n' is not the only character in this line as expected!");
+            MOZ_LOG(IMAP, mozilla::LogLevel::Debug,
+                    ("PARSER: expecting just '\\n' but line is = |%s|",
+                     fCurrentLine));
+          } else {
+            // Discard the line containing only \n.
+            processTheLine = false;
+            MOZ_LOG(IMAP, mozilla::LogLevel::Debug,
+                    ("PARSER: discarding lone '\\n'"));
+          }
+        }
+        if (processTheLine) {
           fServerConnection.HandleMessageDownLoadLine(fCurrentLine,
             !lastChunk && (charsReadSoFar == numberOfCharsInThisChunk),
             fCurrentLine);
         }
-        else
-        {
-          // Ignore the orphan '\n' on a line by itself.
-          MOZ_ASSERT(strlen(fCurrentLine) == 1 && fCurrentLine[0] == '\n',
-                     "Expect '\\n' as only character in this line");
-          fNextChunkStartsWithNewline = false;
-        }
+        fNextChunkStartsWithNewline = false;
       }
     }
   }
@@ -3276,9 +3300,8 @@ bool nsImapServerResponseParser::msg_fetch_literal(bool chunk, int32_t origin)
       skip_to_CRLF();
       AdvanceToNextToken();
     }
-  }
-  else
-  {
+  } else {
+    // Don't typically (maybe never?) see this.
     fNextChunkStartsWithNewline = false;
   }
   return lastChunk;