Last Comment Bug 704707 - don't need event target (thread) when running imap urls
: don't need event target (thread) when running imap urls
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Thunderbird 12.0
Assigned To: David :Bienvenu
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-22 17:58 PST by David :Bienvenu
Modified: 2012-01-10 07:37 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (137.35 KB, patch)
2011-11-22 18:02 PST, David :Bienvenu
neil: review+
standard8: superreview-
Details | Diff | Splinter Review
de-bitrotted patch (97.23 KB, patch)
2011-12-26 17:16 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
fix for unit tests (153.75 KB, patch)
2012-01-09 14:36 PST, David :Bienvenu
standard8: superreview+
Details | Diff | Splinter Review

Description David :Bienvenu 2011-11-22 17:58:33 PST

    
Comment 1 David :Bienvenu 2011-11-22 18:00:57 PST
When we got rid of xpcom proxies, we could have also gotten rid of the event target stuff. This had a rather large ripple effect, but it gets rid of an unneeded parameter/member variable in a bunch of places.
Comment 2 David :Bienvenu 2011-11-22 18:02:11 PST
Created attachment 576374 [details] [diff] [review]
proposed fix

it's a rather large patch, but very simple.
Comment 3 neil@parkwaycc.co.uk 2011-11-24 16:27:14 PST
Comment on attachment 576374 [details] [diff] [review]
proposed fix

Haven't run any tests, but it opened my Inbox OK.

>+    rv = imapService->DiscoverChildren( this, this, m_onlineFolderName, nsnull);
Nit: extraneous space after (

> {
>+
>+  return ChangeFolderSubscription(aFolder, aFolderName, 
>                                   "/subscribe>", urlListener, url);
Nit: extraneous blank line after {
Comment 4 David :Bienvenu 2011-11-30 14:47:01 PST
pinging for sr; I'd like to land this relatively soon, to avoid conflicts...
Comment 5 Mark Banner (:standard8) 2011-12-02 02:54:46 PST
Comment on attachment 576374 [details] [diff] [review]
proposed fix

Review of attachment 576374 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, but for some reason, I'm seeing lots of failures in debug mode, specifically in:

mailnews/base/test/unit/test_imapPump.js
mailnews/imap/test/unit/test_filterCustomHeaders.js
mailnews/imap/test/unit/test_imapMove.js
mailnews/imap/test/unit/test_preserveDataOnMove.js
mailnews/imap/test/unit/test_saveImapDraft.js
mailnews/imap/test/unit/test_saveTemplate.js

These don't seem to happen without the patch applied. Although I'm a bit confused as functionally you don't seem to be changing that much.

It looks like it is generally the shutdown of the test that is the issue.

::: mailnews/local/src/nsLocalUndoTxn.cpp
@@ +157,5 @@
> +    // folder so use lite select to do the trick
> +    rv = imapService->LiteSelectFolder(folder,
> +                                       urlListener, nsnull, nsnull);
> +    if (!deleteFlag)
> +        rv =imapService->AddMessageFlags(folder,

nit: needs a space after equals.
Comment 6 David :Bienvenu 2011-12-02 07:25:54 PST
hmm, I guess that's what I get for running the tests in release mode. I'll look into it.
Comment 7 David :Bienvenu 2011-12-02 11:48:12 PST
tests are all working fine here - I'll try try server.
Comment 8 David :Bienvenu 2011-12-05 07:00:05 PST
try server worked fine, but try server doesn't finish debug builds, so it wasn't so helpful. I still can't reproduce any test failures, and since I'm essentially only removing an unused paramter, it's hard to believe that the test failures are caused by my patch.
Comment 9 David :Bienvenu 2011-12-08 15:57:30 PST
OK, I do see those test failures on mac. Suspiciously, I think most if not all of the failing tests use IMAPPump.js
Comment 10 David :Bienvenu 2011-12-26 17:16:26 PST
Created attachment 584359 [details] [diff] [review]
de-bitrotted patch

this applies on the trunk, and no longer do I see the test failures on debug mac builds, so I suspect the pluggable store landing changed some timing issues. I'll request a try server build with this patch.
Comment 11 David :Bienvenu 2011-12-28 20:50:06 PST
The test failures seem to be race conditions. test_imapPump.js needs to turn off autosync, or an autosync can start during shutdown, which leads to an assertion. test_filterCustomHeaders has a mark read filter, which starts a url that runs during shutdown, again causing an assertion. I suspect the other tests have similar issues.
Comment 12 David :Bienvenu 2012-01-09 14:36:52 PST
Created attachment 587157 [details] [diff] [review]
fix for unit tests

This fixes all the unit tests for me, using several approaches:

1. Make sure auto sync is turned off for tests
2. Abort queued urls at fake server shutdown.
3. Don't try to run queued urls if we're offline
4. For tests that run "extra" urls, do a performTest() on the expected command

I'll generate a try server build with this patch, though I'm not having a lot of luck getting the try server to report actual tests results at the moment (times out)
Comment 14 Mark Banner (:standard8) 2012-01-10 04:18:05 PST
Comment on attachment 587157 [details] [diff] [review]
fix for unit tests

Review of attachment 587157 [details] [diff] [review]:
-----------------------------------------------------------------

Neil already gave r, so I'll make this sr ;-)
Comment 15 David :Bienvenu 2012-01-10 07:37:30 PST
fixed - http://hg.mozilla.org/comm-central/rev/25d62c5e3121

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