Closed Bug 656243 Opened 9 years ago Closed 9 years ago

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

Categories

(MailNews Core :: Backend, defect, major)

defect
Not set
major

Tracking

(blocking-thunderbird5.0 beta1+)

RESOLVED FIXED
Thunderbird 5.0b1
Tracking Status
blocking-thunderbird5.0 --- beta1+

People

(Reporter: standard8, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(3 files, 8 obsolete files)

128.46 KB, text/plain
Details
5.00 KB, patch
standard8
: review+
Bienvenu
: feedback+
Details | Diff | Splinter Review
3.15 KB, patch
Bienvenu
: review+
Details | Diff | Splinter Review
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.
Attached file IMAP protocol log (obsolete) —
IMAP protocol log from the test run.
Assignee: nobody → mconley
Attached patch Patch v1 (obsolete) — Splinter Review
Not sure if this is the best route to go to prevent multiple fetches, but it's the one I took.  All feedback welcome.
Attachment #531715 - Flags: review?(mbanner)
Attachment #531715 - Flags: review?(dbienvenu)
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.
Attached patch fix for crash with empty author (obsolete) — Splinter Review
this fixes the crash - or, checking if num is 0 before doing the stuff after ParseHeadersWithArray might also work.
Attachment #531734 - Attachment is obsolete: true
Attachment #531736 - Flags: review?(mconley)
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?
Attachment #531715 - Attachment is obsolete: true
Attachment #531736 - Attachment is obsolete: true
Attachment #531715 - Flags: review?(mbanner)
Attachment #531715 - Flags: review?(dbienvenu)
Attachment #531736 - Flags: review?(mconley)
Attachment #531765 - Flags: feedback?(mbanner)
Attachment #531765 - Flags: feedback?(dbienvenu)
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.
Attachment #531765 - Flags: feedback?(dbienvenu) → feedback+
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.
Attachment #531765 - Flags: feedback?(mbanner) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #531765 - Attachment is obsolete: true
Attachment #531864 - Flags: review?(dbienvenu)
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
Attachment #531765 - Attachment description: Patch v2 → [checked in] Patch v2
Attachment #531765 - Attachment is obsolete: false
Attachment #531765 - Flags: approval-thunderbird3.3+
Comment on attachment 531864 [details] [diff] [review]
Patch v3

Mike, can you attach a patch against the current trunk? thx!
Attachment #531864 - Flags: review?(dbienvenu)
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.
Attachment #531864 - Attachment is obsolete: true
Attachment #532127 - Flags: review?(dbienvenu)
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.
Severity: blocker → major
blocking-thunderbird5.0: --- → alpha4+
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.
Attachment #532127 - Flags: review?(dbienvenu) → review-
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
Attached patch Patch v5 (obsolete) — Splinter Review
I now use an nsTArray<nsCstring> of message URIs to determine whether or not a preview is being retrieved for a particular message.
Attachment #532127 - Attachment is obsolete: true
Attachment #532644 - Flags: review?(dbienvenu)
Attached patch Patch v6 (obsolete) — Splinter Review
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
Attachment #532644 - Attachment is obsolete: true
Attachment #532644 - Flags: review?(dbienvenu)
Attachment #532698 - Flags: review?(dbienvenu)
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.
Attached patch Patch v7Splinter Review
Thanks for pointing out that nit - fixed.
Attachment #532698 - Attachment is obsolete: true
Attachment #532698 - Flags: review?(dbienvenu)
Attachment #532719 - Flags: review?(dbienvenu)
Comment on attachment 532719 [details] [diff] [review]
Patch v7

works here, thx!
Attachment #532719 - Flags: review?(dbienvenu) → review+
Checked into trunk:

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

I'll do Miramar branch later.
Target Milestone: --- → Thunderbird 3.4
Pushed to Miramar as well: http://hg.mozilla.org/releases/comm-miramar/rev/97a8d24ec3cf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 3.4 → Thunderbird 3.3a4
You need to log in before you can comment on or make changes to this bug.