MOZ_ASSERT(strlen(fCurrentLine) == 1 && fCurrentLine[0] == '\n', "Expect '\n' as only character in this line") - triggered again during last (or only) chunk

REOPENED
Assigned to
(Needinfo from 2 people)

Status

defect
--
major
REOPENED
8 months ago
7 days ago

People

(Reporter: aceman, Assigned: gds, NeedInfo)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 65.0
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 months ago
+++ This bug was initially created as a clone of Bug #1451137 +++

I just let a testing IMAP account sync messages and regularly get this TB crash:

Assertion failure: strlen(fCurrentLine) == 1 && fCurrentLine[0] == '\n' (Expect '
' as only character in this line), at mailnews/imap/src/nsImapServerResponseParser.cpp:3209

I is supposed to be fixed in bug 1451137, but I still get it on TB trunk.
(Assignee)

Comment 1

8 months ago
A few months ago I also saw this error on trunk. I have a fix for it stashed away but haven't revisited it yet or submitted a patch. I think it was caused by fetching a series of messages, each as a single chunk, and on one message the last line of the email was corrupted so that it ended in just \r and not \r\n. This caused the next email to be fetched to be assumed to have just a \n for the first line that triggers the assert.

I will test this again, duplicate it and fix it.
Assignee: nobody → gds
(Assignee)

Comment 2

7 months ago
I re-duplicated the problem I saw and tested the patch and it fixes the assert that I saw. If the assert still occurs after this patch applied, I will need more info such as the source for the email that causes the assert and other steps to causes the problem, if possible.

The problem I was seeing was because the last line of the email stored on the server ended in just \r and not \r\n. So this was being treated as a split \r\n and I was expecting to see the \n as the first line of the next chunk. But this was last chunk of message A. When I looked at message B, the assert hit because I was expecting to see just \n but instead see a normal full line.
Attachment #9019931 - Flags: review?(jorgk)
Attachment #9019931 - Flags: feedback?(acelists)
(Assignee)

Updated

7 months ago
Status: NEW → ASSIGNED

Comment 3

7 months ago
Comment on attachment 9019931 [details] [diff] [review]
1494764-ignore-corrupted-eol-on-last-line-of-last-chunk.patch

Looks reasonable. I'll run the unit tests for you locally.
Attachment #9019931 - Flags: review?(jorgk) → review+

Comment 4

7 months ago
What's the functional problem mentioned in the commit comment? Does this need backport to TB 60.x?
(Assignee)

Comment 5

7 months ago
The problem is that without the assert to cause a crash the next email opened after the one that ends with just \r fails to display. I didn't test that this time but I will to to be sure. Will let you know ASAP.

Comment 6

7 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7a29a83fb1ab
Ignore missing \n on last line of last chunk. r=jorgk
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED

Updated

7 months ago
Target Milestone: --- → Thunderbird 65.0
(Reporter)

Comment 7

7 months ago
Comment on attachment 9019931 [details] [diff] [review]
1494764-ignore-corrupted-eol-on-last-line-of-last-chunk.patch

Review of attachment 9019931 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
I have not seen the crash since this was landed.
It may also happen the problem message is no longer on the server.
I'll speak up if it happens again.
Attachment #9019931 - Flags: feedback?(acelists) → feedback+

Comment 8

4 months ago

Comment on attachment 9019931 [details] [diff] [review]
1494764-ignore-corrupted-eol-on-last-line-of-last-chunk.patch

Time to move this to ESR 60.

Attachment #9019931 - Flags: approval-comm-esr60+

Comment 10

3 months ago

Hmm, could this be causing problems: https://support.mozilla.org/en-US/questions/1249414 ?

We're doing a TB 60.5.1 next week, so I can revert that change.

Comment 11

3 months ago

Note that this is essentially a one line change:

-      if (charsReadSoFar > numberOfCharsInThisChunk)
+      if (!lastChunk && (charsReadSoFar > numberOfCharsInThisChunk))

The rest of the changes are comments and a string change.

Comment 12

3 months ago

Could you just build it Jorg and I can ask him to test?

Comment 13

3 months ago

That's possible, but I think it's pretty obvious: Change in chunking causes chunking issue. I can do a build, but I need to know the platform. Win, Mac, Linux, 32 or 64 bits? I've offered try builds to SUMO people before and they refused since Windows blocks those binaries.

(Assignee)

Comment 14

3 months ago

Oops, thought I was referring to this bug when I referenced the earlier bug with a similar title: bug 1451137. Anyhow, I can check this again with large binary non-inline attachments (e.g., pdf) since I don't see that the support reporter's file types are important and I don't have the microsoft apps to produce them.
Re: https://support.mozilla.org/en-US/questions/1249414

Comment 15

3 months ago

Any chance to get an answer here quickly. I'm getting the code for TB 60.5.1 ready right now and I need to know whether to pull this change out or not.

(Assignee)

Comment 16

3 months ago

No luck duplicating the problem and about ready to call it a day. Had the following info queued up to post when I just now saw your message come in. If you want to back it out I guess it's OK and I will still try again tomorrow to duplicate and fix the problem.

Found a large docx file (19M) that I attached and save to a message in an imap folder (no offline storage) but it fetches it in one big chunk and not multiple 98k chunks. Opens OK in Libre Office.

Another test message with an abstract art picture from the original signature bug opens fine with "close to trunk" version pulled fairly recently.

But I'm still a not sure when chunking actually occurs. I think it happen when you are fetching the whole "not modified" message and you are storing the whole thing. If individual parts are accessed, they are fetched completely and not in chunks.

I did see this ram caching error several times:

Assertion failure: !aNew (Logic error: Trying to read part from entire message which doesn't exist), 
at /home/gene/mozilla/comm/mailnews/imap/src/nsImapProtocol.cpp:9422

Seems to occur when I copy the large message from one folder to another and then try to open it before it has completely loaded. I think this is a known problem that I can fix with my prototype ram cache changes that I haven't yet submitted.

Comment 17

3 months ago

(In reply to Matt from comment #12)

Could you just build it Jorg and I can ask him to test?

Comment posted at SUMO to try
https://queue.taskcluster.net/v1/task/AcRd5mSOROO3yWDvEyrmSQ/runs/0/artifacts/public/build/install/sea/target.installer.exe

(Assignee)

Comment 18

3 months ago

Still no luck duplicating the problem. Like I've observed in the past(e.g., bug 1216951 comment29), I rarely see spontaneous fetching in multiple chunks, but typically see the fetch occur as a single large "chunk", even for attachments that exceed the chunk threshold, 98k. But not sure that a single chunk might not be a problem too since it still uses the msg_fetch_literal() code that handles multiple chunks. However, haven't seen a problem when the single big chunk is fetched -- the docx attachment opens OK.

But I have found a reliable way to "force" chunking: set mime_parts_on_demand (2 places) both to false. Then put the emails with big docx attachments into a folder that has no offline storage. Each time you clear cache and access the messages they are fetched in multiple chunks. I see no problem opening the docx file or or seeing an even larger png attachment when fetched in chunks.

I also set the chunk size smaller than the default, 65535 down to 4097, so that many more chunks occur and have no problem opening attachments.

I will try some more, but at this point I will ask the SUMO reporter to send me a sample email that supposedly won't open.

Jorg, I send a sample email to the tbtest account that you can try to open if you please. It is subject "huge docx".

Comment 19

3 months ago

That document opens fine, but then the inbox is synchronised for offline use. As you said, there must be very special circumstances that trigger the bug (if it is indeed related to the change here).

Maybe the way to experiment is to add some debug to the code to see which if/else branch runs and whether reverting the change causes a change in behaviour.

We made this change for a reason, see comment #5 (although maybe not 100% convincing).

(Assignee)

Comment 20

3 months ago

(In reply to Jorg K (GMT+1) from comment #19)

We made this change for a reason, see comment #5 (although maybe not 100% convincing).

I tried to correlate what I saw at one time to acemann's report. I probably should have asked aceman for a sample email when he reported this.

(Assignee)

Updated

3 months ago
Duplicate of this bug: 1527632
(Assignee)

Comment 23

3 months ago

The problem is that on the last or only chunk of a message, some servers (e.g., dovecot, openwave) end like this with 2 separate lines:

the last literal data\r\n
)\r\n

and others (e.g., gmail) like this with one line:

the last literal data)\r\n

In the later case, with 60.5 the closing ')' on the literal data was never removed and caused the problem. It is correctly removed with revert in 60.5.1.

(More reported info for this bug can be found starting at probably unrelated bug 570914 comment 96 and in bug 1527632 that I have marked as duplicate of this.)

Comment 24

3 months ago

Thanks for looking into it. We love server specific problems :-(

(Assignee)

Comment 25

3 months ago

At this point I probably don't suggest changing this again, at least until the original bug that this was supposed to fix is seen again. However, for possible future use, here is another way to fix the problem.

The problem with the original patch for this bug is that on the last (or only) chunk for a part or full download, if the amount downloaded exceeds the expected chunk size, as it will when the last line includes the terminating ')', the code to remove the')'is skipped causing the')'to look like another literal and be displayed.

With this diff, if the last line of the last (or only) chunk contain the terminating ')'and possibly a missing\n, the last line will still be repaired but thefNextChunksStartsWithNewLinebool will be reset to false for the next part or full download and avoid the assert.

I will continue to watch for the subject assert failure and other possible problems with and without this diff in place.

Attachment #9019931 - Attachment is obsolete: true

Updated

3 months ago
Duplicate of this bug: 1526819

Updated

3 months ago
Duplicate of this bug: 1528555

Updated

3 months ago
Duplicate of this bug: 1527833

Comment 29

3 months ago

Gene, quite a few problems reported where for when this fix was out in the wild. Any progress?

Comment 30

3 months ago

I can confirm that 60.5.1 fixed this.

(Assignee)

Comment 31

3 months ago

(In reply to Jorg K (GMT+1) from comment #29)

Gene, quite a few problems reported where for when this fix was out in the wild. Any progress?

See comment 25. Otherwise, don't know what progress you're asking about. I have only seen the problem as described in comment 1 on an imported *.eml I probably corrupted using vi editor.

Comment 32

3 months ago

OK, well, yes, we need to fix the assert failure locally. Now that we backed this out, is it back, Aceman?

Flags: needinfo?(acelists)

Comment 33

a month ago

I hit this one just now too.

Pretty fuzzy STR:
I had a couple of gmail IMAP accounts configured. One was all synced, the other was still downloading messages. I was clicking wildly on messages, using 'n', 'p', cursor keys (trying to trigger another bug :-)... and hit this assert.

I was in the debugger at the time, so I've attached a stacktrace and dumped the contents of *this, just in case.

(Assignee)

Comment 34

a month ago

Ben, Thanks for the report. Was the gmail account downloading messages a newly set-up account so all messages were being downloaded? Or was it just a couple of new messages?

Also, can you possibly attach or at least describe the email that seemed to cause the assert:

$1 = 0x7fffd3985ab0 "Content-Type: multipart/alternative; boundary=0016e6d99fb959a7930482a13924\r\n"

Maybe the boundary number is unique in the account so you might be able to find it in the mbox file. If you don't want to attach it, maybe you can tell me its size and number and type of attachments.

Ben, see comment 34

(bug 1264302 has dependencies where I want to asses the effect of chunking patches, so temporarily making this block bug 1264302)

Blocks: 1264302
Flags: needinfo?(benc)
Summary: MOZ_ASSERT(strlen(fCurrentLine) == 1 && fCurrentLine[0] == '\n', "Expect '\n' as only character in this line") - triggered again → MOZ_ASSERT(strlen(fCurrentLine) == 1 && fCurrentLine[0] == '\n', "Expect '\n' as only character in this line") - triggered again during last (or only) chunk
Blocks: 628646
You need to log in before you can comment on or make changes to this bug.