Mark Banner (:standard8) Reporter
	Description • 16 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 ***

<<<<<<<

	neil@parkwaycc.co.uk
	Comment 1 • 16 years ago

Might this be because TellThreadToDie is now "more async" than it was before?

	Mark Banner (:standard8) Reporter
	Comment 2 • 16 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.

	neil@parkwaycc.co.uk
	Comment 3 • 16 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?

	Mark Banner (:standard8) Reporter
	Comment 4 • 16 years ago

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

	Mark Banner (:standard8) Reporter
	Comment 5 • 16 years ago

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.

	David :Bienvenu Assignee
	Comment 6 • 16 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.

	Mark Banner (:standard8) Reporter
	Comment 7 • 16 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.

	David :Bienvenu Assignee
	Comment 8 • 16 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.

	Bryan Clark (DevTools PM) [:clarkbw]
	Comment 9 • 16 years ago

I think this is blocking the wrong bug

	David :Bienvenu Assignee
	Comment 10 • 16 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.

	Ludovic Hirlimann [:Usul]
	Comment 11 • 16 years ago

Is this fixed ?

	Wayne Mery (:wsmwk)
	Comment 12 • 12 years ago

(In reply to Ludovic Hirlimann [:Usul] from comment #11)
> Is this fixed ?

ref: comment 8

	:Irving Reid (No longer working on Firefox)
	Comment 13 • 12 years ago

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.

	Mark Banner (:standard8) Reporter
	Comment 14 • 12 years ago

Ok, lets close it off.