The default bug view has changed. See this FAQ.

Windows test failure in test_bug460636.js due to bug 476960

RESOLVED FIXED in Thunderbird 3.0b3

Status

MailNews Core
Networking: IMAP
--
major
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: standard8, Assigned: Bienvenu)

Tracking

({regression})

Trunk
Thunderbird 3.0b3
regression
Dependency tree / graph
Bug Flags:
blocking-thunderbird3 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
Bug 476960 adjusted how we do things in an imap case. This has caused us to fail test_bug460636.js on windows. I'd normally backout straight away, however given where we are in the freeze...

I suggest we take a quick look and see if we can see if its a test issue or a issue with the patch and take appropriate action.

Tinderbox log:

make[3]: Entering directory `/e/buildbot/win32-comm-1.9.1-check/build/objdir-tb/mailnews/imap/test'
NEXT ERROR TEST-UNEXPECTED-FAIL | ../../../mozilla/_tests/xpcshell-simple/test_imap/unit/test_bug460636.js | test failed, see log
../../../mozilla/_tests/xpcshell-simple/test_imap/unit/test_bug460636.js.log:
>>>>>>>
*** test pending
Directory request for: SysD 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 pending
Directory request for: MFCaF that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: cachePDir that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: ProfLD that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: cachePDir that we (mailDirService.js) are not handling, leaving it to another handler.
Directory request for: ProfLD that we (mailDirService.js) are not handling, leaving it to another handler.
*** test finished
*** running event loop
*** exiting
NEXT ERROR *** TEST-UNEXPECTED-FAIL | e:/buildbot/win32-comm-1.9.1-check/build/mozilla/testing/xpcshell/head.js | SaveMessageToDisk did not complete within 10 seconds (incorrect messageURI?). ABORTING.
JS frame :: e:/buildbot/win32-comm-1.9.1-check/build/mozilla/testing/xpcshell/head.js :: do_throw :: line 101
JS frame :: e:/buildbot/win32-comm-1.9.1-check/build/mozilla/testing/xpcshell/head.js :: anonymous :: line 62
JS frame :: e:/buildbot/win32-comm-1.9.1-check/build/mozilla/testing/xpcshell/head.js :: anonymous :: line 62
*** FAIL ***

<<<<<<<
Flags: blocking-thunderbird3+

Comment 1

8 years ago
Might this be because TellThreadToDie is now "more async" than it was before?
(Reporter)

Comment 2

8 years ago
(In reply to comment #1)
> Might this be because TellThreadToDie is now "more async" than it was before?

Quite possibly. All I know at the moment is OnStopRunningUrl isn't being called which implies that we're not going to be successful if someone tries to save a message to disk.

Updated

8 years ago
Flags: blocking-seamonkey2.0a3+

Comment 3

8 years ago
OK, so I used my build from 12 hours ago, logged into my IMAP server, saved a message, message seems to have saved OK, so what am I missing?
(Reporter)

Comment 4

8 years ago
I just noticed, we are getting into OnStopRunningUrl, but gIMAPIncomingServer.closeCachedConnections() doesn't seem to return.
(Reporter)

Comment 5

8 years ago
Created attachment 362889 [details] [diff] [review]
Fix the test

My earlier comments on this bug were confused by windows not having symlinks.

I should have paid more attention to bienvenu's comments on tinderbox. The issue is that we are obviously not closing the file before we get to OnStopRunningUrl, but soon after. Therefore the test needs to take account of this.

We could argue we should be closing the file straight away (or before we get this far), but I think we should deal with that in a separate bug if we want to fix it properly.
Assignee: nobody → bugzilla
Attachment #362889 - Flags: review?(bienvenu)
Blocks: 477103
(Assignee)

Comment 6

8 years ago
Comment on attachment 362889 [details] [diff] [review]
Fix the test

is this needed?

+
+  let temp = gIMAPIncomingServer.rootMsgFolder;

I'm not sure if nsImapUrl or nsImapProtocol is holding onto the file.
Attachment #362889 - Flags: review?(bienvenu) → review+
(Reporter)

Comment 7

8 years ago
This has now landed (http://hg.mozilla.org/comm-central/rev/4b85ed8f77b9), bienvenu said he'd like to find out what is holding onto the file i.e. why it regressed, so assigning to him and moving to b3 for further investigation - we believe this won't affect users/extensions of TB 3 beta 2/SM 2 alpha 3.
Assignee: bugzilla → bienvenu
Flags: blocking-seamonkey2.0a3+ → blocking-seamonkey2?
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
(Reporter)

Updated

8 years ago
No longer blocks: 477103
Blocks: 477103
(Assignee)

Comment 8

8 years ago
I keep forgetting to explain this - the saveaslistener is holding onto the file. It lets go in OnStopRequest, which seems to be called after OnStopRunningUrl. The old test code closed all the cached connections synchronously, which had the side effect of closing the channel, which generated an OnStopRequest. Now closing cached connections is asynchronous, so the test broke.

There's sometimes an ordering problem between OnStopRequest and OnStopRunningUrl (e.g., the detach code), and an ordering problem between listeners for the same notification, so it's really best not to make any assumptions in the code about the order of notifications.
I think this is blocking the wrong bug
Blocks: 477222
No longer blocks: 477103
(Assignee)

Comment 10

8 years ago
now that I understand this, I don't think the remaining issue is a blocker so I'm moving off the blocking list.
Flags: blocking-thunderbird3+ → blocking-thunderbird3-
(Reporter)

Updated

8 years ago
Flags: blocking-seamonkey2?
Is this fixed ?
(In reply to Ludovic Hirlimann [:Usul] from comment #11)
> Is this fixed ?

ref: comment 8
Flags: needinfo?(irving)
Based on a quick look at comment 8 and not digging into the code at all, I don't think there's much more we can do about this bug.

It sounds similar to a lot of the problems we're having with unclean shutdowns - references being held too long, modules making poor assumptions about what order the notifications will come in - but there's nothing here specific enough to make substantial progress. I say close it.
Flags: needinfo?(irving)
(Reporter)

Comment 14

4 years ago
Ok, lets close it off.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.