656243 - Permanent orange: test_imapContentLength.js | test failed (with xpcshell return code: 3) caused by Linux notification changes

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Mark Banner (:standard8)]

This is a regression from bug 555536.

On tinderbox we're seeing:

TEST-UNEXPECTED-FAIL | /buildbot/comm-central-linux-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/imap/test/unit/test_imapContentLength.js | test failed (with xpcshell return code: 3), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending
Directory request for: MailD that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: MFCaF that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: DefRt that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: IMapMD that we (mailDirService.js) are not handling, leaving it to another handler.

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop
AUTH PLAIN line -AHVzZXIAcGFzc3dvcmQ=-
authorize-id: --, username: -user-, password: -password-

TEST-PASS | /buildbot/comm-central-linux-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/imap/test/unit/test_imapContentLength.js | [verifyContentLength : 103] 1026 == 1026

TEST-PASS | /buildbot/comm-central-linux-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/imap/test/unit/test_imapContentLength.js | [verifyContentLength : 111] 1026 == 1026
AUTH PLAIN line -AHVzZXIAcGFzc3dvcmQ=-
authorize-id: --, username: -user-, password: -password-

TEST-INFO | (xpcshell/head.js) | test 1 finished

TEST-INFO | (xpcshell/head.js) | exiting test
/buildbot/comm-central-linux-opt-unittest-xpcshell/build/xpcshell/head.js:381: Error: cannot open file '/buildbot/comm-central-linux-opt-unittest-xpcshell/build/xpcshell/tests/mailnews/imap/test/unit/tail_imap.js' for reading

I've done a bit of debug and the behaviour has changed:

- I think somehow the new mail biff code is getting invoked in that test (which is probably fine).
- nsMessengerUnixIntegration sees the new message, then in FillToolTipInfo it looks to get the message preview.
- So it calls FetchMsgPreviewText, which seems to sometimes be returning as async.
- IMAP then goes off and tries to get the message, and calls back to OnStopRunningUrl
- OnStopRunningUrl then calls back to FillToolTipInfo, which calls FetchMsgPreviewText again.
- At this stage, I guess that the preview property hasn't been set on the message header, so IMAP then goes off and tries and get the message again.
- and this just repeats.

I think it stops probably because it runs out of file handles.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: IMAP protocol log

IMAP protocol log from the test run.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: IMAP protocol log - text format

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Not sure if this is the best route to go to prevent multiple fetches, but it's the one I took.  All feedback welcome.

Comment 4

[David :Bienvenu]

Because the fake server is returning "undefined" as the message body, *and* the content type is multi-part mixed, the code that tries to parse the message body for content fails, since there's no mime boundary. 

I think the fake server just doesn't handle this, fixing it might be tricky, and in any case, the new unix integration code should both respect the mail.biff.alert.show_preview, and not try to fetch the same message multiple times.

I'll try to figure out why Mike was seeing a crash running with his patch, when running the xpcshell test.

Comment 5

[David :Bienvenu]

Attachment: fix for crash with empty author

this fixes the crash - or, checking if num is 0 before doing the stuff after ParseHeadersWithArray might also work.

Comment 6

[David :Bienvenu]

Attachment: better fix for empty author crash

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: [checked in] Patch v2

This patch makes sure that we only attempt to fetch the preview if the show_preview preference is set to true.  This also integrates Bienvenu's patch that avoids a crash when notifying on e-mails with no authors.

This isn't perfect though - for some reason, when I test this out manually, the preview is no longer showing up in the notification.  Running it through GDB, it looks like the preview gets fetched, but is returning the empty string.

What's even stranger is that my notification tests are still passing...

Any ideas on what that could be all about?

Comment 8

[David :Bienvenu]

Comment on attachment 531765 [details] [diff] [review]
[checked in] Patch v2

If I change

+    mFetchingPreview = PR_TRUE;

to 
  mFetchingPreview = asyncResult;

things do work better. But it does point up the danger of using a global for this - what I would have suggested is to keep track of the keys that there are outstanding requests for, and not refetch the same key.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 531765 [details] [diff] [review]
[checked in] Patch v2

I agree with David that the global isn't ideal. Either we need something in the message that says "I tried fetching the preview but failed to translate it", or we need to track keys we've already looked for.

However, I think this patch is enough as it stands to let us get the tree green again (and open), and we can do the improvement as a follow-up (but still a blocker for the 3.3 builds).

Therefore r=me, and I'll land this with the extra change that David mentioned.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3

Ok, I think I've got it.

Apparently, I need to use FetchMsgPreviewText in order to populate the "preview" string attribute, even if it's already been fetched - I just make sure that it fetches from local, as opposed to going out and getting it from the web.

The orange test passes for me, my notification tests pass, and manual testing works as expected.

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 531765 [details] [diff] [review]
[checked in] Patch v2

Mike: not sure if you have already seen or not, but I landed this on comm-central already:

http://hg.mozilla.org/comm-central/rev/87738eac2df7

I'm also taking this onto comm-miramar so that we can have the Linux nightlies again without danger of crashing:

http://hg.mozilla.org/releases/comm-miramar/rev/35f5e4697d5c

Comment 12

[David :Bienvenu]

Comment on attachment 531864 [details] [diff] [review]
Patch v3

Mike, can you attach a patch against the current trunk? thx!

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4 - fix presence of preview text

Sorry this was late - been a busy week at UDS.

I've replaced the global boolean with an array of keys for messages we've fetched.  The xpcshell test still passes, as do my notification tests, and manual testing.

Comment 14

[David :Bienvenu]

Comment on attachment 532127 [details] [diff] [review]
Patch v4 - fix presence of preview text

I should have asked this earlier, but is it possible to get fetch requests for multiple folders going at the same time, such that the msgKey is not sufficient to distinguish between two messages? I.e., I'm fetching UID 10 for folder A and then try to fetch UID 10 for folder B and decide not to do it because the fetch for UID 10 from folder A is still outstanding? If that's a problem, you could use the URI and an nsTArray of nsCString &'s. Also, the folder really isn't "parentFolder", it's just folder, I believe.

Comment 15

[David :Bienvenu]

Comment on attachment 532127 [details] [diff] [review]
Patch v4 - fix presence of preview text

minusing because I think we need to differentiate between messages with the same UID in different folders. But if I'm wrong, please let me know.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Sorry for the late response - UDS just finished up, and we just landed back in Toronto.

> minusing because I think we need to differentiate between
> messages with the same UID in different folders. But if I'm
> wrong, please let me know.

I don't think you're wrong - I think it's a good call.  I'll put together a patch tomorrow.  Thanks for the review!

-Mike

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v5

I now use an nsTArray<nsCstring> of message URIs to determine whether or not a preview is being retrieved for a particular message.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v6

Patch v5 failed to check that an element existed in an array before attempting to remove it, and that caused some test failures.

Fixed that problem.  All tests pass on Try:  http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=966beca714ce

Comment 19

[David :Bienvenu]

Comment on attachment 532698 [details] [diff] [review]
Patch v6

looks reasonable - I'll have to run this at home tonight where I have a tree that uses nsMessengerUnixIntegration on Windows. One nit:

+  } else
+    localOnly = PR_TRUE;

else needs to be on its own line.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v7

Thanks for pointing out that nit - fixed.

Comment 21

[David :Bienvenu]

Comment on attachment 532719 [details] [diff] [review]
Patch v7

works here, thx!

Comment 22

[Mark Banner (:standard8)]

Checked into trunk:

http://hg.mozilla.org/comm-central/rev/e5dbe854dad7

I'll do Miramar branch later.

Comment 23

[Mark Banner (:standard8)]

Pushed to Miramar as well: http://hg.mozilla.org/releases/comm-miramar/rev/97a8d24ec3cf