Last Comment Bug 479011 - Windows test failure in test_bug460636.js due to bug 476960
: Windows test failure in test_bug460636.js due to bug 476960
Status: RESOLVED FIXED
: regression
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: All All
: -- major (vote)
: Thunderbird 3.0b3
Assigned To: David :Bienvenu
:
:
Mentors:
Depends on:
Blocks: 476960 477222
  Show dependency treegraph
 
Reported: 2009-02-18 00:39 PST by Mark Banner (:standard8, afk until Dec)
Modified: 2012-11-12 01:10 PST (History)
9 users (show)
mozilla: blocking‑thunderbird3-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Fix the test (1.54 KB, patch)
2009-02-18 06:49 PST, Mark Banner (:standard8, afk until Dec)
mozilla: review+
Details | Diff | Splinter Review

Description Mark Banner (:standard8, afk until Dec) 2009-02-18 00:39:22 PST
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 ***

<<<<<<<
Comment 1 neil@parkwaycc.co.uk 2009-02-18 01:07:30 PST
Might this be because TellThreadToDie is now "more async" than it was before?
Comment 2 Mark Banner (:standard8, afk until Dec) 2009-02-18 03:41:14 PST
(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.
Comment 3 neil@parkwaycc.co.uk 2009-02-18 04:30:22 PST
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?
Comment 4 Mark Banner (:standard8, afk until Dec) 2009-02-18 06:19:32 PST
I just noticed, we are getting into OnStopRunningUrl, but gIMAPIncomingServer.closeCachedConnections() doesn't seem to return.
Comment 5 Mark Banner (:standard8, afk until Dec) 2009-02-18 06:49:17 PST
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.
Comment 6 David :Bienvenu 2009-02-18 08:04:06 PST
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.
Comment 7 Mark Banner (:standard8, afk until Dec) 2009-02-18 08:33:39 PST
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.
Comment 8 David :Bienvenu 2009-02-20 12:29:32 PST
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.
Comment 9 Bryan Clark (DevTools PM) [:clarkbw] 2009-02-26 12:45:04 PST
I think this is blocking the wrong bug
Comment 10 David :Bienvenu 2009-03-03 12:46:42 PST
now that I understand this, I don't think the remaining issue is a blocker so I'm moving off the blocking list.
Comment 11 Ludovic Hirlimann [:Usul] 2009-04-20 00:23:19 PDT
Is this fixed ?
Comment 12 Wayne Mery (:wsmwk, NI for questions) 2012-10-25 14:58:37 PDT
(In reply to Ludovic Hirlimann [:Usul] from comment #11)
> Is this fixed ?

ref: comment 8
Comment 13 :Irving Reid (No longer working on Firefox) 2012-11-08 12:50:44 PST
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.
Comment 14 Mark Banner (:standard8, afk until Dec) 2012-11-12 01:10:22 PST
Ok, lets close it off.

Note You need to log in before you can comment on or make changes to this bug.